Skip to content

Conversation

@amikofalvy
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2026

🦋 Changeset detected

Latest commit: fd6c74e

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-manage-api Major
@inkeep/agents-run-api Major
@inkeep/agents-core Major
@inkeep/agents-eval-api Major
@inkeep/agents-manage-ui Major
@inkeep/agents-cli Major
@inkeep/agents-sdk Major
@inkeep/ai-sdk-provider Major
@inkeep/create-agents Major
@inkeep/agents-manage-mcp Major

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

@vercel
Copy link

vercel bot commented Jan 17, 2026

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

Project Deployment Review Updated (UTC)
agents-docs Ready Ready Preview, Comment Jan 20, 2026 7:17pm
agents-eval-api Ready Ready Preview, Comment Jan 20, 2026 7:17pm
agents-manage-api Ready Ready Preview, Comment Jan 20, 2026 7:17pm
agents-manage-ui Ready Ready Preview, Comment Jan 20, 2026 7:17pm
agents-run-api Ready Ready Preview, Comment Jan 20, 2026 7:17pm

Request Review

@claude
Copy link

claude bot commented Jan 17, 2026

Claude finished @amikofalvy's task —— View job


Staff Engineer Review: Triggers Feature Implementation

I've completed a comprehensive review of this substantial triggers feature implementation. This adds webhook-based agent invocation capabilities with authentication, payload transformation, and audit logging. Below are my key findings and recommendations.

🎯 Feature Overview

The implementation successfully delivers a webhook trigger system allowing external services to invoke agents. The feature includes input validation, payload transformation, multiple authentication methods, and comprehensive audit logging. The PRD indicates ~70% completion with core infrastructure complete but missing agent invocation integration.

⚠️ Critical Architecture Issues

1. Security: Credentials in Database Schema

High Priority

// packages/agents-core/src/validation/schemas.ts:1393-1410
authentication: z.object({
  type: z.literal('api_key'),
  data: z.object({
    value: z.string().describe('Expected API key value'), // ⚠️ Stored in plaintext
    // Also basic_auth password, bearer_token stored in plaintext
  })
})

Issue: Authentication credentials (API keys, passwords, bearer tokens) are stored in plaintext in the triggers.authentication JSONB field.

Impact: Major security vulnerability. Database compromise exposes all webhook credentials.

Recommendation:

  • Integrate with existing credentialReferences system for encrypted credential storage
  • Store credential reference IDs in triggers table instead of raw credentials
  • Add migration to encrypt existing credentials

2. Incomplete Feature: Missing Agent Invocation

High Priority

// agents-run-api/src/routes/webhooks.ts:220-226
// TODO: US-013 - Invoke agent via /api/chat endpoint
const invocationId = `inv_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`;
return c.json({ success: true, invocationId }, 202);

Issue: Webhook endpoint returns 202 Accepted but never actually invokes the agent. This breaks the core user contract.

Impact: Feature appears to work but provides no business value. Users will expect agents to respond to webhooks.

Recommendation:

  • Complete US-013 implementation before merge
  • Add proper invocation logging to trigger_invocations table
  • Implement message template interpolation
  • Add error handling for failed agent invocations

3. Data Access Layer: Missing Error Handling

Medium Priority

// packages/agents-core/src/data-access/manage/triggers.ts:82-83
const result = await db.insert(triggers).values(params).returning();
return result[0] as TriggerSelect; // ⚠️ No validation result exists

Issue: Database operations assume success without validating results exist. Similar pattern in all CRUD operations.

Recommendation:

const result = await db.insert(triggers).values(params).returning();
if (!result[0]) {
  throw createApiError({ code: 'internal_server_error', message: 'Failed to create trigger' });
}
return result[0];

🔧 Code Quality Considerations

4. Type Safety: Overly Permissive Authentication Schema

// packages/agents-core/src/db/manage/manage-schema.ts:137
authentication: jsonb('authentication').$type<unknown>(), // ⚠️ Should be typed

Issue: Authentication field uses unknown type instead of the well-defined TriggerAuthenticationSchema.

Fix: .$type<z.infer<typeof TriggerAuthenticationSchema>>()

5. Template Interpolation: No Input Sanitization

