Retry triggers in trace ui

[shagun-singh-inkeep]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agents-api	Ready	Preview, Comment	Feb 9, 2026 11:24pm
agents-docs	Ready	Preview, Comment	Feb 9, 2026 11:24pm
agents-manage-ui	Ready	Preview, Comment	Feb 9, 2026 11:24pm

[changeset-bot]

🦋 Changeset detected

Latest commit: eb7f8e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
@inkeep/agents-core	Patch
@inkeep/agents-manage-ui	Patch
@inkeep/agents-api	Patch
@inkeep/agents-cli	Patch
@inkeep/agents-sdk	Patch
@inkeep/agents-work-apps	Patch
@inkeep/ai-sdk-provider	Patch
@inkeep/create-agents	Patch
@inkeep/agents-manage-mcp	Patch
@inkeep/agents-mcp	Patch

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

[claude]

PR Review Summary

5 Key Findings | Risk: Medium

🟠⚠️ Major (3) 🟠⚠️

🟠 1) triggers.ts:758 Unsafe type cast bypasses type safety

Issue: The code casts messageParts to any when calling dispatchExecution. This bypasses TypeScript's type checking and could lead to runtime errors if the data structure doesn't match what dispatchExecution expects (which expects Part[] from @inkeep/agents-core).

Why: Type safety is lost, and there's no validation that the incoming parts match the expected schema. If a malformed message part is passed, it could cause silent failures or unexpected behavior deep in the execution pipeline.

Fix: Define a proper type guard or validation for messageParts before passing to dispatchExecution:

const partsSchema = z.array(z.object({ 
  kind: z.enum(['text', 'data', 'image', 'file']),
  text: z.string().optional(),
  data: z.unknown().optional(),
  metadata: z.record(z.string(), z.unknown()).optional()
}));

const validatedParts = partsSchema.parse(messageParts);

Refs: triggers.ts:758 · Part type in agents-core

---

🟠 2) agents-api/src/__tests__/ Missing test coverage for rerun endpoint

Issue: The existing test file (triggers.test.ts) has comprehensive tests for CRUD operations and invocations, but there are no tests for the new POST /{id}/rerun endpoint.

Why: This endpoint has several edge cases that should be tested: successful rerun, rerun with messageParts, trigger not found, disabled trigger, permission checks. Without tests, regressions could be introduced in future changes.

Fix: Add a new describe block for POST /{id}/rerun with tests covering:

• Successful rerun returns 202 with invocationId and conversationId
• Rerun with messageParts preserves the original structure
• 404 for non-existent trigger
• 400 for disabled trigger (after the fix above)
• Permission validation (requires 'use' permission)

Refs: triggers.test.ts

---

🟠 3) triggers.ts:756 Reruns not distinguishable in traces

Issue: The { _rerun: true } flag is passed to dispatchExecution as payload, which gets stored in requestPayload in the database. However, this flag is NOT propagated to the OpenTelemetry span attributes. The spans set 'invocation.type': 'trigger' but do not include any attribute to distinguish reruns from original invocations.

Why: This makes it difficult to filter/analyze rerun behavior in observability tools and during incident investigations. When debugging production issues, operators cannot easily distinguish "was this a rerun or an original invocation?"

Fix: Add a span attribute such as 'trigger.is_rerun': true in the executeAgentAsync function when the payload contains _rerun: true. Alternatively, consider passing an isRerun boolean parameter through dispatchExecution rather than relying on payload inspection.

Refs: TriggerService.ts:641-654

🟡 Minor (2) 🟡

🟡 1) triggers.ts:750 No idempotency protection for rapid reruns

Issue: Multiple rapid clicks on the 'Rerun Trigger' button (or rapid API calls) will each create a new conversation and invocation record, potentially leading to duplicate agent executions.

Fix: Consider adding an optional idempotencyKey parameter, or server-side rate limiting per trigger/user.

---

🟡 2) TriggerService.ts:520 No trace lineage tracking

Issue: When a rerun is triggered, there is no link stored between the original conversationId and the new rerun's conversationId, making it difficult to understand trace lineage during debugging.

Fix: Consider adding an optional sourceConversationId field to enable trace lineage queries.

📌 Inline Comments (2)

• 🟠 triggers.ts:744 Missing check for disabled trigger
• 🟡 page.tsx:117-121 Silent JSON parse failure

