PR Review Summary

2 Key Findings | Risk: Medium

This PR introduces a comprehensive Slack Work App integration with solid architecture overall. The security foundations (HMAC verification, JWT tokens, tenant isolation in the data layer) are well-designed. However, there's one critical issue with tenant handling that needs attention before merge.

🔴❗ Critical (1) ❗🔴

🔴 1) nango.ts:133 Hardcoded 'default' tenant bypasses multi-tenancy

Issue: The findWorkspaceConnectionByTeamId function always calls findWorkspaceByTeamIdDb(runDbClient)('default', teamId) with a hardcoded 'default' tenant, ignoring the actual tenant context. This pattern appears in ~30+ locations across the Slack integration.

Why: In a multi-tenant deployment, this breaks tenant isolation guarantees. The workspace lookup would search tenant 'default' regardless of which tenant the request originated from. While Slack teamIds are globally unique (mitigating cross-tenant data leakage risk), this creates:

• Incorrect tenant associations for workspaces
• Potential authorization bypass if tenant-specific policies exist
• Data consistency issues in multi-tenant deployments

Fix: The findWorkspaceConnectionByTeamId function is called in a context where we only have the Slack teamId (from Slack webhook). Two approaches:

1. Recommended: Look up workspace by teamId without tenant filter first (teamId is unique), then use the workspace's stored tenantId for authorization/subsequent operations
2. Alternative: Pass through the tenantId from request context where available, accept that initial workspace lookup by teamId may need a tenant-agnostic query

Similar hardcoded 'default' patterns exist in:

• Route schemas: tenantId: z.string().optional().default('default')
• Fallback patterns: tenantId || 'default'

Refs: nango.ts:133 · data-access tenant pattern

🟠⚠️ Major (1) 🟠⚠️

🟠 1) handleAppMention, handleCommand, streamAgentResponse Missing unit test coverage for critical runtime paths

Issue: The core Slack event handlers (handleAppMention, handleCommand) and the streaming response logic (streamAgentResponse) lack dedicated unit tests despite being the main entry points for user interactions.

Why: These handlers contain complex logic including:

• Agent resolution with channel/workspace/user fallbacks
• Streaming response parsing and Slack block formatting
• Error handling for timeout, rate limits, and API failures
• Conversation context management

Without tests, regressions in these critical paths won't be caught until production.

Fix: Add unit tests for:

1. handleAppMention - test agent resolution logic, error responses, streaming integration
2. handleCommand - test command parsing, /inkeep link flow, agent selection
3. streamAgentResponse - test SSE parsing, block assembly, timeout handling

Refs: tests reviewer finding · Existing test patterns in packages/agents-work-apps/src/slack/services/__tests__/

💭 Consider (2) 💭

💭 1) jwt-helpers.ts (not in diff) JwtVerifyResult type allows impossible states

Issue + Why: The existing JwtVerifyResult<T> interface uses valid: boolean with optional payload? and error?, allowing invalid states like { valid: true, error: 'something' }. A discriminated union would provide compile-time safety.

Suggestion: This affects the Slack token implementations. Consider a follow-up PR to refactor to:

export type JwtVerifyResult<T> =
  | { valid: true; payload: T; error?: undefined }
  | { valid: false; payload?: undefined; error: string };

💭 2) packages/agents-work-apps/ New package exports not documented

Issue + Why: The new package adds significant public exports (SlackWorkApp, types, services) but the barrel exports and public API surface aren't documented for internal consumers.

Suggestion: Add JSDoc comments to main exports in src/index.ts and consider adding a brief API section to the README.

📌 Inline Comments (1)

• 🔴 nango.ts:133 Hardcoded 'default' tenant breaks multi-tenancy

Note: An inline comment was also added to jwt-helpers.ts:24 but this file is not in the PR diff - that finding is captured in Consider above instead.

🕐 Pending Recommendations (2)

From review threads:

• 💭 agents-api/src/createApp.ts:405 Early return question (resolved - omar-inkeep confirmed ok)
• 💭 agents-api/src/createApp.ts:437 Consider consolidating middleware for /users and /workspaces routes

Summary: This is a well-architected feature addition with solid security foundations. The HMAC signature verification, JWT token design with proper claims/audiences, and database schema with tenant foreign keys are all implemented correctly. The critical tenant hardcoding issue should be addressed before production deployment in a true multi-tenant environment - though for single-tenant deployments or if multi-tenancy isn't an immediate requirement, this could be tracked as follow-up work. The test coverage gaps are notable but not blocking given the existing integration tests.

PR Review Summary

7 Key Findings | Risk: Medium

This is a re-review following 24 commits that addressed most prior feedback. The Slack Work App integration is well-architected overall — the security foundations (HMAC verification, JWT tokens, tenant isolation in the data layer), streaming implementation, and database schema are solid. The remaining findings are primarily around test coverage gaps, consistency refinements, and minor security hardening.

🟠⚠️ Major (4) ⚠️🟠

🟠 1) workspaces.ts:1086-1189 test-message endpoint lacks admin authorization

Issue: The POST /:teamId/test-message endpoint allows any authenticated tenant member to post messages via the bot without requiring org admin/owner privileges — unlike other workspace settings endpoints that use requireWorkspaceAdmin.

Why: A non-admin tenant member could use this endpoint to post messages impersonating the bot. This is inconsistent with the permission model applied to other workspace management operations.

Fix: Apply requireWorkspaceAdmin middleware to this endpoint.

Refs:

• workspaces.ts:1086-1189
• requireWorkspaceAdmin usage

🟠 2) modal-submission.ts Modal submission handlers have zero test coverage

Issue: The handleModalSubmission and handleFollowUpSubmission handlers (461 lines) have no test coverage despite being primary user interaction paths for /inkeep private queries.

Why: These handlers contain complex logic: user link validation, thread context handling, agent API calls with 30s timeout, and error recovery paths. Without tests, regressions in modal flows won't be caught until production.

Fix: Add modal-submission.test.ts covering: (1) happy path with mocked workspace/user/agent API, (2) user not linked → link prompt, (3) API timeout → error message, (4) follow-up with existing conversationId.

Refs:

• modal-submission.ts
• Existing test patterns

🟠 3) block-actions.ts Block action handlers have zero test coverage

Issue: The handleOpenAgentSelectorModal, handleOpenFollowUpModal, and handleMessageShortcut handlers (291 lines) lack tests. A prior review flagged that handleOpenAgentSelectorModal silently fails without user notification — this fix cannot be verified without tests.

Why: These handlers manage interactive button clicks (Select Agent, Follow Up) and message shortcuts. Without tests, button interactions could silently break.

Fix: Add block-actions.test.ts covering: (1) modal open success, (2) no bot token → early return, (3) no projects → error ephemeral, (4) error during modal open → sendResponseUrlMessage with error.

Refs:

• block-actions.ts

🟠 4) package.json Missing changesets for new Slack integration exports

Issue: This PR adds significant new public exports to @inkeep/agents-work-apps (the ./slack export) and @inkeep/agents-core (Slack JWT utilities, SSE parser), but no changesets cover these additions. The only changeset (fiery-owls-clap.md) is for @inkeep/agents-manage-ui.

Why: Without changesets, the new exports won't be documented in release notes, and semver versioning won't reflect the additions.

Fix:

pnpm bump minor --pkg agents-work-apps "Add Slack Work App integration with OAuth, slash commands, @mention support, and modal interactions"
pnpm bump minor --pkg agents-core "Add Slack JWT utilities for user tokens and link tokens, plus shared SSE parser"

Refs:

• agents-work-apps exports
• AGENTS.md changeset guidance

Inline comments:

• 🟠 Major: workspaces.ts:1092 test-message lacks admin authorization

🟡 Minor (7) 🟡

🟡 1) nango.ts Split source of truth creates inconsistency risk

Issue: Workspace default agent is stored in Nango connection metadata, while channel-level configs are stored in PostgreSQL. This split creates consistency risks during failures (no transactional guarantees across systems).

Why: If Nango metadata update succeeds but a related PostgreSQL operation fails, the system ends up inconsistent. This contradicts the architecture note at the top of nango.ts stating "PostgreSQL is the authoritative source of truth."

Fix: Consider storing workspace default agent config in PostgreSQL (add columns to work_app_slack_workspaces: default_project_id, default_agent_id). Use Nango exclusively for OAuth token storage.

Refs:

• nango.ts:280-316

🟡 2) WaysToTalkToAgent.mdx Slack not listed in overview snippet

Issue: The snippet listing ways to talk to agents (React UI, API, A2A, MCP, Triggers) doesn't mention Slack. Users discovering features via the overview page will miss this major new capability.

Why: The Slack Work App is a first-class way to talk to agents and belongs alongside other methods in the overview.

Fix: Add Slack entry to WaysToTalkToAgent.mdx in both the bullet list and Cards section.

Refs:

• WaysToTalkToAgent.mdx

🟡 3) strings.ts:11,20-21 "Trigger Agent" terminology mismatch

Issue: Modal titles use "Trigger Agent" terminology while docs use "Ask", "Send", "Prompt" language. "Trigger" suggests webhooks/automation, not user-facing chat.

Why: Creates vocabulary mismatch between the modal UX and documentation. Users opening the modal via /inkeep expect to "Ask" an agent, not "Trigger" one.

Fix: Consider renaming to "Ask Agent" or "Send to Agent" for consistency with docs.

Refs:

• strings.ts
• commands.mdx terminology

🟡 4) entities.ts Missing Slack entity type exports

Issue: Zod validation schemas for Slack tables exist in schemas.ts, but corresponding TypeScript entity types are NOT exported from entities.ts per the data-model-changes checklist.

Why: Downstream code won't have type inference for Slack entities via standard imports, requiring manual inference from Zod schemas.

Fix: Add entity type exports following the established pattern in packages/agents-core/src/types/entities.ts.

Refs:

• data-model-changes skill

🟡 5) workspaces.ts OperationId naming diverges from pattern

Issue: Slack routes use slack-{verb}-{noun} format while manage API uses {verb}-{noun}. This introduces a parallel naming scheme.

Why: Inconsistent OpenAPI spec may confuse SDK generators and API consumers.

Fix: This may be intentional namespacing. If so, document the convention for future Work Apps integrations. Otherwise, align with manage API pattern.

Refs:

• workspaces.ts operationIds
• projects.ts operationIds

🟡 6) workspaces.ts Several endpoints return 200 with success:false on errors

Issue: Multiple endpoints (PUT/DELETE channel settings, bulk operations) return HTTP 200 with { success: false } when workspace lookup or tenant verification fails.

Why: HTTP 200 masks the failure. Error handling that relies on status codes would miss these failures. The UI may not properly distinguish between success and failure.

Fix: Return appropriate HTTP status codes (404 for not found, 403 for forbidden) instead of 200 with success:false.

Refs:

• workspaces.ts:656-660

🟡 7) app-mention.test.ts Thread-specific flows not tested

Issue: The app-mention tests cover 5 paths but miss thread handling (~40% of handler logic) including bot thread detection, non-bot thread auto-execution with context, and the prompt injection mitigation XML delimiters.

Why: Thread context flows are security-sensitive. The <slack_thread_context> delimiters could be accidentally removed in refactoring without test protection.

Fix: Add tests for thread scenarios: bot thread detection, thread context formatting, XML delimiter presence.

Refs:

• app-mention.ts:125-214

Inline comments:

• 🟡 Minor: events.ts:218-220 Silent early return when workspace lookup fails
• 🟡 Minor: nango.ts:27-31 Workspace connection cache unbounded
• 🟡 Minor: 0012_add-slack-work-app-tables.sql:52-60 Migration contains DROP CONSTRAINT artifacts
• 🟡 Minor: package.json:71 Repository directory typo

💭 Consider (3) 💭

💭 1) slack-api.ts:1 UI API client diverges from established pattern

Issue: The file uses raw fetch() calls instead of makeManagementApiRequest() from api-config.ts, missing cookie forwarding, error normalization, and bypass secret injection.

Fix: Refactor to use makeManagementApiRequest() or create a makeWorkAppsApiRequest() helper.

Refs: Prior review discussion

💭 2) modals.ts:102,123 Project concept exposed in modal UX

Issue: The agent picker modal requires selecting a "Project" then an "Agent", but docs focus only on agents. Users may not understand the Project concept.