// packages/agents-core/src/utils/template-interpolation.ts:72-73
return template.replace(/\{\{([^}]+)\}\}/g, (match, path: string) => {
  const value = getValue(payload, trimmedPath); // ⚠️ No XSS protection

Issue: Template interpolation doesn't sanitize output, potential XSS if templates contain user data.

Recommendation: Add optional HTML escaping parameter and sanitize interpolated values.

6. Webhook Auth: Timing Attack Vulnerability

// packages/agents-core/src/utils/trigger-auth.ts:89-95
if (credentials !== expectedCredentials) {
  return { success: false, status: 403, message: 'Invalid username or password' }; // ⚠️ String comparison
}

Issue: Basic auth uses direct string comparison instead of timing-safe comparison.

Fix: Use timingSafeEqual for all credential comparisons, not just HMAC signatures.

🏗️ Database Design Issues

7. Foreign Key Inconsistency

The trigger_invocations table references both triggers and agents directly:

CONSTRAINT "trigger_invocations_trigger_fk" FOREIGN KEY ("tenant_id","project_id","agent_id","trigger_id") REFERENCES "public"."triggers"

Issue: Redundant foreign key to agents table since triggers already cascade from agents.

Recommendation: Remove direct agent FK from invocations table - triggers FK is sufficient.

8. Index Optimization

Missing composite index for webhook URL generation:

-- Add index for webhook endpoint lookups
CREATE INDEX "triggers_tenant_project_agent_trigger_idx" ON "triggers" ("tenant_id", "project_id", "agent_id", "id");

🎨 API Design Concerns

9. Webhook URL Generation Missing

The PRD mentions webhookUrlTemplate field but it's not implemented in the API responses.

Impact: Users can't easily discover webhook URLs for their triggers.

Recommendation: Add computed webhook URL field to API responses:

webhookUrl: `${baseUrl}/tenants/${tenantId}/projects/${projectId}/agents/${agentId}/triggers/${triggerId}`

10. Error Response Inconsistency

Webhook endpoint mixes custom error format with standard API error format:

// Inconsistent - should use handleApiError pattern
return c.json({ error: authResult.error || 'Unauthorized' }, 401);

📊 Testing & Observability Gaps

11. Missing Integration Tests

Per the PRD, user stories US-014, US-016, US-018 for integration tests are incomplete.

Risk: Complex authentication flows and error cases are untested.

Recommendation: Prioritize integration tests before production deployment.

12. Limited Observability

Webhook processing lacks comprehensive logging for debugging failed invocations.

Enhancement: Add structured logging for payload transformation, authentication steps, and invocation attempts.

🚀 Positive Implementation Notes

Excellent HMAC signature verification with timing-safe comparison
Well-structured authentication system supporting multiple auth types
Comprehensive input validation with JSON schema support
Clean data access layer following established patterns
Proper database migrations with indexes and foreign key constraints
Good TypeScript usage with discriminated unions for auth types

📋 Recommendations Summary

Before Merge (Critical):

  1. Fix credential storage security vulnerability
  2. Complete agent invocation implementation (US-013)
  3. Add error handling to data access layer

Post-Merge (High Priority):
4. Fix timing attack vulnerability in basic auth
5. Complete integration tests
6. Add webhook URL generation to API responses

Enhancement (Medium Priority):
7. Improve type safety for authentication schema
8. Add comprehensive observability
9. Optimize database indexes

This is a well-architected feature with solid foundations, but the security and completeness issues need immediate attention before production deployment.

amikofalvy and others added 9 commits January 19, 2026 18:36
- Added triggers table with fields: id, tenantId, projectId, agentId, name, description, enabled, inputSchema, outputTransform, messageTemplate, authentication, signingSecret, timestamps
- Added trigger_invocations table with fields: id, tenantId, projectId, agentId, triggerId, conversationId, status, requestPayload, transformedPayload, errorMessage, createdAt
- Added foreign key constraints with cascade delete
- Updated prd.json with real user stories from ralph/prd.json

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Marked US-001 and US-002 as complete in prd.json
- Added iteration 4 details to progress.txt with implementation summary

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Added TriggerAuthenticationSchema with discriminated union for api_key, basic_auth, bearer_token, none
- Added TriggerOutputTransformSchema with jmespath and objectTransformation fields
- Added TriggerInvocationStatusEnum with pending, success, failed values
- Added TriggerSelectSchema, TriggerInsertSchema, TriggerUpdateSchema
- Added TriggerApiSelectSchema, TriggerApiInsertSchema, TriggerApiUpdateSchema
- Added TriggerInvocationSelectSchema, TriggerInvocationInsertSchema, TriggerInvocationUpdateSchema
- Added TriggerInvocationApiSelectSchema, TriggerInvocationApiInsertSchema, TriggerInvocationApiUpdateSchema
- All schemas follow existing pattern with agent-scoped API schemas
- Typecheck passes

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Created triggers.ts with CRUD operations: getTriggerById, listTriggers, listTriggersPaginated, createTrigger, updateTrigger, deleteTrigger
- Created triggerInvocations.ts with operations: getTriggerInvocationById, listTriggerInvocationsPaginated (with status and date filtering), createTriggerInvocation, updateTriggerInvocationStatus
- All functions follow agent-scoped pattern with curried database client
- Exported from data-access/index.ts
- Added TriggerSelect, TriggerInsert, TriggerUpdate types to entities.ts
- Added TriggerInvocationSelect, TriggerInvocationInsert, TriggerInvocationUpdate types to entities.ts
- Typecheck passes

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- 6/26 user stories completed (23%)
- Core database and data access foundation complete
- Ready for webhook endpoint implementation

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Implements authentication verification, signing secret verification,
and message template interpolation for trigger webhooks.

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
@github-actions
Copy link
Contributor

🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants