feat(api): add webhook CRUD API router and tests

[nickmeinhold]

Summary

• Add tRPC webhook router with list, create, update, delete, and test endpoints
• All endpoints require authentication and admin workspace role
• URL validation (max 2048 chars), event subscription filtering against webhookEvents enum
• test endpoint sends a synthetic card.created payload and returns success/error status
• Add unit tests for all router procedures
• Add integration tests with PGlite test database
• Add vitest config and test infrastructure for the API package

Dependencies

Depends on #391 (DB schema & repository). Independent of #392 (delivery utility).

PR series

• PR 1: DB schema & repository (feat(db): add webhook schema, migration, and repository #391)
• PR 2: Webhook delivery utility + card event integration (feat(api): add webhook delivery utility and card event integration #392)
• PR 3 (this): Webhook CRUD API router + tests
• PR 4: Webhook management UI

Test plan

• Unit tests pass for all router procedures
• Integration tests pass with PGlite
• CRUD operations enforce admin role requirement
• Test endpoint delivers a synthetic payload and reports result
• Invalid URLs and unknown event types are rejected

🤖 Generated with Claude Code

[MaxwellMergeSlam]

MaxwellMergeSlam's Review

Verdict: COMMENT

Summary: Comprehensive CRUD router with solid test coverage, but uses the older assertUserInWorkspace instead of the preferred assertPermission helpers, and the test endpoint has an SSRF surface.

Dutch (Predator): "If it bleeds, we can kill it." — And if it has an SSRF surface, we can exploit it. But let's talk about the good stuff first.

Findings:

• packages/api/src/routers/webhook.ts: All five endpoints use assertUserInWorkspace(ctx.db, userId, workspace.id, "admin") for authorization. Per the project's CLAUDE.md: "the permissions utils are preferred for new code" — this should use assertPermission from packages/api/src/utils/permissions.ts instead. The old helper still works, but it's inconsistent with the direction of the codebase.

• packages/api/src/routers/webhook.ts:138-164: The test endpoint dynamically imports ../utils/webhook to avoid circular dependencies. This works, but it means PR 3 has a runtime dependency on PR 2's code. The PR description says they're independent, but they're not — the test endpoint will crash if PR 3 is merged without PR 2.

• packages/api/src/routers/webhook.ts:33: Input validation uses .min(12) for workspacePublicId, but publicIds in this codebase are exactly 12 characters. Should this be .length(12) instead? Same for webhookPublicId.

• packages/api/integration-tests/test-db.ts:13-14: The TestDbClient type is cast through unknown — db as unknown as TestDbClient. This is a pragmatic workaround for PGlite vs node-postgres type mismatch, but it means zero type safety in integration tests.

• packages/api/integration-tests/test-db.ts:33: The migration folder path is ../../packages/db/migrations — this is a relative path that depends on the test being run from packages/api. If the test runner's CWD changes, migrations won't be found. Consider using path.resolve(__dirname, ...) for robustness.

• packages/api/vitest.config.ts:11: The alias "@kan/db": resolve(__dirname, "../db/src") maps to a sibling package's source — this bypasses the package's own build output. Fine for testing but worth noting.

The Good:

• OpenAPI metadata on all endpoints — enables automatic API documentation
• Proper ownership validation (checking webhook.workspaceId !== workspace.id) prevents cross-workspace access
• Both unit tests (mocked) and integration tests (PGlite) — defense in depth
• The integration test infrastructure (createTestDb, seedTestData) is reusable for future test suites
• Input validation is thorough: URL format, length limits, enum-constrained events

The Concerns:

• assertUserInWorkspace vs assertPermission — tech debt that contradicts project conventions
• The test endpoint creates a hidden dependency between PR 3 and PR 2
• Boilerplate repetition — the workspace lookup + auth check is copy-pasted across all 5 endpoints. A middleware or helper could DRY this up, though that's a style preference

Tyler Durden: "The first rule of Fight Club is: you do not talk about Fight Club." — The first rule of this codebase is: you use assertPermission, not assertUserInWorkspace. Read the damn CLAUDE.md.

[KelvinBitBrawler]

KelvinBitBrawler's Review

Verdict: REQUEST_CHANGES

Summary: You've built a structure, but it leaks entropy like a cracked hull on Titan.

Findings:

• packages/api/integration-tests/webhook.integration.test.ts:3: import * as webhookRepo from "../../db/src/repository/webhook.repo";

  • Replicant: "I've seen things you people wouldn't believe." Like relative imports reaching across package boundaries when you explicitly configured an alias.
  • Correction: You set up @kan/db alias in vitest.config.ts. Use it. Don't traverse the file system like a rat in a ventilation shaft.

• packages/api/src/routers/webhook.ts:79 (and others): url: z.string().url().max(2048)

  • Thermodynamics check: Accessing internal networks via user-supplied URLs causes the system temperature to rise to critical levels.
  • Critique: z.string().url() allows http://localhost, http://169.254.169.254, etc. Without SSRF protection (blocking internal IP ranges), this is a security hole wide enough to fly a drop-ship through. At minimum, block localhost/private IPs or acknowledge the risk.

• packages/api/src/routers/webhook.ts:335: const { sendWebhookToUrl... } = await import("../utils/webhook");

  • Observation: Dynamic imports inside a handler? Trying to avoid a circular dependency loop? It's uglier than a Xenomorph, but efficient. I'll allow it if it keeps the startup time close to absolute zero.

The Good:

• You actually wrote integration tests with PGlite. Most developers are too lazy to spin up a proper test environment, preferring to mock reality until it breaks.
• The authorization checks assertUserInWorkspace(..., "admin") are present on every endpoint. Good. Trust no one.

The Concerns:

• Security: Sending the secret in plain text to create/update relies entirely on TLS. If that layer fails, your security model enters heat death.
• Error Handling: if (!result) throw ... INTERNAL_SERVER_ERROR. Vague. If the database rejects the insert (e.g. constraint violation), returning a generic 500 is as helpful as a screen door on a submarine. Catch specific errors.

MacReady: "I know I'm human. And if you were all these things, then you'd just attack me right now."
Fix the imports and address the SSRF risk before I break out the flamethrower.

[nickmeinhold]

Reviewed all bot feedback on this PR. The flagged integration test import (@kan/db alias) is correct — no changes needed.

[nickmeinhold]

Pushed review feedback changes to this branch (30e616e):

• Use extracted webhookUrlSchema zod validator in create/update input schemas for consistent SSRF checks