---

💡 APPROVE WITH SUGGESTIONS

Summary: This PR implements a well-structured "Rerun Trigger" feature with clean separation between the API endpoint, service layer, and UI components. The core functionality is solid. The main areas for improvement are: (1) adding a disabled trigger check for consistency with webhook behavior, (2) adding test coverage for the new endpoint, and (3) improving observability to distinguish reruns in traces. None of these are blocking, but addressing the disabled trigger check and test coverage would strengthen the implementation.

Other Findings (5)

Potentially valid

(these are minor or info and not fully confident)

Location	Issue	Reason Excluded
triggers.ts:746	Default messageParts structure may not match Part type exactly	Low impact - fallback behavior is reasonable
api/triggers.ts:141	Missing agentId/triggerId validation in API client	Validated at API level, client-side validation is nice-to-have
page.tsx:104	Guard clause finds first user_message, may need clarification	Intent is clear enough, existing behavior is reasonable
actions/triggers.ts:121	No revalidatePath call after rerun	Rerun doesn't modify trigger, so revalidation may not be necessary
TriggerService.ts:561	Fire-and-forget lacks tracking in non-Vercel	Existing pattern, not introduced by this PR

Discarded as invalid or not applicable

Location	Issue	Reason Excluded
types.ts:11	Unused 'trigger_invocation' PanelType	Pre-existing code, not introduced by this PR

Reviewer Stats

Reviewer	Returned	Inline Comments	Main Findings	Pending Recs	Other Findings
general-purpose	13	1	2	0	4
pr-review-sre	5	1	1	0	2
Total	18	2	3	0	6

[claude]

PR Review Summary

4 Key Findings | Risk: Medium

🔴❗ Critical (1) ❗🔴

🔴 1) triggers.ts:758 Unsafe type cast bypasses type safety for Part[] discriminated union

Issue: The code casts messageParts to any when calling dispatchExecution. The request schema accepts z.array(z.record(z.string(), z.unknown())) which allows arbitrary object structures. This data is then passed to dispatchExecution which expects Part[] — a discriminated union requiring kind: 'text' | 'file' | 'data' with specific required fields per variant. Invalid message parts (missing kind, wrong shape) will pass validation and cause runtime errors deep in the execution pipeline.

Why: If a client sends malformed parts like [{ "text": "hello" }] (missing kind) or [{ "kind": "file", "data": "not-a-file" }] (wrong file shape), the API accepts them at the 202 response, but agent execution fails later with cryptic errors. These failures occur after invocation records are created, making debugging difficult.

Fix: Define a Zod schema matching the Part discriminated union and validate before passing to dispatchExecution:

import type { Part } from '@inkeep/agents-core';

const PartSchema = z.discriminatedUnion('kind', [
  z.object({
    kind: z.literal('text'),
    text: z.string(),
    metadata: z.record(z.string(), z.unknown()).optional(),
  }),
  z.object({
    kind: z.literal('data'),
    data: z.record(z.string(), z.unknown()),
    metadata: z.record(z.string(), z.unknown()).optional(),
  }),
  z.object({
    kind: z.literal('file'),
    file: z.union([
      z.object({ bytes: z.string(), name: z.string().optional(), mimeType: z.string().optional() }),
      z.object({ uri: z.string(), name: z.string().optional(), mimeType: z.string().optional() }),
    ]),
    metadata: z.record(z.string(), z.unknown()).optional(),
  }),
]);

const messageParts: Part[] = rawMessageParts
  ? PartSchema.array().parse(rawMessageParts)  // Throws 400 if invalid
  : [{ kind: 'text', text: userMessage }];

Refs: triggers.ts:758 · Part type in a2a.ts

🟠⚠️ Major (2) 🟠⚠️

🟠 1) agents-api/src/__tests__/ Missing test coverage for rerun endpoint

Issue: The new POST /{id}/rerun endpoint has zero test coverage. Per AGENTS.md, all new code paths require test coverage.

Why: This endpoint has several edge cases: successful rerun (with/without messageParts), 404 for non-existent trigger, 404 for disabled trigger (after fixing that issue), permission validation, and tenant isolation. Without tests, regressions could be silently introduced.

Fix: Add a describe('POST /{id}/rerun') block in triggers.test.ts with tests for:

• Successful rerun returns 202 with invocationId and conversationId
• Rerun with messageParts preserves original structure
• 404 for non-existent trigger
• 404 for disabled trigger
• Permission validation (requires 'use' permission)
• Tenant isolation

Refs: triggers.test.ts

---

🟠 2) triggers.ts:750-760 No try-catch around dispatchExecution

Issue: If dispatchExecution throws (database constraint violation, connection timeout), the exception propagates unhandled. Users receive generic 500 errors with potentially exposed stack traces.

Why: Database errors like "connection refused" or "constraint violation" leak internal details. Operators cannot distinguish between database unavailability, invalid trigger state, or configuration errors.

Fix: Wrap dispatchExecution call in try-catch with proper error mapping:

try {
  const { invocationId, conversationId } = await dispatchExecution({...});
  return c.json({ success: true, invocationId, conversationId }, 202);
} catch (error) {
  logger.error({ err: error, tenantId, triggerId }, 'Failed to dispatch trigger rerun');
  throw createApiError({
    code: 'internal_server_error',
    message: 'Failed to dispatch trigger execution',
  });
}

Refs: triggers.ts:750-760

🟡 Minor (1) 🟡

🟡 1) TriggerService.ts Reruns not distinguishable in traces

Issue: The { _rerun: true } flag is passed to dispatchExecution as payload but is NOT propagated to OpenTelemetry span attributes. Spans show 'invocation.type': 'trigger' but don't distinguish reruns from original invocations.

Fix: Add a span attribute like 'trigger.is_rerun': true in executeAgentAsync when payload contains _rerun: true, or pass an explicit isRerun boolean through dispatchExecution.

Refs: TriggerService.ts:641-654

📌 Inline Comments (2)

• 🟠 triggers.ts:744 Missing check for disabled trigger
• 🟡 page.tsx:117-121 Silent JSON parse failure

---

💡 APPROVE WITH SUGGESTIONS

Summary: This PR implements a well-structured "Rerun Trigger" feature with clean separation between API, service, and UI layers. The core functionality is solid. The main areas for improvement are: (1) adding proper Zod validation for messageParts to match the Part discriminated union, (2) adding test coverage for the new endpoint, and (3) adding the disabled trigger check for consistency with webhook behavior. None are blocking, but the type safety issue (CRITICAL) could cause runtime failures that are hard to debug.

Other Findings (8)

Potentially valid

(minor or info severity, not fully confident)

Location	Issue	Reason Excluded
triggers.ts:746	Fallback text part construction relies on structural coincidence	Works correctly, but could use explicit type annotation
types.ts:219-221	Nullability pattern (string | null) differs from Activity interface (string | undefined)	Minor inconsistency, runtime behavior same
triggers.ts:715-719	Response envelope { success, invocationId, conversationId } without data wrapper	Consistent with webhook invocation style, not CRUD style
TriggerService.ts:558	waitUntil() on Vercel has no error handling for early executeAgentAsync failures	Existing pattern, not introduced by this PR
actions/triggers.ts:132	Generic error message doesn't differentiate API vs network errors	Minor UX issue

Discarded as invalid or not applicable

Location	Issue	Reason Excluded
api/triggers.ts:143-144	Missing agentId/triggerId validation in UI client	Consistent with peer functions in same file
types.ts:81	messageParts as string without type constraint	Existing pattern for JSON-serialized DB fields
Security/IAM concerns	Permission bypass or tenant isolation issues	Reviewed and found sound - 'use' permission is appropriate

Reviewer Stats

Reviewer	Returned	Inline Comments	Main Findings	Pending Recs	Other Findings
pr-review-standards	3	1	1	0	0
pr-review-types	4	1	1	0	2
pr-review-tests	5	0	1	0	3
pr-review-errors	6	0	1	0	3
pr-review-consistency	5	0	0	0	5
pr-review-frontend	0	0	0	0	0
pr-review-security-iam	0	0	0	0	0
Total	23	2	4	0	13

[claude]

MINOR Silent JSON parse failure loses structured data without indication

When messageParts contains malformed JSON, the catch block silently falls back to text-only mode. Users lose file/image/data parts without any indication that something went wrong during rerun.