Fix: Consider auto-selecting the project when only one exists, or adding brief explanation in docs.

💭 3) commands/index.ts:83 Nested fetch calls share single AbortController timeout

Issue: A single 10-second AbortController is used for both projects fetch and all subsequent agent fetches. If projects takes 8 seconds, agents only get 2 seconds.

Fix: Consider separate AbortControllers per operation group, or increase overall timeout.

🕐 Pending Recommendations (1)

• 💭 slack-api.ts:1 UI API client pattern divergence (from prior review)

Summary: This is a well-architected Slack integration that addresses most prior review feedback. The core security patterns (HMAC, JWT, tenant isolation), streaming implementation, and database schema are solid. The main gaps are test coverage for modal/block-action handlers and some consistency refinements. The changesets should be added before merge to ensure proper release notes. None of the findings are blocking for functionality, but the test coverage gaps represent meaningful risk for a user-facing integration of this scope.

PR Review Summary

5 Key Findings | Risk: Medium

🔴❗ Critical (0) ❗🔴

No critical issues identified. Prior reviews have addressed most security concerns (OAuth state signing, tenant isolation, auth bypass).

🟠⚠️ Major (2) 🟠⚠️

🟠 1) modal-submission.ts · block-actions.ts Modal/block action handlers have zero test coverage

Issue: Two core user interaction paths have no test coverage:

• modal-submission.ts (461 lines) — handles the private query flow via /inkeep slash commands
• block-actions.ts (291 lines) — handles button clicks for agent selection and follow-up queries

Why: These are primary UX paths where regressions would silently break the Slack experience. Specific risks include:

• 30-second AbortController timeout logic could fail to abort properly
• JSON parsing of modal metadata could throw uncaught exceptions
• Error classification could return incorrect error types
• Agent selector fallback to workspace default could break

Fix: Add dedicated test files covering:

1. Happy path: valid metadata + linked user + successful API response → ephemeral message posted
2. Missing user link: → returns 'link your account' ephemeral
3. API timeout: mock fetch to abort after 30s → returns timeout error message
4. Follow-up submission: verify conversationId is passed correctly

Refs:

• modal-submission.ts
• block-actions.ts

🟠 2) packages/agents-work-apps/package.json Missing changeset for new Slack package exports

Issue: This PR adds a new ./slack export to @inkeep/agents-work-apps which is a published package (publishConfig.access: public). No changeset documents this addition.

Why: Without a changeset:

• The new exports won't appear in release notes
• The npm version won't reflect the feature addition (should be minor bump)
• Consumers upgrading won't know about the new Slack integration APIs

Fix:

pnpm bump minor --pkg agents-work-apps "Add Slack Work App integration with OAuth, slash commands, and @mention support"

If @inkeep/agents-core also gained new exports (Slack JWT utilities), include that package as well.

Refs:

• AGENTS.md: Creating Changelog Entries

Inline comments:

• 🟠 Major: block-actions.ts:156-168 handleOpenFollowUpModal catches errors without user notification

🟡 Minor (3) 🟡

🟡 1) slack-api.ts UI API client diverges from established pattern

Issue: The Slack API client uses raw fetch() calls directly instead of following the established pattern. All other API clients in /lib/api/ use makeManagementApiRequest() from api-config.ts which handles cookie forwarding, error normalization, and bypass secret injection. The file also lacks the 'use server' directive present in peer API files.

Why: Inconsistency with established patterns makes the codebase harder to maintain and could lead to subtle auth/error handling bugs.

Fix: Refactor to use makeManagementApiRequest() with appropriate path handling for work-apps routes, add 'use server' directive, and follow the function export pattern from github.ts.

Refs:

• Existing pattern: github.ts

🟡 2) workspaces.ts:4-13 Header comment lists 9 endpoints but file has 13

Issue: The header comment's route listing is incomplete. Missing:

• PUT /:teamId/channels/bulk (Bulk Set Channel Agents)
• DELETE /:teamId/channels/bulk (Bulk Remove Channel Configs)
• GET /:teamId/health (Check Workspace Health)
• POST /:teamId/test-message (Send Test Message)

Why: Developers consulting the header comment would miss 4 significant endpoints, making the file harder to navigate.

