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

6 Key Findings | Risk: Medium

🟠⚠️ Major (3) 🟠⚠️

🟠 1) packages/agents-work-apps/src/slack/routes/users.ts User endpoints accept arbitrary userId without caller verification

Issue: Multiple /work-apps/slack/users/* endpoints accept userId from query/body parameters without verifying the authenticated caller is that user or has admin privileges. This enables any authenticated user to:

• /users/status (GET): Query the Slack connection status (email, tenantId, slackUserId, slackTeamId) of any other user
• /users/connect (POST): Initiate a Nango OAuth session on behalf of any arbitrary userId
• /users/disconnect (POST): Unlink any other user's Slack account

Why: An authenticated attacker within the same deployment can enumerate and manipulate other users' Slack connections. The status endpoint leaks PII (email, Slack display name) and tenant correlation data.

Fix: For each endpoint, validate that the requested userId matches the authenticated user or the caller has admin privileges:

const sessionUserId = c.get('userId') as string | undefined;
if (sessionUserId !== userId && !sessionUserId?.startsWith('apikey:') && sessionUserId !== 'system') {
  return c.json({ error: 'Can only manage your own Slack connection' }, 403);
}

Alternatively, remove the userId parameter entirely and derive it from c.get('userId') for these user-self-service endpoints.

Refs:

• users.ts:385-458 — /users/status endpoint
• users.ts:265-287 — /users/connect endpoint
• users.ts:328-375 — /users/disconnect endpoint

🟠 2) agents-docs/ Missing conceptual documentation for Slack integration

Issue: This PR adds comprehensive Slack Work App integration (workspace installation, user linking, slash commands, @mentions, dashboard configuration) but includes only auto-generated API reference pages. No conceptual or tutorial documentation explains what the integration does or how customers should use it.

Why: Customers discovering the Slack integration will find API endpoints but have no guidance on how to actually use the feature. The dashboard links to https://docs.inkeep.com/integrations/slack which presumably does not exist yet. Questions like "How do I get the Inkeep bot into my Slack workspace?" or "What does /inkeep link do?" have no answers in customer-facing docs.

Fix: Add a conceptual/tutorial page at agents-docs/content/talk-to-your-agents/slack.mdx (or similar location) covering:

1. Overview of Slack integration capabilities
2. Prerequisites (Slack workspace admin access, Nango account if self-hosting)
3. Installation flow via dashboard
4. Account linking with /inkeep link
5. Configuring workspace and channel default agents
6. Using slash commands (/inkeep, /inkeep help, /inkeep status, /inkeep run)
7. Using @mentions for public responses
8. Troubleshooting common issues

The internal README at packages/agents-work-apps/src/slack/README.md is a good starting point to adapt.

Refs:

• slack-dashboard.tsx:94-95 — Help button links to non-existent docs
• README.md — Internal docs to adapt

🟠 3) packages/agents-work-apps/src/__tests__/slack/ OAuth state management and modal submission handlers have zero test coverage

Issue: Critical security and user-facing code paths have no test coverage:

• OAuth state functions (createOAuthState, parseOAuthState in oauth.ts lines 41-93) — implement HMAC-SHA256 signature generation/verification for CSRF protection
• Modal submission handlers (handleModalSubmission, handleFollowUpSubmission in modal-submission.ts lines 32-339) — 300+ lines handling form submissions, JWT signing, API calls with timeouts

Why:

• OAuth: A regression in signature generation could allow state forgery attacks. A bug in timestamp validation could cause OAuth flows to fail or allow replay attacks.
• Modal: A regression in metadata parsing could cause agent selection to fail silently. The 30-second timeout handling is untested and could cause unhandled promise rejections.

Fix: Add dedicated test files:

// oauth.test.ts
it('should reject forged state signature', async () => {
  const state = createOAuthState('tenant-1');
  const [data] = state.split('.');
  const forgedState = data + '.forgedSignature';
  expect(parseOAuthState(forgedState)).toBeNull();
});

// modal-submission.test.ts
it('should prompt unlinked user to link account', async () => {
  vi.mocked(findCachedUserMapping).mockResolvedValue(null);
  await handleModalSubmission(mockView);
  expect(mockPostEphemeral).toHaveBeenCalledWith(
    expect.objectContaining({ text: expect.stringContaining('link your account') })
  );
});

Refs:

• oauth.ts:41-93 — OAuth state functions
• modal-submission.ts:32-339 — Modal handlers

Inline comments:

• 🟠 Major: users.ts:426 User endpoints accept arbitrary userId
• 🟠 Major: commands/index.ts:479 Background agent execution fetch lacks timeout

🟡 Minor (3) 🟡

🟡 1) packages/agents-work-apps/src/slack/services/ Multiple internal API fetches lack timeout

Issue: Several fetch() calls to internal manage/run APIs lack timeouts: fetchAgentsFromManageApi (commands/index.ts:52-58), fetchProjectsForTenant (utils.ts:212), fetchAgentsForProject (utils.ts:253), storeWorkspaceInstallation (nango.ts:428).

Why: Internal services are not immune to latency spikes. Hung requests delay user feedback and can accumulate under load.

Fix: Add a shared timeout constant (10s for internal calls) and apply AbortController consistently.

Refs:

• modal-submission.ts:353-381 — timeout pattern already implemented

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

Issue: This PR adds significant new exports to @inkeep/agents-work-apps (Slack integration) but no changeset exists. The existing changeset only covers @inkeep/agents-manage-ui.

Why: Without a changeset, new exports won't appear in release notes and consumers upgrading won't know about the Slack integration.

Fix:

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

Refs:

• AGENTS.md — Changesets

🟡 3) agents-manage-ui/src/features/work-apps/slack/components/slack-dashboard.tsx:91 Icon-only link button missing aria-label

Issue: The help button (line 91-99) uses <Button variant="ghost" size="icon"> wrapping an anchor but lacks an explicit aria-label. The Tooltip wrapper doesn't reliably provide accessible names for screen readers.

Why: Screen reader users may hear only the link destination URL when navigating to this button.

Fix: Add aria-label="View Slack integration documentation" to the anchor element.

Refs:

• Same class of issue as previously flagged at workspace-hero.tsx

Inline comments:

• 🟡 Minor: schemas.ts:2774 Mid-file import placement breaks convention
• 🟡 Minor: workspaces.ts:319 Inconsistent workspace identifier naming

💭 Consider (4) 💭

💭 1) packages/agents-core/src/db/runtime/runtime-schema.ts:205 Varchar vs enum type mismatch for status field
Issue: The status field is typed as varchar while the Zod schema restricts it to ['active', 'suspended', 'disconnected']. Direct DB writes could insert invalid values.
Why: Relies on Zod layer for enforcement; database doesn't reject invalid values.
Fix: Consider using a Postgres enum type for stronger enforcement, or accept the current design if all writes go through Zod validation.

💭 2) packages/agents-work-apps/src/slack/types.ts:1 WorkAppsVariables duplicates BaseAppVariables fields
Issue: WorkAppsVariables manually defines fields that already exist in BaseAppVariables from agents-api/src/types/app.ts.
Why: Creates parallel type definitions that could drift.
Fix: Consider importing and extending BaseAppVariables, or add a comment explaining the intentional duplication for package encapsulation.

💭 3) packages/agents-core/src/db/runtime/runtime-schema.ts Slack tables missing Drizzle relations
Issue: GitHub work app tables define explicit Drizzle relations. Slack tables do not, despite having logical relationships.
Why: May limit query capabilities (no eager loading of related records).
Fix: Add workAppSlackWorkspacesRelations etc. following the GitHub pattern, if relational queries are needed.

💭 4) packages/agents-work-apps/src/slack/i18n/strings.ts Emoji usage in user-facing strings
Issue: Emojis are used throughout Slack-facing UI strings. A prior human review thread discussed whether this signals "vibe-coded" vs enterprise-grade.
Why: Product taste decision with accessibility implications (screen readers announce emoji names).
Fix: No code change required — flagging for explicit product decision. Consider auditing for consistency if emojis are kept.

🕐 Pending Recommendations (2)

• 🟡 security.ts:38 timingSafeEqual lacks explicit buffer length check
• 🟡 changeset Missing changeset for new package exports

Summary: This is a comprehensive Slack integration that follows established patterns well. The security fixes from prior reviews have been implemented correctly. The remaining issues are primarily around: (1) authorization checks on user-facing endpoints, (2) missing customer documentation, and (3) test coverage gaps for security-critical code paths. None are blocking, but the user endpoint authorization issue (#1) should be addressed before launch to prevent cross-user data access.

PR Review Summary

0 Key Findings | Risk: None (refactoring only)

📋 Delta Review Scope

This is a delta review scoped to 1 commit since the last automated review:

Verification: ✅ The middleware logic is identical — this is purely a code organization improvement. No functional changes, no new security concerns.

This refactoring directly addresses feedback from @miles-kt-inkeep who suggested moving the middleware to its own file for better organization.

🕐 Pending Recommendations (7)

Issues raised in prior reviews that remain active:

Major:

• 🟠 /work-apps/slack/users endpoint authorization` User endpoints need authorization checks — current implementation allows any authenticated user to access any workspace's users
• 🟠 Missing changeset PR adds new database tables and public API surface without a changeset entry
• 🟠 Fetch timeout handling External API calls (Nango, Slack) should have explicit timeouts

Minor:

• 🟡 OAuth state signature verification Thread still active but appears addressed in recent commits
• 🟡 Test coverage gaps OAuth flow and modal submission lack dedicated test coverage
• 🟡 Documentation gaps No conceptual docs at /integrations/slack — dashboard links to non-existent page
• 🟡 Type safety in middleware as any casts in workAppsAuth.ts could be improved

Summary: This delta commit is a clean refactoring that improves code organization by extracting the workAppsAuth middleware to its own file. The logic is identical to the previous inline implementation. No new issues introduced. The 7 pending recommendations from prior reviews should be addressed before merge.

PR Review Summary

0 Key Findings | Risk: None (fixes only)

📋 Delta Review Scope

This is a delta review scoped to 1 commit since the last automated review:

✅ Fixes Verified

The delta commit directly addresses 6 prior review findings. All implementations are correct:

Implementation Quality

Authorization helper (isAuthorizedForUser):

• ✅ Correctly handles no-session case (returns true for unauthenticated endpoints like link-status)
• ✅ Allows matching session userId
• ✅ Bypasses for system tokens and API keys (sessionUserId === 'system' or startsWith('apikey:'))
• ✅ Includes dev bypass for local development

Timeout patterns:

• ✅ 30s timeout for background agent execution matches existing pattern in modal-submission.ts:353-381
• ✅ 10s timeout for internal API calls is consistent across both fetch functions
• ✅ Proper finally block ensures timeout is always cleared
• ✅ AbortError is caught and returns user-friendly message

🕐 Pending Recommendations (6)

Issues raised in prior reviews that remain unaddressed:

Summary: The delta commit correctly addresses all 6 targeted issues from prior review feedback. The authorization helper follows established patterns, timeout implementations are consistent with existing code, and the naming/import fixes improve consistency. The 6 pending recommendations from earlier reviews should still be addressed before merge — particularly the OAuth state signature and Nango webhook verification which are security-related.

PR Review Summary

5 Key Findings | Risk: High

This is a comprehensive Slack Work App integration (~21K lines, 109 files) establishing the foundational pattern for real-time Work App integrations. The architecture is generally solid with proper HMAC signature verification, JWT-based authentication, and tenant isolation in the database layer. Recent commits have addressed many prior review findings. However, critical tenant isolation gaps remain in the workspace GET endpoints that should be addressed before merge.

🟠⚠️ Major (3) 🟠⚠️

🟠 1) packages/agents-work-apps/src/slack/routes/workspaces.ts Multiple GET routes lack tenant ownership verification

Issue: Beyond the base GET /workspaces endpoint (covered in inline comment), the following GET endpoints accept a teamId from URL params and return data without verifying the workspace belongs to the authenticated user's tenant:

• GET /workspaces/:teamId (lines 139-200)
• GET /workspaces/:teamId/settings (lines 202-247)
• GET /workspaces/:teamId/channels (lines 427-531)
• GET /workspaces/:teamId/channels/:channelId/settings (lines 533-584)
• GET /workspaces/:teamId/users (lines 869-929)
• GET /workspaces/:teamId/health (lines 931-1045)

The requireWorkspaceAdmin middleware only runs on PUT/DELETE operations (lines 54-66), not GET.

Why: An authenticated user from Tenant A can access workspace details, channel configurations, linked users, and health status for any workspace belonging to Tenant B by providing Tenant B's Slack team ID. This leaks sensitive information including channel names, linked user email addresses, and configuration details across tenant boundaries.

Fix: Add tenant ownership verification to all GET workspace routes. Either:

1. Apply requireWorkspaceAdmin to GET operations (rename to requireWorkspaceAccess)
2. Or add inline check: if (workspace.tenantId !== c.get('tenantId')) { return c.json({ error: 'Workspace not found' }, 404); }

Refs:

• workspaces.ts:139-200
• OWASP Broken Access Control

🟠 2) agents-docs/ Missing conceptual documentation for Slack integration

Issue: The dashboard links to https://docs.inkeep.com/integrations/slack (slack-dashboard.tsx line 96), but no documentation page exists at that path. The agents-docs/content/ directory has no /integrations/ subdirectory. The only Slack documentation is auto-generated OpenAPI reference which lists endpoints but doesn't explain the user experience.

Why: Customers who click the help icon, search for "Slack integration" in docs, or want to understand the mental model before diving into API reference will hit a 404 or find nothing. This creates a poor onboarding experience and increases support burden.

Fix: Create a conceptual docs page at agents-docs/content/integrations/slack.mdx covering:

1. Overview: What the Slack integration enables
2. Prerequisites: Required env vars, Nango configuration
3. Installation: Step-by-step for workspace admins
4. User linking: How /inkeep link works and why it's needed
5. Usage: Public (@mention) vs Private (/inkeep) responses
6. Configuration hierarchy: workspace default → channel default

Update the help link in slack-dashboard.tsx to point to the actual docs path.

Refs:

• slack-dashboard.tsx:94-97

Inline comments:

• 🟠 Major: workspaces.ts:116-131 Cross-tenant workspace enumeration
• 🟠 Major: oauth.ts (file) OAuth flow has zero test coverage

🟡 Minor (2) 🟡

🟡 1) packages/agents-work-apps/src/slack/services/commands/index.ts:52-58 Fetch calls in fetchAgentsFromManageApi lack timeout

Issue: Unlike fetchProjectsForTenant and fetchAgentsForProject in utils.ts which use AbortController with INTERNAL_FETCH_TIMEOUT_MS = 10_000, the fetch calls in fetchAgentsFromManageApi (lines 52 and 77) can block indefinitely if the manage API is slow.

Why: This function is called by /inkeep list and /inkeep run commands. A hung request blocks the Slack command response, consuming server resources. The same codebase has already established the timeout pattern, making this inconsistency notable.

Fix: Add AbortController with timeout matching the pattern in utils.ts.

Refs:

• utils.ts:213-246

🟡 2) packages/agents-work-apps/src/slack/README.md:13-30 README references non-existent documentation folder

Issue: Lines 13-30 link to docs/INDEX.md, docs/spec/, docs/flows/, and docs/developer/ directories that do not exist in the repository.

Why: Developers following this README will encounter broken links when trying to access architecture documentation.

Fix: Either create the referenced documentation structure, or update the README to remove references to non-existent files.

Inline comments:

• 🟡 Minor: app-mention.ts:282-288 Fallback error silently swallowed
• 🟡 Minor: streaming.ts:258-260 Error notification failure silently swallowed
• 🟡 Minor: workspace-hero.tsx:245-252 Icon-only link missing accessible name

💭 Consider (3) 💭

💭 1) packages/agents-core/src/utils/jwt-helpers.ts:36-40 JwtVerifyResult could use discriminated union
Issue: The type uses optional fields for mutually exclusive states, allowing { valid: true, payload: undefined }.
Why: Downstream code must use defensive checks that TypeScript can't narrow automatically.
Fix: Refactor to type JwtVerifyResult<T> = | { valid: true; payload: T } | { valid: false; error: string };

💭 2) packages/agents-work-apps/src/slack/i18n/strings.ts Emoji usage needs deliberate product decision
Issue: Extensive emoji use in customer-facing Slack messages (⚙️, 🔗, 💡, 🔊, 🔒, ❌). Per discussion, there's team disagreement.
Fix: Make a deliberate product choice: keep emojis (document the style), remove them, or use hybrid (status indicators only).

💭 3) packages/agents-work-apps/src/slack/services/agent-resolution.ts Dual source of truth for workspace defaults
Issue: Workspace default agent is stored in Nango metadata while channel defaults are in PostgreSQL. This creates different consistency guarantees.
Fix: Consider adding default_project_id and default_agent_id columns to work_app_slack_workspaces table, or document this split as intentional.

🕐 Pending Recommendations (6)

Issues from prior reviews that remain active and unresolved:

• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification (still has optional verification)
• 🟠 package.json:1 Missing changeset for new package exports
• 🟠 users.ts:140 User link verification accepts arbitrary userId from request body
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing accessible labels (broader than the specific link I flagged)
• 🟡 permissions.ts:65-70 Test environment bypass weakens permission testing
• 🟡 workAppSlack.ts:328-385 Check-then-act pattern vulnerable to TOCTOU race

Summary: The tenant isolation gaps in workspace GET routes are significant security issues that should be addressed. An authenticated user from any tenant can currently enumerate workspaces and access workspace details, channel configurations, and linked users across all tenants. The fix is straightforward (add tenant verification to GET routes). Once these issues are addressed, this is a well-architected integration that establishes a clean pattern for future Work App integrations. 🎉

PR Review Summary

1 Key Finding | Risk: Low

This delta review covers 1 commit (76cfd0ad) with 5 files changed (+159/-6 lines) since the last automated review. The changes directly address prior review findings: tenant isolation fix, OAuth test coverage, error logging improvements, and an accessibility fix. All implementations are correct.

🟠⚠️ Major (1) 🟠⚠️

🟠 1) oauth.test.ts Tests re-implement production logic instead of testing actual code

Issue: The new OAuth state management tests at lines 24-70 re-implement the createOAuthState and parseOAuthState functions rather than importing and testing the actual production code from oauth.ts. The comment on lines 24-25 explicitly acknowledges this: "Import the functions we need to test by re-implementing the logic (since createOAuthState/parseOAuthState are not exported)".

Why: This pattern provides false confidence. If the production code in oauth.ts diverges from this test implementation (e.g., a bug in parseOAuthState, the signing secret retrieval via getStateSigningSecret() changes, or the TTL constant is modified), the tests will continue passing while production is broken. The test hardcodes STATE_SECRET = 'test-signing-secret-for-oauth-state' while production uses getStateSigningSecret() with fallback logic. For security-critical HMAC verification code, this gap is concerning.

Fix: Export the production functions for testing:

// Option 1: Export from oauth.ts
export function createOAuthState(tenantId?: string): string { ... }
export function parseOAuthState(stateStr: string): OAuthState | null { ... }

// Option 2: Create dedicated oauth-state.ts module
// packages/agents-work-apps/src/slack/services/oauth-state.ts
export function createOAuthState(tenantId?: string): string { ... }
export function parseOAuthState(stateStr: string): OAuthState | null { ... }

// Then in oauth.test.ts:
import { createOAuthState, parseOAuthState } from '../../slack/services/oauth-state';

describe('parseOAuthState', () => {
  it('should accept valid state', () => {
    const state = createOAuthState('tenant-1');
    expect(parseOAuthState(state)).toMatchObject({ tenantId: 'tenant-1' });
  });
});

Refs:

• oauth.test.ts:24-70 — duplicated implementation
• oauth.ts — production code not exported
• pr-review-tests skill

🕐 Pending Recommendations (7)

Prior issues that remain unaddressed and should be considered before merge:

• 🟠 workspaces.ts:116-137 Cross-tenant workspace enumeration — multiple GET routes still lack tenant ownership verification
• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification
• 🟠 users.ts:140 User link verification accepts arbitrary userId without session validation
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing accessible labels (multiple instances)
• 🟡 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 addresses prior review findings well — the tenant isolation filter, OAuth tests, error logging, and accessibility fix are all correctly implemented. The one new finding (tests re-implementing production logic) is worth addressing to ensure the security-critical HMAC code is actually being tested. The pending recommendations from prior reviews represent remaining work that should be addressed before this large PR merges.

PR Review Summary

3 Key Findings | Risk: Medium

This is a comprehensive Slack Work App integration (~22K lines, 110 files) establishing the foundational patterns for real-time Work App integrations. The architecture is generally solid with proper HMAC signature verification, JWT-based authentication, and tenant isolation in the database layer. Recent commits have addressed many prior review findings. However, two authorization bugs remain that block session-based users from using the dashboard, and a prompt injection vector needs mitigation.

🟠⚠️ Major (3) ⚠️🟠

Inline comments:

• 🟠 Major: workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification, enabling cross-tenant data access
• 🟠 Major: permissions.ts:72-92 Permission middleware checks tenantId before resolveWorkAppTenantContext sets it, blocking all session users
• 🟠 Major: app-mention.ts:179-195 Thread context concatenated unsanitized into LLM prompt, enabling prompt injection via prior messages

🟡 Minor (3) 🟡

Inline comments:

• 🟡 Minor: slack-dashboard.tsx:94 Docs link points to non-existent /integrations/slack page (404)
• 🟡 Minor: env.ts:23-27 INKEEP_AGENTS_MANAGE_DATABASE_URL description says "Doltgres" but Slack uses runtime DB (Postgres)
• 🟡 Minor: slack-api.ts:1 Custom fetch wrapper diverges from makeManagementApiRequest pattern used elsewhere

🕐 Pending Recommendations (8)

Issues raised in prior reviews that remain active:

• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing aria-label attributes
• 🟡 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
• 🟠 users.ts:140 User link verification accepts arbitrary userId from request body
• 🟡 routes.test.ts:1 Test file location inconsistent with package convention
• 🟡 strings.ts:4 Emoji usage in i18n strings under discussion

Summary: The two authorization issues (tenant verification gap in GET settings + permission middleware blocking session users) effectively break the dashboard for production users. The session user bug means requireWorkspaceAdmin and requireChannelMemberOrAdmin always return 403 for cookie-authenticated dashboard users because tenantId is checked before resolveWorkAppTenantContext has a chance to set it. The fix is straightforward: move the !tenantId check to after the resolveWorkAppTenantContext call, or restructure to check only after context resolution. The prompt injection issue is lower priority but should be addressed before GA.

PR Review Summary

6 Key Findings | Risk: Medium

This is a comprehensive Slack Work App integration (~22K lines, 110 files) establishing the foundational pattern for real-time Work App integrations. The architecture is generally solid with proper HMAC signature verification, JWT-based authentication, and tenant isolation in the database layer. Recent commits have addressed many prior review findings (OAuth state signing, auth bypass removal, fetch timeouts in modal-submission.ts). However, tenant isolation gaps remain in several workspace endpoints, and SRE-critical timeout patterns are inconsistently applied.

🟠⚠️ Major (4) 🟠⚠️

Inline comments:

• 🟠 Major: workspaces.ts:1102 POST /:teamId/test-message lacks tenant ownership verification, allowing cross-tenant message injection
• 🟠 Major: workspaces.ts:736 Bulk channel operations (add/remove multiple channels) lack tenant verification
• 🟠 Major: nango.ts:428 External fetch to Nango API lacks AbortController timeout — can block indefinitely
• 🟠 Major: utils.ts:388 sendResponseUrlMessage fetch lacks timeout — Slack response_url calls can hang

🟡 Minor (6) 🟡

Inline comments:

• 🟡 Minor: workspaces.ts:660 PUT/DELETE channel settings missing tenant verification (parallel to GET endpoint issue)
• 🟡 Minor: block-actions.ts:128 Error handler provides no user feedback when agent selector modal fails
• 🟡 Minor: workspaces.ts:16 JSDoc header misleadingly implies permission middleware runs on all routes
• 🟡 Minor: slack-link-token.ts:14 JSDoc incorrectly describes flow as "device authorization"
• 🟡 Minor: permissions.ts:57 "workspace administrators" terminology is ambiguous (Slack vs org admin)
• 🟡 Minor: events.ts:359 Nango webhook returns success even when deletion cascade partially fails

🕐 Pending Recommendations (10)

Issues raised in prior reviews that remain active and should be addressed:

• 🟠 workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 app-mention.ts:179-195 Unsanitized thread context enables indirect prompt injection
• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification
• 🟠 package.json:1 Inconsistent @nangohq/node versions across packages (0.48.1 vs 0.69.x)
• 🟠 users.ts:140 User link verification accepts arbitrary userId from request body
• 🟡 slack-dashboard.tsx:94 Documentation link points to non-existent /integrations/slack page
• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing accessible aria-label attributes
• 🟡 permissions.ts:65-70 Test environment bypass skips all permission logic

Summary: The Slack Work App integration is architecturally sound with proper security patterns in place (HMAC verification, JWT authentication, tenant isolation at the DB layer). Recent commits have addressed critical security findings from prior reviews. The remaining issues fall into two categories: (1) tenant verification gaps in several workspace GET/POST endpoints that should be addressed before production use with multiple tenants, and (2) fetch timeout consistency — the pattern exists in modal-submission.ts but wasn't applied to all external API calls. The inline comments provide specific fix suggestions with code examples.

PR Review Summary

0 Key Findings | Risk: Low

This delta review covers commit 621e5f04 ("fix: address remaining tenant isolation, timeout, and documentation findings") with 7 files changed (+61/-21 lines) since the last automated review.

Excellent work addressing the prior feedback. All 10 MAJOR and MINOR findings from the previous automated review that were actionable in this delta have been correctly implemented.

🔴❗ Critical (0) ❗🔴

None.

🟠⚠️ Major (0) 🟠⚠️

None.

🟡 Minor (0) 🟡

None.

💭 Consider (1) 💭

💭 1) utils.ts:388-398 Timeout cleanup pattern differs from sibling functions

Issue: The sendResponseUrlMessage function clears the timeout on the success path (line 398) rather than in a finally block.

Why: This differs from the established pattern in fetchProjectsForTenant (lines 244-246 in the same file) which uses try/finally to ensure cleanup even when fetch throws. The practical impact is minimal (a 10-second hanging setTimeout reference that's a no-op), but the inconsistency could cause confusion during future maintenance.

Fix: Consider wrapping in try/finally to match sibling functions:

try {
  const response = await fetch(...);
  // ... handle response
} finally {
  clearTimeout(responseTimeout);
}

Refs: fetchProjectsForTenant pattern

🕐 Pending Recommendations (5)

These prior findings remain ACTIVE and were not addressed in this delta (they may be out of scope or intentionally deferred):

• 🟠 app-mention.ts:179-195 Unsanitized thread context enables indirect prompt injection
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification
• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 slack-dashboard.tsx:94 Documentation link points to non-existent page

Summary: This delta excellently addresses the prior review findings for tenant isolation, timeout handling, error feedback, and documentation accuracy. All implementations are correct and follow established patterns. The remaining pending recommendations are out of scope for this commit (they address files not modified in the delta). Great responsiveness to the review feedback! 🎉

PR Review Summary

0 Key Findings | Risk: Low

This is a delta review scoped to the 1 commit (85c3fe6b) since the last automated review.

Delta Changes Reviewed

The delta adds comprehensive Slack Work App documentation under agents-docs/content/talk-to-your-agents/slack/:

Additionally, the dashboard documentation link was fixed:

• Before: /api-reference/slack (which exists but was not the right user-facing doc)
• After: /talk-to-your-agents/slack/overview (the new user guide)

Documentation Quality Assessment

The documentation is well-structured and follows good practices:

✅ Proper frontmatter with title, sidebarTitle, description, and icon
✅ Clear organization using Steps, Cards, Tables, Tips, and Notes components
✅ Complete coverage of installation, linking, commands, agent resolution, and troubleshooting
✅ Accurate content that aligns with the implementation (verified against slack-dashboard.tsx)
✅ Consistent navigation with proper meta.json and cross-linking between pages

Prior Finding Addressed

🔴 The dashboard link fix directly addresses the ACTIVE prior finding at slack-dashboard.tsx:94 — documentation link now points to the newly created user guide.

🕐 Pending Recommendations (4)

These ACTIVE findings from prior reviews remain unaddressed and are outside this delta's scope:

• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟠 app-mention.ts:179-195 Unsanitized thread context enables indirect prompt injection
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification

Summary: The delta adds high-quality user documentation for the Slack Work App and fixes the dashboard documentation link. The documentation is well-organized, accurate, and provides clear guidance for users. The 4 pending recommendations from prior reviews are outside the scope of this documentation-focused commit — they should be addressed separately before merging. Great work on the docs! 📚

PR Review Summary

0 Key Findings | Risk: Low

This is a delta review scoped to 1 commit (9bc0c3f7) since the last automated review.

Delta Changes Reviewed

The delta fixes TypeScript errors in test mock objects by adding missing nullable fields:

Assessment

✅ Correct fix — The mock objects now match the actual WorkAppSlackUserMapping schema definition which includes these nullable fields
✅ Type safety — Aligns mock return values with expected types (e.g., null vs undefined for getBotTokenForTeam)
✅ No functional changes — Pure test infrastructure fix, no behavior differences

🕐 Pending Recommendations (9)

These ACTIVE findings from prior reviews remain unaddressed and are outside this delta's scope:

Major:

• 🟠 app-mention.ts:179-195 Unsanitized thread context enables indirect prompt injection
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification
• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification
• 🟠 users.ts:140 User link verification accepts arbitrary userId from request body
• 🟠 workAppSlack.ts:328-385 Check-then-act pattern vulnerable to TOCTOU race

Minor:

• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 workspace-hero.tsx:1 Icon-only buttons missing aria-label attributes
• 🟡 package.json:1 Inconsistent @nangohq/node versions across packages

Summary: The delta commit is a trivial test fix that correctly adds missing nullable fields to mock objects. No issues introduced. The prior ACTIVE findings remain relevant and should be addressed before final merge.

PR Review Summary

0 Key Findings | Risk: Low

This is a delta review scoped to 1 commit (4ea3f04a) since the last automated review.

Delta Changes Reviewed

The delta fixes a dev-mode edge case in the user link verification endpoint:

Before:

const inkeepUserId = sessionUserId || body.userId;

After:

const isRealSessionUser = sessionUserId && sessionUserId !== 'dev-user' && !sessionUserId.startsWith('apikey:') && sessionUserId !== 'system';
const inkeepUserId = isRealSessionUser ? sessionUserId : body.userId;

Assessment

✅ Correct fix — The change properly handles the case where sessionUserId is set to a synthetic identity (dev-user in dev mode, apikey:* for API key auth, or system for system tokens). These are not real database users and cannot be meaningfully linked to Slack accounts.

✅ Consistent with existing patterns — The logic aligns with isAuthorizedForUser() at lines 30-37 which uses the same set of bypass conditions.

✅ Security-appropriate — Production users get sessionUserId (authenticated session), while service principals and dev mode correctly fall back to body.userId for their intended use cases.

✅ No edge case gaps — API keys and system tokens are privileged principals trusted to act on behalf of users; dev mode is only available in non-production environments.

🕐 Pending Recommendations (9)

These ACTIVE findings from prior reviews remain unaddressed and are outside this delta's scope:

Major:

• 🟠 app-mention.ts:179-195 Unsanitized thread context enables indirect prompt injection
• 🟠 permissions.ts:72-91 Permission middleware blocks session-based users due to premature tenantId check
• 🟠 workspaces.ts:245 GET /:teamId/settings lacks tenant ownership verification
• 🟠 events.ts:289 Nango webhook endpoint lacks signature verification
• 🟠 users.ts:140 User link verification accepts arbitrary userId from request body
• 🟠 workAppSlack.ts:328-385 Check-then-act pattern vulnerable to TOCTOU race
• 🟠 package.json:1 Inconsistent @nangohq/node versions across packages

Minor:

• 🟡 slack-api.ts:1 UI API client diverges from established makeManagementApiRequest pattern
• 🟡 permissions.ts:65-70 Test environment bypass weakens permission testing

Summary: The delta fix is correct and addresses a legitimate dev-mode edge case. The change properly distinguishes between real authenticated users and synthetic/dev identities. Prior ACTIVE findings remain unresolved but are outside this delta's scope.