Suggested change

	try {
	messageParts = JSON.parse(userMessageActivity.messageParts);
	} catch {
	// Fall back to text-only
	}
	try {
	messageParts = JSON.parse(userMessageActivity.messageParts);
	} catch (parseError) {
	console.warn('Failed to parse messageParts for rerun, falling back to text-only', {
	conversationId,
	error: parseError instanceof Error ? parseError.message : String(parseError),
	});
	}

[amikofalvy]

We should issue a warn level log message. Something that will be caught by Sentry and send us an alert to investigate.

[claude]

PR Review Summary

2 Key Findings | Risk: Low | Most prior feedback addressed ✅

Progress Since Last Review ✅

The latest commits have addressed several previously raised concerns:

• ✅ PartSchema created in agents-core (commit 26055cf9) — Addresses human reviewer's request to use existing schemas
• ✅ 409 Conflict for disabled triggers (code shows code: 'conflict') — Addresses human reviewer feedback
• ✅ Changeset updated (commits 2009193c, b9eed50f) — Now lists only 3 packages (agents-core, agents-manage-ui, agents-api) with descriptive message
• ✅ Stack trace included in error logging (lines 772-774) — Now includes errorStack

🟠⚠️ Major (1) 🟠⚠️

🟠 1) packages/agents-core/src/validation/schemas.ts:100-115 FilePartSchema allows illegal state

Issue: The new FilePartSchema uses z.union([FileWithBytesSchema, FileWithUriSchema]) but neither sub-schema forbids the other's required field. The TypeScript types in a2a.ts use uri?: never and bytes?: never to enforce mutual exclusivity, but the Zod schemas don't enforce this.

Why: A file part like { kind: 'file', file: { bytes: 'abc', uri: 'https://...' } } would pass Zod validation. The union picks the first match (FileWithBytesSchema), keeping both properties in passthrough mode. This creates ambiguity: should downstream code use inline bytes or fetch from URI?

Fix: Use .strict() to reject extra properties:

const FileWithBytesSchema = z.object({
  name: z.string().optional(),
  mimeType: z.string().optional(),
  bytes: z.string(),
}).strict();  // Rejects 'uri'

const FileWithUriSchema = z.object({
  name: z.string().optional(),
  mimeType: z.string().optional(),
  uri: z.string(),
}).strict();  // Rejects 'bytes'

Refs: a2a.ts:15-23 — TypeScript types with never constraints

🟡 Minor (1) 🟡

Inline comments:

• 🟡 Minor: triggers.ts:724 OpenAPI spec missing 409 response for disabled trigger

🕐 Pending Recommendations (4)

From automated review threads (still valid):

• 🟠 triggers.ts:779 — Error message exposes raw internal details to API consumers
• 🟡 page.tsx:117-121 — Silent JSON parse failure loses structured data without indication

From previous reviews:

• 🟠 scope: tests — Missing test coverage for POST /{id}/rerun endpoint (AGENTS.md requires tests for new features)

---

💡 APPROVE WITH SUGGESTIONS

Summary: The "Rerun Trigger" feature is well-structured with clean separation between API, service, and UI layers. The latest commits addressed most of the human reviewer's feedback (PartSchema reuse, 409 status code, changeset scope). The remaining concerns are: (1) FilePartSchema should enforce mutual exclusivity to match TypeScript types, (2) the error message at line 779 still exposes raw internal details, and (3) test coverage is still missing. None are blocking — the feature works correctly — but addressing them would strengthen the implementation.

Discarded (4)

Location	Issue	Reason Discarded
TriggerService.ts:641-654	Reruns not distinguishable in OTEL traces	Architectural scope — would require changes to dispatchExecution signature and span attributes; not actionable in this PR
Response envelope style	Action-style 202 response without data wrapper	Follows existing webhook endpoint pattern — consistent with peer code
Inline request body schema	Schema defined inline vs named schema	Matches webhook endpoint pattern for action endpoints
page.tsx:142	Non-null assertion in closure	Now uses optional chaining result.data?.conversationId — already fixed

Reviewers (6)

Reviewer	Returned	Main Findings	Consider	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0
pr-review-consistency	4	0	0	1	0	2
pr-review-tests	1	0	0	0	1	0
pr-review-types	1	1	0	0	0	0
pr-review-frontend	0	0	0	0	0	0
pr-review-errors	0	0	0	0	0	0
Total	6	1	0	1	1	2

[claude]

PR Review Summary