Fix: Update the header comment to include all 13 routes.

Refs:

• workspaces.ts

🟡 3) env.ts / .env.example Env var sync drift

Issue: SLACK_BOT_TOKEN is defined in env.ts but missing from .env.example. Additionally, NANGO_SLACK_INTEGRATION_ID appears required for OAuth but is marked optional in the schema.

Why: New developers setting up the Slack integration may not realize which env vars are truly required for the OAuth flow to work.

Fix: Add SLACK_BOT_TOKEN to .env.example and consider adding .describe() to clarify when each Slack env var is required.

Refs:

• adding-env-variables skill

Inline comments:

• 🟡 Minor: client.ts:21 Slack WebClient calls lack explicit timeout configuration
• 🟡 Minor: slack-link-token.ts:56-61 JSDoc references stale "device authorization flow" terminology
• 🟡 Minor: blocks/index.ts:91-99 Agent list truncation at 15 items without clear path to see more
• 🟡 Minor: commands/index.ts:731-742 Command aliases may fragment user mental models
• 🟡 Minor: installation.mdx:14-15 Prerequisites lack env var documentation link

💭 Consider (4) 💭

💭 1) workspaces.ts operationId naming inconsistency
Issue: Slack routes use slack-<action> format while manage API GitHub routes use <verb>-github-<noun> format.
Fix: Document as intended pattern evolution or align for consistency.

💭 2) types.ts WorkAppsVariables interface allows illegal states
Issue: All optional fields allow invalid combinations (e.g., tenantRole without tenantId).
Fix: Consider discriminated union to represent valid authentication states.

💭 3) agent-configuration-card.tsx:149 Waterfall fetch pattern
Issue: Three sequential network requests on mount (fetchAgents, fetchChannels, fetchWorkspaceSettings).
Fix: Use Promise.all for coordinated loading like workspace-hero.tsx does.

💭 4) streaming.ts:233 Stream reader not explicitly released on error
Issue: When an error occurs in the stream processing loop, the reader from response.body.getReader() is not explicitly released.
Fix: Add await reader.cancel() in the catch block.

🕐 Pending Recommendations (4)

• 🟠 workspaces.ts:1095 POST /:teamId/test-message lacks admin authorization
• 🟡 nango.ts:27-31 Workspace connection cache unbounded unlike user mapping cache
• 🟡 events.ts:218-220 Silent early return when workspace lookup fails
• 🟡 0012_add-slack-work-app-tables.sql:52-60 Migration contains Drizzle-generated DROP CONSTRAINT artifacts

Summary: This is a well-structured, comprehensive Slack Work App integration that follows established patterns and includes good security foundations. The main gaps are test coverage for modal/block action handlers and a missing changeset for the new package exports. Prior reviews have successfully addressed most security concerns. Addressing the Major findings (test coverage, changeset) and the Pending Recommendations would strengthen this integration before release. 🎉

PR Review Summary

0 Key Findings | Risk: None

This delta review covers 1 commit (216e76c4) since the last automated review, with 1 file changed (agents-api/package.json, -5 lines).

Delta Assessment

✅ Clean dependency cleanup — The commit removes 5 unused Slack-related dependencies from agents-api/package.json:

• @nangohq/node (production)
• @slack/bolt (production)
• @slack/web-api (production)
• slack-block-builder (production)
• @slack/types (devDependencies)

Verified correct: These dependencies are properly declared in packages/agents-work-apps/package.json where the Slack integration code resides. Since agents-api depends on @inkeep/agents-work-apps: workspace:^, it receives transitive access to these packages through the workspace dependency.

🔴❗ Critical (0) ❗🔴

None.

🟠⚠️ Major (0) 🟠⚠️

None.

🟡 Minor (0) 🟡

None.

🕐 Pending Recommendations (18+)

These ACTIVE findings from prior reviews remain unaddressed on unchanged code paths. They are outside this delta's scope but should be addressed before merge:

Major (6):

• 🟠 app-mention.ts:179-196 Unsanitized thread context enables indirect prompt injection
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:248 GET /:teamId/settings lacks tenant ownership verification
• 🟠 events.ts:376 Nango webhook endpoint lacks signature verification
• 🟠 users.ts:143 User link verification accepts arbitrary userId from request body
• 🟠 workspaces.ts:1095 POST /:teamId/test-message lacks admin authorization

