feat: join from workspace — auto-invite Slack users without Inkeep accounts#2219
feat: join from workspace — auto-invite Slack users without Inkeep accounts#2219amikofalvy merged 8 commits intomainfrom
Conversation
…counts When 'Join from Workspace' is enabled for a Slack workspace, users who run /inkeep link and don't have an Inkeep account are directed to an accept-invitation flow that creates their account and seamlessly chains into Slack account linking. Key changes: - tryAutoInvite creates (or reuses) a pending invitation via direct DB calls, using the org's serviceAccountUserId and preferredAuthMethod - handleLinkCommand chains: signup → accept invitation → /link?token=... - createInvitationInDb resolves inviterId/authMethod from the org record - verify-token handler handles re-linking and unique constraint races - JoinFromWorkspaceToggle UI with clearer copy and consistent styling - accept-invitation page supports returnUrl for post-signup redirect
🦋 Changeset detectedLatest commit: 343f1e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(7) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) GET /workspaces endpoint API response missing shouldAllowJoinFromWorkspace field
files:
packages/agents-work-apps/src/slack/routes/workspaces.ts:100-144agents-manage-ui/src/features/work-apps/slack/api/slack-api.ts:7-11agents-manage-ui/src/features/work-apps/slack/context/slack-provider.tsx:22
Issue: The GET /workspaces endpoint response schema and handler do not include shouldAllowJoinFromWorkspace. The handler uses listWorkspaceInstallations() which fetches data from Nango, not the PostgreSQL database where this field is stored. The frontend interface expects this field (shouldAllowJoinFromWorkspace?: boolean) but it will always be undefined.
Why: The JoinFromWorkspaceToggle component reads workspace?.shouldAllowJoinFromWorkspace to initialize its state. Since the API never returns this field, the toggle will always initialize to false regardless of the actual database value. Users will see the toggle in the wrong state after page refresh. The toggle appears to work when clicked (PUT endpoint updates DB), but state is lost on navigation/refresh.
Fix: Add shouldAllowJoinFromWorkspace to the response schema and fetch it from the database:
// In response schema (line 101-110), add:
shouldAllowJoinFromWorkspace: z.boolean().optional(),
// In handler, fetch from DB and merge:
const dbWorkspaces = await Promise.all(
workspaces.map((w) => findWorkAppSlackWorkspaceByTeamId(runDbClient)(sessionTenantId, w.teamId))
);
const dbWorkspaceMap = new Map(dbWorkspaces.filter(Boolean).map((w) => [w!.slackTeamId, w]));
return c.json({
workspaces: workspaces.map((w) => ({
// ... existing fields
shouldAllowJoinFromWorkspace: dbWorkspaceMap.get(w.teamId)?.shouldAllowJoinFromWorkspace ?? false,
})),
});Refs:
- workspaces.ts:100-144 — endpoint that needs update
- slack-api.ts:7-11 — frontend interface expecting the field
🔴 2) scope: test coverage No tests for new functionality
Issue: The PR adds significant new functionality with zero test coverage:
tryAutoInvitefunction (128 lines, multiple branches)createInvitationInDbfunction (throws on missing config)getOrganizationMemberByEmailfunction (DB join query)PUT /{teamId}/join-from-workspaceendpoint (authorization + tenant checks)
Why: This is a core user-facing flow that determines whether new users can join organizations. Without tests, regressions could go undetected: auto-invite silently failing, duplicate invitations, wrong user flows triggered, authorization bypasses.
Fix: Add test coverage for:
-
packages/agents-work-apps/src/__tests__/slack/tryAutoInvite.test.ts:should return null when shouldAllowJoinFromWorkspace is disabledshould return null when user already has Inkeep accountshould reuse existing pending invitationshould create new invitation when no existing pending invitationshould catch and log errors, returning null
-
packages/agents-core/src/data-access/runtime/__tests__/organizations.test.ts:createInvitationInDb - should throw when serviceAccountUserId not configuredcreateInvitationInDb - should throw when preferredAuthMethod not configuredcreateInvitationInDb - should create invitation with correct fields
-
packages/agents-work-apps/src/__tests__/slack/routes.test.ts:PUT /workspaces/:teamId/join-from-workspaceauthorization and CRUD tests
Refs:
🟠⚠️ Major (4) 🟠⚠️
🟠 1) scope: documentation New customer-facing feature without documentation
Issue: This PR introduces a significant new customer-facing feature ("Join from Workspace") that changes Slack user onboarding behavior, but no documentation updates are included.
Why: Admins will see a new toggle in the Slack dashboard with no documentation explaining: what it does, prerequisites (serviceAccountUserId, preferredAuthMethod must be configured), security implications (any Slack workspace member can create an account), or the user experience flow.
Fix: Update the following docs:
agents-docs/content/talk-to-your-agents/slack/configuration.mdx— Add section documenting the toggleagents-docs/content/talk-to-your-agents/slack/commands.mdx— Update/inkeep linkto describe both flowsagents-docs/content/talk-to-your-agents/slack/installation.mdx— Update onboarding section
Refs:
🟠 2) scope: changeset Missing changeset for public package changes
Issue: @inkeep/agents-core is a published npm package (version 0.50.5 with publishConfig.access: public). This PR adds new columns to the database schema and new exported data-access functions (createInvitationInDb, getOrganizationMemberByEmail).
Why: Schema changes that affect published packages should have a changeset to communicate the change to consumers. The new functions are exported via the package's public API surface.
Fix:
pnpm bump patch --pkg agents-core "Add organization service account and Slack workspace join-from-workspace support"Refs:
🟠 3) packages/agents-work-apps/src/slack/services/commands/index.ts Silent failure when org prerequisites not configured
Issue: If an admin enables "Join from Workspace" but the organization lacks serviceAccountUserId or preferredAuthMethod, the auto-invite flow silently fails. The tryAutoInvite function catches the error from createInvitationInDb and returns null, falling back to the JWT link flow with no indication that auto-invite failed due to misconfiguration.
Why: An admin enables the feature expecting one behavior, but users get a different flow without any feedback. The only evidence is a warn-level log, but no actionable user feedback exists. This could be confusing to debug months later.
Fix: Consider either:
- Validate prerequisites when toggling on in the
PUT /{teamId}/join-from-workspaceendpoint — return 400 if org is not configured - Distinguish configuration errors from transient failures in
tryAutoInviteand log at error level with actionable context - Add UI in Organization Settings to configure
preferredAuthMethodandserviceAccountUserId
Refs:
- organizations.ts:200-209 — throws on missing config
- commands/index.ts:131-134 — catches and returns null
Inline Comments:
- 🟠 Major:
accept-invitation/[invitationId]/page.tsx:151Open redirect vulnerability via unvalidated returnUrl
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
join-from-workspace-toggle.tsx:75-76Switch missing accessible label association - 🟡 Minor:
workspaces.ts:386Error response format inconsistency (c.text vs c.json)
💭 Consider (C) 💭
💭 1) packages/agents-core/src/data-access/runtime/organizations.ts:213 Invitation expiration mismatch
Issue: The invitation created by createInvitationInDb has a 1-hour expiry, but the Slack message tells users the link "expires in 10 minutes" (using LINK_CODE_TTL_MINUTES). These are different tokens with different expiries.
Why: Could cause confusion if the link token expires before the invitation or vice versa.
Fix: Consider aligning the expiry times or clarifying the user-facing message to reflect the shorter expiry (link token at 10 min).
💭 2) packages/agents-work-apps/src/slack/services/commands/index.ts:103-112 PII (email) logged at info level
Issue: User email addresses are logged at info level in multiple places in tryAutoInvite.
Why: If logs are shipped to third-party services or retained long-term, this could violate data handling policies.
Fix: Consider masking emails in logs (e.g., u***@domain.com) or logging at debug level.
🕐 Pending Recommendations (0)
None — this is the first review.
🚫 REQUEST CHANGES
Summary: This PR implements a valuable feature but has critical gaps that need to be addressed before merging:
- Functional bug: The API doesn't return
shouldAllowJoinFromWorkspace, so the toggle UI will always show the wrong state after page refresh - Security: The
returnUrlparameter needs validation to prevent open redirect - Testing: Core user-facing flow has zero test coverage per project requirements
- Documentation: New feature needs docs for admins to understand prerequisites and security implications
The graceful degradation design (silent fallback to JWT link flow) is well-considered, but the prerequisite configuration gap (serviceAccountUserId/preferredAuthMethod with no UI) may cause confusion.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
organizations.ts |
Function placed in organizations.ts instead of workAppSlack.ts | Valid placement — operates on invitation table which is part of Better Auth's organization domain |
auth-schema.ts:105-106 |
preferredAuthMethod should be an enum | Valid improvement but lower priority nitpick — runtime validation can catch invalid values |
organizations.ts:186-227 |
Return type could be more complete | Minor clarity improvement — current callers don't need the additional fields |
join-from-workspace-toggle.tsx:81 |
AlertCircle icons missing aria-hidden | Very minor accessibility nit — Alert component already has role="alert" |
0015_soft_payback.sql |
Missing trailing newline | Cosmetic issue — migration will apply correctly |
Reviewers (11)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
5 | 2 | 1 | 0 | 0 | 0 | 2 |
pr-review-consistency |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-breaking-changes |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-docs |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
7 | 1 | 0 | 0 | 0 | 0 | 6 |
pr-review-types |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-devops |
6 | 1 | 0 | 0 | 0 | 0 | 5 |
pr-review-frontend |
5 | 0 | 0 | 0 | 1 | 0 | 4 |
pr-review-errors |
3 | 1 | 1 | 0 | 0 | 0 | 1 |
| Total | 43 | 9 | 3 | 0 | 3 | 0 | 28 |
Note: High discard rate reflects extensive deduplication — multiple reviewers flagged the same API response issue.
| // Redirect to the organization after a short delay | ||
| setTimeout(() => { | ||
| router.push(orgId ? `/${orgId}/projects` : '/'); | ||
| router.push(returnUrl || (orgId ? `/${orgId}/projects` : '/')); |
There was a problem hiding this comment.
🟠 MAJOR: Open redirect vulnerability via unvalidated returnUrl
Issue: The returnUrl search parameter is passed directly to router.push() without validation. An attacker could craft a malicious URL with an external returnUrl (e.g., returnUrl=https://evil.com/phishing) that redirects users to a phishing site after signup.
Why: The codebase already has isValidReturnUrl() in agents-manage-ui/src/lib/utils/auth-redirect.ts that validates returnUrl must be a relative path starting with / and rejects protocol-relative URLs (//). The login page uses this validation, but this page doesn't — creating inconsistent security posture.
Fix: Use the existing validation utility:
import { getSafeReturnUrl } from '@/lib/utils/auth-redirect';
// In both setTimeout callbacks:
router.push(getSafeReturnUrl(returnUrl, orgId ? `/${orgId}/projects` : '/'));Refs:
| <Label className="text-sm font-medium">Allow workspace members to join</Label> | ||
| <Switch checked={isEnabled} onCheckedChange={handleToggle} disabled={isUpdating} /> |
There was a problem hiding this comment.
🟡 Minor: Switch missing accessible label association
Issue: The <Label> element is not associated with the <Switch> control. The Label lacks an htmlFor attribute and the Switch lacks an id. Screen reader users will hear the Switch announced without context.
Why: Per accessibility best practices, form controls require accessible names. Currently users may hear "toggle button, off" without knowing what it controls.
Fix:
| <Label className="text-sm font-medium">Allow workspace members to join</Label> | |
| <Switch checked={isEnabled} onCheckedChange={handleToggle} disabled={isUpdating} /> | |
| <Label htmlFor="join-from-workspace-switch" className="text-sm font-medium">Allow workspace members to join</Label> | |
| <Switch id="join-from-workspace-switch" checked={isEnabled} onCheckedChange={handleToggle} disabled={isUpdating} /> |
Refs:
| // Get the session tenant ID for authorization | ||
| const sessionTenantId = c.get('tenantId') as string | undefined; | ||
| if (!sessionTenantId) { | ||
| return c.text('Unauthorized', 401); |
There was a problem hiding this comment.
🟡 Minor: Error response format inconsistency
Issue: This endpoint returns plain text errors via c.text() while peer endpoints in the same file use c.json({ error: ... }) format.
Why: Consistent error formats make client-side error handling more predictable. The frontend slackApi.updateJoinFromWorkspaceSetting expects JSON errors (see response.json().catch()).
Fix: Change to JSON error responses:
return c.json({ error: 'Unauthorized' }, 401);
// ...
return c.json({ error: 'Workspace not found' }, 404);
// ...
return c.json({ error: 'Failed to update setting' }, 500);Refs:
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
This is a delta review scoped to changes since the previous automated review. The bd9b52ed7 commit ("address comments") successfully addressed 2 of the 7 prior issues:
✅ Fixed: API response now properly returns shouldAllowJoinFromWorkspace via dedicated endpoints
✅ Fixed: Comprehensive test coverage added (535+ new lines across 3 test files)
🔴❗ Critical (0) ❗🔴
None in this delta.
🟠⚠️ Major (0) 🟠⚠️
None in this delta.
🟡 Minor (0) 🟡
None in this delta.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (4)
These issues were raised in the prior review and remain unresolved in this delta:
- 🟠
accept-invitation/page.tsx:151Open redirect vulnerability via unvalidatedreturnUrl— usegetSafeReturnUrl()from existing auth-redirect utility - 🟠
scope: documentationMissing user documentation for the new "Join from Workspace" feature - 🟡
join-from-workspace-toggle.tsx:83-84Switch missing accessible label association (htmlFor/id) - 🟡
workspaces.ts:433Error response format inconsistency (c.text()vsc.json())
Delta Review Assessment:
The new code introduced in this commit is well-structured:
- ✅ New GET endpoint correctly returns the setting from the database
- ✅ Frontend change properly fetches from the dedicated endpoint on mount with loading state
- ✅ Test coverage is comprehensive — tests cover disabled setting, enabled setting, auth failures, 404, 500, and reuse of pending invitations
What still needs attention:
The open redirect vulnerability (returnUrl passed directly to router.push()) is the most critical outstanding item. The fix is straightforward — the codebase already has getSafeReturnUrl() in agents-manage-ui/src/lib/utils/auth-redirect.ts that validates return URLs. This should be applied before merging.
💡 APPROVE WITH SUGGESTIONS
Summary: The delta effectively addresses the API response and test coverage issues from the prior review. The open redirect vulnerability (Major) and missing documentation (Major) remain unaddressed and should be fixed before merging. The accessibility and error format consistency issues are minor but worth fixing for polish.
Once the open redirect is fixed (single-line change using existing utility), this PR will be in good shape.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
scope: changeset |
Missing changeset for public package changes | Lower priority — changesets can be added before merge if maintainers prefer |
scope: silent failure |
Silent failure on org prerequisite misconfiguration | Design choice — graceful degradation is intentional; could be improved but not blocking |
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
6 | 0 | 0 | 0 | 0 | 4 | 2 |
| Total | 6 | 0 | 0 | 0 | 0 | 4 | 2 |
Note: Delta review performed by orchestrator directly — no subagent dispatch needed for the focused scope of changes.
There was a problem hiding this comment.
remove this file (rebase on latest main should fix this flaky test, i think)
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (bd9b52ed7). The latest commit (39f57476a address comments) successfully addresses:
✅ Fixed: Open redirect vulnerability — now uses getSafeReturnUrl() to validate returnUrl before passing to router.push()
✅ Fixed: Removed flaky test screenshot file per @amikofalvy's review comment
🔴❗ Critical (0) ❗🔴
None in this delta.
🟠⚠️ Major (0) 🟠⚠️
None in this delta.
🟡 Minor (0) 🟡
None in this delta.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (2)
These issues were raised in prior reviews and remain unresolved:
- 🟡
join-from-workspace-toggle.tsx:83-84Switch missing accessible label association (htmlFor/id) - 🟡
workspaces.ts:433Error response format inconsistency (c.text()vsc.json())
Delta Assessment:
The security fix is correctly implemented:
- Line 16: Imports
getSafeReturnUrlfrom@/lib/utils/auth-redirect - Lines 152, 190: Both redirect paths now use
getSafeReturnUrl(returnUrl, fallback)instead of rawreturnUrl
The getSafeReturnUrl function validates that returnUrls are:
- Relative paths (start with
/) - Not protocol-relative URLs (reject
//) - Not encoded absolute URLs (rejects URLs containing
://after decoding)
This matches the existing validation pattern used by the login page.
✅ APPROVE
Summary: The critical open redirect vulnerability has been properly fixed using the existing getSafeReturnUrl() utility. The remaining Minor issues (accessibility label association, error format consistency) are polish items that can be addressed in a follow-up. This PR is now safe to merge.
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
2 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta review performed by orchestrator directly — minimal changes didn't warrant subagent dispatch.
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE "organization" ADD COLUMN "preferred_auth_method" text;--> statement-breakpoint | |||
| ALTER TABLE "organization" ADD COLUMN "service_account_user_id" text;--> statement-breakpoint | |||
There was a problem hiding this comment.
In future PR, disallow removal of the organization's Service Account User
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (39f57476a). The only change is merge commit 00aea0078 which brings main into the feature branch.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None in this delta.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (2)
These issues were raised in prior reviews and remain unresolved:
- 🟡
join-from-workspace-toggle.tsx:83-84Switch missing accessible label association (htmlFor/id) - 🟡
workspaces.ts:433Error response format inconsistency (c.text()vsc.json())
Delta Assessment
What changed: Merge commit 00aea0078 integrating main into feat/auto-invite-work-app
Merged changes from main (not part of this PR's scope):
- Tool approvals in Slack (#2149)
- Copilot UI widget refactor (#1995)
- Version packages bump (#2218)
- Noisy instrumentation removal (#2194)
- Form screenshot update (#2220)
Merge integrity verified:
- ✅
createCreateInkeepAccountMessagefunction correctly coexists with new tool approval blocks inblocks/index.ts - ✅ No conflicts detected in PR-specific files
- ✅ Imports and exports remain consistent
- ✅ Test files unchanged from prior approved state
PR-specific code status: Unchanged since last APPROVED review — the 12 files and ~630 lines of additions remain identical to commit 39f57476a.
✅ APPROVE
Summary: The merge cleanly integrates main branch changes without affecting this PR's functionality. The prior approval stands — the "Join from Workspace" feature implementation is complete with comprehensive test coverage and proper security handling. The two remaining Minor issues (accessibility label, error format) are polish items that can be addressed in a follow-up PR if desired.
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
2 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta review performed by orchestrator directly — merge-only change didn't warrant subagent dispatch.
- Add useRef guard in link page to prevent duplicate useEffect invocations - Improve server-side 23505 unique constraint detection via error.cause.code - Return success instead of error when concurrent link resolves to existing mapping
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (00aea0078). The latest commit (2e686a10c fix: handle concurrent verify-token calls from React Strict Mode) addresses a race condition discovered during testing.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None in this delta.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (2)
These issues were raised in prior reviews and remain unresolved:
- 🟡
join-from-workspace-toggle.tsx:83-84Switch missing accessible label association (htmlFor/id) - 🟡
workspaces.ts:433Error response format inconsistency (c.text()vsc.json())
Delta Assessment
What changed: Commit 2e686a10c fixes concurrent verify-token API calls caused by React Strict Mode's double-render behavior.
Frontend (link/page.tsx):
- ✅ Added
useRefguard (linkingRef) to preventhandleLinkWithTokenfrom being called twice - ✅ Standard React pattern for preventing double-execution of effects in Strict Mode
- ✅ Ref is set synchronously before the async call, preventing race conditions
Backend (users.ts):
- ✅ Improved unique constraint violation detection to include PostgreSQL error code
23505(not just string matching) - ✅ Changed behavior: returns
{ success: true }instead of 409 conflict when the mapping already exists - ✅ This is the correct idempotent pattern — if two concurrent requests race to create the same mapping, the second request finding the mapping already exists should be treated as success
- ✅ Appropriate logging level change (
infoinstead ofwarn)
Code quality:
- The unique violation detection covers both string matching (for different drivers) and PostgreSQL-specific error codes (for Drizzle/pg)
- The
(error as any)cast is acceptable for error cause inspection where TypeScript can't know the shape
✅ APPROVE
Summary: The delta correctly addresses the React Strict Mode race condition with a well-implemented fix on both frontend (ref guard) and backend (idempotent unique constraint handling). The two remaining Minor issues from prior reviews (accessibility label, error format) are polish items that can be addressed in a follow-up PR if desired. This PR is ready to merge.
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
2 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta review performed by orchestrator directly — focused scope didn't warrant subagent dispatch.
amikofalvy
left a comment
amikofalvy
left a comment
There was a problem hiding this comment.
Address the two minor pending recommendations from the auto reviewer then 
🕐 Pending Recommendations (2)
These issues were raised in prior reviews and remain unresolved:
🟡 #2219 (comment) Switch missing accessible label association (htmlFor/id)
🟡 #2219 (comment) Error response format inconsistency (c.text() vs c.json())
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (2e686a10c). The latest commit (a39d11718 Fix accessible label association and error response format consistency) addresses the final two pending Minor recommendations.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None — both pending Minor issues from prior reviews have been resolved.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (0)
All prior recommendations have been addressed.
Delta Assessment
Commit a39d11718 successfully resolves the two remaining Minor issues:
✅ Accessibility fix (join-from-workspace-toggle.tsx):
- Added
htmlFor="join-from-workspace-switch"to the Label - Added
id="join-from-workspace-switch"to the Switch - Screen readers will now correctly associate the label with the toggle control
✅ Error response consistency (workspaces.ts):
- Changed all
c.text()error responses toc.json({ error: '...' }) - Now consistent with peer endpoints in the same file
- Frontend error handling will work correctly (
response.json().catch())
Review History Summary:
- 🔴 Critical (2) — API response missing field ✅, Test coverage ✅ — Fixed in earlier commits
- 🟠 Major (3) — Open redirect vulnerability ✅, Missing docs ⏸️, Silent failure ⏸️ — Security fixed; docs/UX improvements can be follow-up
- 🟡 Minor (2) — Accessibility ✅, Error format ✅ — Fixed in this commit
✅ APPROVE
Summary: All blocking issues and pending recommendations have been addressed. The "Join from Workspace" feature implementation is complete with proper test coverage, security handling (open redirect fix), accessibility compliance, and API consistency. This PR is ready to merge. 🚀
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review performed by orchestrator directly — focused scope of changes (2 files, 21 lines) addressing previously identified issues didn't warrant subagent dispatch.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (a39d11718). The latest commit (afb01269b Add changeset for error response format fix) adds a changeset documenting the error response format consistency fix.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (0)
All prior recommendations have been addressed.
Delta Assessment
Commit afb01269b adds a changeset for @inkeep/agents-work-apps documenting the error response format consistency fix:
---
"@inkeep/agents-work-apps": patch
---
Fix error response format consistency in join-from-workspace endpointsThis is good housekeeping — the changeset ensures the fix will be properly documented in the package changelog when released.
Review History Summary:
- 🔴 Critical (2) — API response missing field ✅, Test coverage ✅ — Fixed
- 🟠 Major (3) — Open redirect vulnerability ✅, Missing docs ⏸️, Silent failure ⏸️ — Security fixed; docs/UX improvements can be follow-up
- 🟡 Minor (2) — Accessibility ✅, Error format ✅ — Fixed
- ✅ Changeset — Now added for the error format fix
✅ APPROVE
Summary: The delta is a trivial housekeeping change adding a changeset for the error response format fix. All blocking issues and pending recommendations from prior reviews have been addressed. The "Join from Workspace" feature implementation is complete with proper test coverage, security handling, accessibility compliance, API consistency, and proper changelog documentation. This PR is ready to merge. 🚀
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review performed by orchestrator directly — trivial changeset addition didn't warrant subagent dispatch.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since the previous automated review (afb01269b). The latest commit (343f1e748 Add changesets for agents-core (minor) and agents-manage-ui (patch)) adds two changesets for proper release documentation.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
🕐 Pending Recommendations (0)
All prior recommendations have been addressed.
Delta Assessment
Commit 343f1e748 adds two changesets:
-
@inkeep/agents-manage-ui: patchAdd join-from-workspace toggle and auto-invite flow for Slack users
✅ Appropriate — UI additions are additive features,
patchis correct. -
@inkeep/agents-core: minorAdd organization service account, preferred auth method, and Slack workspace join-from-workspace schema support
✅ Appropriate — Schema changes requiring migrations warrant
minorper AGENTS.md semver guidance.
Review History Summary:
- 🔴 Critical (2) — API response missing field ✅, Test coverage ✅ — Fixed
- 🟠 Major (3) — Open redirect vulnerability ✅, Missing docs ⏸️, Silent failure ⏸️ — Security fixed; docs/UX improvements can be follow-up
- 🟡 Minor (2) — Accessibility ✅, Error format ✅ — Fixed
- ✅ Changesets — Now complete for all affected packages
✅ APPROVE
Summary: The delta adds proper changelog entries for the affected packages with correct semver levels. All blocking issues from prior reviews have been addressed. The "Join from Workspace" feature implementation is complete with:
- ✅ Comprehensive test coverage (300+ lines across 3 test files)
- ✅ Security handling (open redirect fix using
getSafeReturnUrl) - ✅ Accessibility compliance (label association for toggle)
- ✅ API consistency (JSON error responses)
- ✅ Proper changelog documentation (3 changesets)
This PR is ready to merge. 🚀
Discarded (0)
None.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review (orchestrator) |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review performed by orchestrator directly — trivial changeset additions didn't warrant subagent dispatch.
Document the new auto-invite flow for Slack users without Inkeep accounts (PR #2219). Adds a new "Join from Workspace" section explaining the feature, how to enable it, and the end-to-end user flow. Also adds the corresponding permission to the permission model table.
2 participants
Summary
When Join from Workspace is enabled for a Slack workspace, users who run
/inkeep linkand don't have an Inkeep account are directed through an accept-invitation flow that creates their account and seamlessly chains into Slack account linking.Flow
/inkeep linkin SlacktryAutoInvitechecks if the workspace hasshouldAllowJoinFromWorkspaceenabled, fetches the user's email from Slack, and verifies they don't already have an Inkeep accountinviterIdandauthMethodsourced from the organization'sserviceAccountUserIdandpreferredAuthMethodcolumns/accept-invitation/{id}?returnUrl=/link?token=.../link?token=...which completes Slack linkingKey Changes
Backend (
agents-work-apps,agents-core)tryAutoInviteincommands/index.ts— uses direct DB calls, deduplicates pending invitations, returns invitation info for the accept-invitation URLcreateInvitationInDbinorganizations.ts— self-resolvesserviceAccountUserIdandpreferredAuthMethodfrom the org record (throws if not configured)verify-tokenhandler inusers.ts— handles re-linking to different users and unique constraint race conditionscreateCreateInkeepAccountMessageinblocks/index.ts— Slack Block Kit message with 'Create Account' CTAgetOrganizationMemberByEmailinusers.tsdata-access — checks org membership by emailpreferredAuthMethodandserviceAccountUserIdcolumns to organization tableFrontend (
agents-manage-ui)accept-invitationpage — supportsreturnUrlsearch param for post-signup redirectJoinFromWorkspaceToggle— new component with clearer copy, consistent card styling, label doesn't toggle switchGated By
The entire auto-invite flow is gated by the
shouldAllowJoinFromWorkspaceflag on the workspace record, controlled by theJoinFromWorkspaceToggleUI.