1 Key Finding | Risk: Low | Most prior feedback addressed ✅

Progress Since Last Review ✅

The latest commits have addressed the majority of human reviewer feedback:

• ✅ PartSchema created in agents-core (commit 26055cf9) — Now uses proper discriminated union schema
• ✅ 409 Conflict for disabled triggers (commit da44e3c8) — Error code changed from not_found to conflict
• ✅ Changeset updated (commits 2009193c, b9eed50f) — Now lists 3 packages with descriptive message
• ✅ Stack trace in error logging (lines 773-774) — Now includes errorStack
• ✅ OpenAPI 409 response documented (line 725) — Added 409: errorSchemaFactory('conflict', 'Trigger is disabled')

🟠⚠️ Major (1) 🟠⚠️

Inline comments:

• 🟠 Major: schemas.ts:100-110 FilePartSchema allows illegal state with both bytes and uri (see inline comment with 1-click fix)

🕐 Pending Recommendations (4)

From automated review threads (still valid):

• 🟠 triggers.ts:781 — Error message exposes raw internal details to API consumers
• 🟡 page.tsx:117-121 — Silent JSON parse failure loses structured data without indication

From previous reviews:

• 🟠 scope: tests — Missing test coverage for POST /{id}/rerun endpoint (AGENTS.md requires tests for new features)

Human reviewer threads (verify resolution):

• 🟠 triggers.ts:703 — Human reviewer requested using existing Part schemas from agents-core — ✅ appears addressed with new PartSchema

💭 Consider

💭 1) triggers.ts:790 Response envelope follows async action pattern
Issue: Returns { success, invocationId, conversationId } directly without data: wrapper.
Why: This is consistent with webhook endpoint (webhooks.ts:153-156) and other 202 async action endpoints. No change needed — just noting this follows the established pattern for async dispatch confirmations vs CRUD operations.

---

💡 APPROVE WITH SUGGESTIONS

Summary: The core "Rerun Trigger" feature is well-structured and the recent commits have addressed most of the human reviewer's feedback. The only new finding is the FilePartSchema mutual exclusivity issue (inline comment with 1-click fix). The remaining pending items from prior reviews (error message exposure, silent JSON parse, test coverage) are still valid but not blocking. Nice work addressing the feedback iteratively! 🎉

Discarded (4)

Location	Issue	Reason Discarded
triggers.ts:717-721	Response schema lacks .openapi() registration	Consistent with webhook endpoint pattern (webhooks.ts:51-55) — intentional for inline action schemas
page.tsx:140-144	Toast closure captures reference	Code now uses result.data?.conversationId which fails safely — already addressed
TriggerService.ts	Reruns not distinguishable in OTEL traces	Architectural scope — would require changes to dispatchExecution signature; not actionable in this PR
triggers.ts:726	OpenAPI 409 already documented	Human reviewer's concern addressed — line 725 now has 409: errorSchemaFactory('conflict', 'Trigger is disabled')

Reviewers (6)

Reviewer	Returned	Main Findings	Consider	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0
pr-review-consistency	3	0	1	1	0	1
pr-review-tests	1	0	0	0	1	0
pr-review-types	1	0	0	1	0	0
pr-review-errors	0	0	0	0	0	0
pr-review-frontend	2	0	0	0	1	1
Total	7	0	1	2	2	2

---

View job run

[claude]

MAJOR FilePartSchema allows illegal state with both bytes and uri

Issue: The Zod schemas don't enforce mutual exclusivity between bytes and uri. An object like { bytes: 'abc', uri: 'https://...' } passes validation because z.union() picks the first match and Zod objects allow extra properties by default.

Why: The TypeScript types in a2a.ts:15-23 use uri?: never and bytes?: never to forbid the other property at compile time. This Zod schema should enforce the same constraint at runtime. Without this, API consumers could send ambiguous file parts where downstream code wouldn't know whether to decode base64 bytes or fetch from URI.

Fix: Add .strict() to reject extra properties:

Suggested change

	const FileWithBytesSchema = z.object({
	name: z.string().optional(),
	mimeType: z.string().optional(),
	bytes: z.string(),
	});
	const FileWithUriSchema = z.object({
	name: z.string().optional(),
	mimeType: z.string().optional(),
	uri: z.string(),
	});
	const FileWithBytesSchema = z.object({
	name: z.string().optional(),
	mimeType: z.string().optional(),
	bytes: z.string(),
	}).strict();
	const FileWithUriSchema = z.object({
	name: z.string().optional(),
	mimeType: z.string().optional(),
	uri: z.string(),
	}).strict();