Minor (12+):

• 🟡 installation.mdx:14-15 Prerequisites lack env var documentation link
• 🟡 commands/index.ts:731-742 Command aliases may fragment user mental models
• 🟡 blocks/index.ts:91-99 Agent list truncation at 15 items without clear path to see more
• 🟡 block-actions.ts:156-168 handleOpenFollowUpModal catches errors without user notification
• 🟡 slack-link-token.ts:56-61 JSDoc references stale "device authorization flow" terminology
• 🟡 client.ts:21 Slack WebClient calls lack explicit timeout configuration
• 🟡 migration:52-60 Migration contains Drizzle-generated DROP CONSTRAINT artifacts
• 🟡 nango.ts:27-31 Workspace connection cache unbounded unlike user mapping cache
• 🟡 events.ts:218-220 Silent early return when workspace lookup fails
• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 slack-dashboard.tsx:94 Documentation link points to non-existent page
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing accessible labels
• 🟡 permissions.ts:65-70 Test environment bypass weakens permission testing
• 🟡 package.json:1 Inconsistent @nangohq/node versions across packages
• 🟡 workAppSlack.ts:328-385 Check-then-act pattern vulnerable to TOCTOU race

Summary: The delta is a clean dependency cleanup with no functional changes. The removed dependencies are verified to exist in the correct package (agents-work-apps). However, 18+ ACTIVE prior findings remain unaddressed on unchanged code paths — including 6 Major issues around tenant isolation, authorization, and prompt injection. These should be reviewed and addressed before merge. The dependency cleanup itself is ✅ good to go.

PR Review Summary

0 Key Findings | Risk: Low

Re-Review Delta Assessment

This re-review covers 1 commit since the last automated review:

• 525dded9 — fix: remove unused barrel exports and files flagged by knip

Delta scope: 4 files (2 deleted, 2 modified) — pure cleanup removing unused exports. No functional changes.

Verification complete:

• ✅ Deleted barrel files were not imported anywhere externally
• ✅ Made-private interfaces (SlackWorkspaceInstallation, DefaultAgentConfig) are only used internally
• ✅ Removed component exports were only used internally within slack-dashboard.tsx
• ✅ Change is consistent with established patterns in the codebase

No new issues identified in the delta.

🕐 Pending Recommendations (11)

The following issues were raised in prior reviews and remain ACTIVE (unaddressed):

Security & Authorization (2 Major)

• 🟠 workspaces.ts:248 GET /:teamId/settings lacks tenant ownership verification
• 🟠 block-actions.ts:156-168 handleOpenFollowUpModal catches errors without user notification

Security Considerations (1 Major)

• 🟠 app-mention.ts:179-196 Unsanitized thread context enables indirect prompt injection

Authorization Flow (1 Major)

• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:1095 POST /:teamId/test-message lacks admin authorization

Minor Items (6)

• 🟡 installation.mdx:14-15 Prerequisites lack env var documentation link
• 🟡 commands/index.ts:731-742 Command aliases may fragment user mental models
• 🟡 blocks/index.ts:91-99 Agent list truncation at 15 items without clear path to see more
• 🟡 slack-link-token.ts:56-61 JSDoc references stale "device authorization flow" terminology
• 🟡 client.ts:21 Slack WebClient calls lack explicit timeout configuration
• 🟡 nango.ts:27-31 Workspace connection cache unbounded unlike user mapping cache
• 🟡 events.ts:218-220 Silent early return when workspace lookup fails
• 🟡 migration 0012:52-60 Migration contains Drizzle-generated DROP CONSTRAINT artifacts
• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 slack-dashboard.tsx:94 Documentation link points to non-existent page

Summary: The delta cleanup is correct and introduces no new issues. The knip-flagged unused exports have been properly removed without breaking any imports. However, 11 prior review findings remain unaddressed — including several Major security/authorization concerns (tenant isolation gaps, permission middleware ordering, prompt injection risk). These should be addressed before merging to ensure the Slack Work App integration maintains proper security boundaries.