Refs:

• a2a.ts:15-23 — TypeScript types with never constraints

[amikofalvy]

@shagun-singh-inkeep I think we should do this suggestion

[claude]

PR Review Summary

1 Key Finding | Risk: Low | Human feedback addressed ✅

Progress Since Last Review ✅

The latest commits have addressed human reviewer feedback:

• ✅ PartSchema created — New discriminated union schema in packages/agents-core/src/validation/schemas.ts
• ✅ 409 Conflict for disabled triggers — Changed from not_found to conflict
• ✅ Changeset updated — Now lists 3 packages (agents-core, agents-manage-ui, agents-api) with descriptive message
• ✅ OpenAPI 409 documented — Added 409: errorSchemaFactory('conflict', 'Trigger is disabled')
• ✅ Stack trace in error logging — Now includes errorStack

🟠⚠️ Major (1) 🟠⚠️

🟠 1) scope: tests Missing test coverage for POST /{id}/rerun endpoint

Issue: The new rerun endpoint (lines 689-792 in triggers.ts) has zero test coverage. AGENTS.md requires: "ALL new work MUST include... Unit Tests" and "All new code paths must have test coverage."

Why: Without tests, regressions could go undetected for: permission validation bypass (security), disabled trigger check removal, messageParts fallback logic, and error handling. The existing triggers.test.ts has comprehensive tests for other operations but none for rerun.

Fix: Add a new describe('POST /{id}/rerun') block in agents-api/src/__tests__/manage/routes/crud/triggers.test.ts covering:

• Successful rerun with userMessage only → 202
• Rerun with messageParts preserved → 202
• Non-existent trigger → 404
• Disabled trigger → 409
• Tenant isolation
• Permission validation ('use' permission required)

Refs:

• triggers.test.ts
• AGENTS.md testing requirements

🕐 Pending Recommendations (3)

From automated review threads (still unresolved):

• 🟠 schemas.ts:100-110 — FilePartSchema allows illegal state with both bytes and uri (add .strict() to enforce mutual exclusivity)
• 🟠 triggers.ts:781 — Error message exposes raw internal details to API consumers
• 🟡 page.tsx:117-121 — Silent JSON parse failure loses structured data without indication

💭 Consider

💭 1) triggers.ts:715-721 Response envelope follows async action pattern
Issue: Returns { success, invocationId, conversationId } directly without { data: ... } wrapper.
Why: This matches the existing webhook invocation pattern (webhooks.ts:153-156) and other 202 async action endpoints. No change needed — just noting this is intentional.

---

💡 APPROVE WITH SUGGESTIONS

Summary: The core "Rerun Trigger" feature is well-implemented with clean separation between API, service, and UI layers. Human reviewer feedback has been addressed (PartSchema, 409 status, changeset). Three prior automated comments remain unresolved: (1) FilePartSchema .strict() for mutual exclusivity, (2) error message sanitization, and (3) test coverage. The FilePartSchema and error message issues have 1-click suggestions in their threads. Test coverage is the primary gap — the new endpoint has several edge cases that should be tested before merge.

Discarded (3)

Location	Issue	Reason Discarded
TriggerService.ts	Reruns not distinguishable in OTEL traces	Architectural scope — would require changes to dispatchExecution signature; not actionable in this PR
triggers.ts:137	Action error handling propagates raw messages	Second-order effect of error message exposure issue at API layer — fixing the API would resolve this
Response envelope	Action-style 202 without data wrapper	Consistent with webhook endpoint pattern — intentional design

Reviewers (4)

Reviewer	Returned	Main Findings	Consider	Inline Comments	Pending Recs	Discarded
pr-review-types	1	0	0	0	1	0
pr-review-tests	4	1	0	0	0	3
pr-review-errors	3	0	0	0	2	1
pr-review-consistency	0	0	1	0	0	0
Total	8	1	1	0	3	4

---

View job run

[amikofalvy]

Is it possible to re-use schemas/types already defined here as well?

[amikofalvy]

Since there were a lot of API schemas that were modified you will need to update the openapi snapshot for the tests to pass successfully

[amikofalvy]

Once we address all of the outstanding claude code review comments, you can ship this PR

[claude]

PR Review Summary

1 Key Finding | Risk: Low | Human reviewer approved with conditions ✅

Progress Since Last Review ✅

The latest commit eb7f8e20 ("claude comments") addressed most prior feedback:

• ✅ Changeset updated — Now lists 3 packages with descriptive message
• ✅ PartSchema with .strict() — FileWithBytesSchema and FileWithUriSchema now correctly enforce mutual exclusivity
• ✅ 409 Conflict for disabled triggers — Error code changed from not_found to conflict
• ✅ OpenAPI 409 documented — Added errorSchemaFactory('conflict', 'Trigger is disabled')
• ✅ Error message sanitized — Now returns "Something went wrong. Please contact support." instead of raw error details
• ✅ Stack trace in error logging — Now includes errorStack for debugging

🟠⚠️ Major (1) 🟠⚠️

🟠 1) scope: tests Missing test coverage for POST /{id}/rerun endpoint

Issue: The new rerun endpoint (lines 689-792 in triggers.ts) has zero test coverage. AGENTS.md requires: "ALL new work MUST include... Unit Tests" and "All new code paths must have test coverage."

Why: Without tests, regressions could go undetected for: (1) permission validation bypass (security), (2) disabled trigger check removal, (3) messageParts fallback logic, and (4) error handling. The existing triggers.test.ts has comprehensive tests for all other CRUD operations but none for rerun.

Fix: Add a new describe('POST /{id}/rerun') block in agents-api/src/__tests__/manage/routes/crud/triggers.test.ts covering:

• Successful rerun with userMessage only → 202
• Rerun with messageParts preserved → 202
• Non-existent trigger → 404
• Disabled trigger → 409
• Tenant isolation
• Permission validation ('use' permission required)

Refs:

• triggers.test.ts
• AGENTS.md testing requirements

🕐 Pending Recommendations (2)

From human review threads (amikofalvy):

• 🟡 page.tsx:125 — Human reviewer requested Sentry-level warn logging; current code uses console.warn which may not be captured by Sentry
• 🟡 page.tsx — Human reviewer asked if schemas/types can be reused in UI component

💭 Consider

💭 1) triggers.ts:715-721 Response envelope follows async action pattern
Issue: Returns { success, invocationId, conversationId } directly without { data: ... } wrapper.
Why: This is consistent with existing async action endpoints (e.g., datasetTriggers.ts:71 returns similar shape with 202). No change needed — just noting this follows the established convention.

---

💡 APPROVE WITH SUGGESTIONS

Summary: The core "Rerun Trigger" feature is well-implemented with clean separation between API, service, and UI layers. Human reviewer amikofalvy has already approved with the condition: "Once we address all of the outstanding claude code review comments, you can ship this PR." The prior automated feedback has been largely addressed (PartSchema, 409 status, error sanitization, changeset). The remaining items are: (1) test coverage which should be added before merge per AGENTS.md requirements, and (2) human reviewer's request for Sentry-level logging vs console.warn. Great work iterating on the feedback! 🎉

Discarded (5)

Location	Issue	Reason Discarded
TriggerService.ts	Reruns not distinguishable in OTEL traces	Architectural scope — would require changes to dispatchExecution signature and span attributes; not actionable in this PR
page.tsx:146	Non-null assertion in closure	Already addressed — code now uses result.data?.conversationId which fails safely
Response envelope	Action-style 202 without data wrapper	Consistent with existing patterns (datasetTriggers.ts, github.ts) — not an issue
Type reuse in UI	Can Part type be reused	Human reviewer already asked this question — not duplicating
Sentry logging	console.warn vs Sentry	Human reviewer already requested this — not duplicating

Reviewers (6)

Reviewer	Returned	Main Findings	Consider	Inline Comments	Pending Recs	Discarded
pr-review-standards	1	1	0	0	0	0
pr-review-consistency	5	0	1	0	0	4
pr-review-tests	1	0*	0	0	0	0
pr-review-types	0	0	0	0	0	0
pr-review-errors	0	0	0	0	0	0
pr-review-frontend	0	0	0	0	0	0
Total	7	1	1	0	0	4

*Deduplicated with pr-review-standards finding

---

View job run