feat: forward identity fields from all bots to control plane#552
feat: forward identity fields from all bots to control plane#552ColeMurray merged 2 commits intomainfrom
Conversation
Wire GitHub bot, Slack bot, and Linear bot to send identity fields (scmUserId/actorUserId, actorDisplayName, actorEmail) when creating sessions, enabling the control plane to resolve canonical user IDs. Part of unified user model Phase 4 (#523).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSession creation flows for GitHub, Linear, and Slack bots now capture and forward actor identity fields (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Bot as Bot (GitHub/Slack/Linear)
participant Provider as Provider API (GitHub/Slack/Linear)
participant Control as Control Plane (/sessions)
Bot->>Provider: optional user-info lookup (id, name, email)
Note right of Provider: may return identity or fail
Bot->>Control: POST /sessions with actorUserId, actorDisplayName?, actorEmail?, spawnSource
Control-->>Bot: session created (response)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR Title and number: feat: forward identity fields from all bots to control plane (#552)
Author: @ColeMurray
Files changed count, additions/deletions: 5 files, +177/-7
This wires the GitHub, Slack, and Linear bot session creation paths to forward provider identity fields to the control plane so canonical users can be resolved there. I reviewed the changed code paths and ran the touched package test suites locally; the implementation looks consistent and the new Slack/GitHub coverage matches the behavior in the diff.
Critical Issues
- None.
Suggestions
- [Testing]
packages/linear-bot/src/webhook-handler.ts:515- This change adds new behavior in the Linear webhook path, but the PR does not add a webhook-level assertion thatactorUserId,actorDisplayName, andactorEmailreach session creation. The existingfetchUserunit tests cover the helper itself; adding one end-to-end handler assertion would make regressions here easier to catch.
Nitpicks
- None.
Positive Feedback
- GitHub bot forwarding is applied consistently across all four handlers, which keeps the identity behavior uniform.
- Slack bot treats identity enrichment as best-effort and preserves the existing session creation path when profile lookup data is unavailable.
- The new Slack and GitHub assertions are focused on externally visible behavior rather than implementation details.
Questions
- None.
Verdict
Approve: Ready to merge, no blocking issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/linear-bot/src/webhook-handler.ts (1)
494-523: Consolidate the twoif (appUserId)blocks.Lines 494–501 and 519–523 both guard on
appUserId. Merging them keeps related identity/preference resolution together and avoids an easy-to-miss second lookup when the field is optional.♻️ Suggested refactor
let userModel: string | undefined; let userReasoningEffort: string | undefined; + let actorDisplayName: string | undefined; + let actorEmail: string | undefined; const appUserId = webhook.appUserId; if (appUserId) { const prefs = await getUserPreferences(env, appUserId); if (prefs?.model) { userModel = prefs.model; } userReasoningEffort = prefs?.reasoningEffort; + + try { + const linearUser = await fetchUser(client, appUserId); + actorDisplayName = linearUser?.name; + actorEmail = linearUser?.email ?? undefined; + } catch { + // best-effort + } } const labelModel = extractModelFromLabels(labels); // ... existing resolveSessionModelSettings call ... - // ─── Resolve user identity ───────────────────────────────────────────── - - let actorDisplayName: string | undefined; - let actorEmail: string | undefined; - if (appUserId) { - const linearUser = await fetchUser(client, appUserId); - actorDisplayName = linearUser?.name; - actorEmail = linearUser?.email ?? undefined; - } - // ─── Create session ───────────────────────────────────────────────────🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/linear-bot/src/webhook-handler.ts` around lines 494 - 523, Merge the two separate appUserId guards into a single if (appUserId) block so preference and identity resolution run together: inside that block call getUserPreferences(env, appUserId) and assign userModel and userReasoningEffort from its result, then call fetchUser(client, appUserId) and set actorDisplayName and actorEmail from the returned linearUser; ensure you preserve use of extractModelFromLabels/resolveSessionModelSettings after userModel/userReasoningEffort are set and keep variable names (appUserId, userModel, userReasoningEffort, actorDisplayName, actorEmail) unchanged.packages/slack-bot/src/index.test.ts (1)
555-668: Solid integration test for identity forwarding.Good coverage: exercises the full
block_actions→startSessionAndSendPrompt→createSessionpath, stubs all required control-plane endpoints, and asserts the exactspawnSourceplus actor fields (display_namepreference overreal_name/nameis covered sinceprofile.display_name: "Jane"is asserted).One optional suggestion: consider adding a negative test that
getUserInforejection /ok: falsestill results in session creation (validating the best-efforttry/catchinindex.tslines 899–909). That swallow path is currently only covered indirectly via the default{ ok: true, user: undefined }mock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack-bot/src/index.test.ts` around lines 555 - 668, Add a negative test that simulates getUserInfo failing (either mockGetUserInfo.mockResolvedValue({ ok: false }) and another case mockRejectedValue(new Error(...))) and assert startSessionAndSendPrompt still triggers createSession on the control plane; specifically, reuse the existing test flow (payload, pending KV entry, CONTROL_PLANE.fetch stubs) but verify the sessionCall exists and that actorDisplayName/actorEmail are absent or undefined while spawnSource === "slack-bot", to validate the try/catch path in getUserInfo handling.packages/slack-bot/src/index.ts (2)
80-91:createSessionnow takes 10 positional parameters — consider switching to an options object.With
slackUserId,actorDisplayName, andactorEmailappended, the signature is getting hard to read and easy to misuse (several are optional strings of the same type, so a transposed argument would silently compile and run wrong). Switching to a params object also mirrors the pattern already used inpackages/github-bot/src/handlers.tsandpackages/linear-bot/src/webhook-handler.ts'screateSessionfunctions, which aids cross-bot consistency.♻️ Suggested signature
async function createSession( env: Env, - repo: RepoConfig, - title: string | undefined, - model: string, - reasoningEffort: string | undefined, - branch: string | undefined, - traceId?: string, - slackUserId?: string, - actorDisplayName?: string, - actorEmail?: string + params: { + repo: RepoConfig; + title?: string; + model: string; + reasoningEffort?: string; + branch?: string; + slackUserId?: string; + actorDisplayName?: string; + actorEmail?: string; + }, + traceId?: string ): Promise<{ sessionId: string; status: string } | null> {Call sites would then destructure
params— a larger change, so feel free to defer if out of scope for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack-bot/src/index.ts` around lines 80 - 91, The createSession function signature is overcrowded with 10 positional parameters (env, repo, title, model, reasoningEffort, branch, traceId, slackUserId, actorDisplayName, actorEmail); convert it to accept a single params object (e.g. createSession(env, repo, params)) where params is an object with named properties for title, model, reasoningEffort, branch, traceId, slackUserId, actorDisplayName, and actorEmail (mark appropriate fields optional), update the function implementation to destructure params, and update all call sites to pass an object (or destructured object) instead of positional args so arguments cannot be transposed and to match the pattern used by the other bots' createSession functions.
896-923: Best-effort identity resolution pattern looks correct.The
try/catchcorrectly swallows failures, and thedisplay_name || real_name || namefallback chain matches Slack's documented preference order. Using||(not??) is intentional here since Slack'sprofile.display_namecan be an empty string when unset, and the fallback toreal_nameis the desired behavior in that case.One minor note:
getUserInfoadds an extra round-trip to Slack on every session creation. This runs insidewaitUntil(background), so it won't affect the 3s Slack response budget, but if Slack's users.info ever slows down it will extend end-to-end "session started" latency. Consider caching the user profile in KV (e.g., 1-hour TTL keyed onuserId) as a follow-up if this becomes a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/slack-bot/src/index.ts` around lines 896 - 923, Best-effort identity resolution currently calls getUserInfo on every session creation which adds an extra Slack round-trip; to reduce latency, first try reading a cached profile from KV (keyed by userId) and only call getUserInfo on a cache miss, then store the resolved profile (display_name, real_name, name, email) into KV with a ~1-hour TTL; preserve the existing fallback logic for displayName and email and the try/catch behavior (i.e., if KV read or getUserInfo fails, continue with undefined displayName/email), and reference getUserInfo, displayName, email, and createSession so the cache read/write wraps the current logic before createSession is invoked (inside the waitUntil background flow).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/linear-bot/src/webhook-handler.ts`:
- Around line 494-523: Merge the two separate appUserId guards into a single if
(appUserId) block so preference and identity resolution run together: inside
that block call getUserPreferences(env, appUserId) and assign userModel and
userReasoningEffort from its result, then call fetchUser(client, appUserId) and
set actorDisplayName and actorEmail from the returned linearUser; ensure you
preserve use of extractModelFromLabels/resolveSessionModelSettings after
userModel/userReasoningEffort are set and keep variable names (appUserId,
userModel, userReasoningEffort, actorDisplayName, actorEmail) unchanged.
In `@packages/slack-bot/src/index.test.ts`:
- Around line 555-668: Add a negative test that simulates getUserInfo failing
(either mockGetUserInfo.mockResolvedValue({ ok: false }) and another case
mockRejectedValue(new Error(...))) and assert startSessionAndSendPrompt still
triggers createSession on the control plane; specifically, reuse the existing
test flow (payload, pending KV entry, CONTROL_PLANE.fetch stubs) but verify the
sessionCall exists and that actorDisplayName/actorEmail are absent or undefined
while spawnSource === "slack-bot", to validate the try/catch path in getUserInfo
handling.
In `@packages/slack-bot/src/index.ts`:
- Around line 80-91: The createSession function signature is overcrowded with 10
positional parameters (env, repo, title, model, reasoningEffort, branch,
traceId, slackUserId, actorDisplayName, actorEmail); convert it to accept a
single params object (e.g. createSession(env, repo, params)) where params is an
object with named properties for title, model, reasoningEffort, branch, traceId,
slackUserId, actorDisplayName, and actorEmail (mark appropriate fields
optional), update the function implementation to destructure params, and update
all call sites to pass an object (or destructured object) instead of positional
args so arguments cannot be transposed and to match the pattern used by the
other bots' createSession functions.
- Around line 896-923: Best-effort identity resolution currently calls
getUserInfo on every session creation which adds an extra Slack round-trip; to
reduce latency, first try reading a cached profile from KV (keyed by userId) and
only call getUserInfo on a cache miss, then store the resolved profile
(display_name, real_name, name, email) into KV with a ~1-hour TTL; preserve the
existing fallback logic for displayName and email and the try/catch behavior
(i.e., if KV read or getUserInfo fails, continue with undefined
displayName/email), and reference getUserInfo, displayName, email, and
createSession so the cache read/write wraps the current logic before
createSession is invoked (inside the waitUntil background flow).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc6375f2-ac1a-4d31-9d10-38cda5fd16ab
📒 Files selected for processing (5)
packages/github-bot/src/handlers.tspackages/github-bot/test/handlers.test.tspackages/linear-bot/src/webhook-handler.tspackages/slack-bot/src/index.test.tspackages/slack-bot/src/index.ts
Address review feedback: - Merge two `if (appUserId)` blocks in Linear bot handleNewSession - Add negative test for Slack getUserInfo failure (best-effort path)
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
1 participant
Summary
Phase 4 of the unified user model migration (#523). Wires all three bots to send identity fields when creating sessions, enabling the control plane (deployed in Phase 2, PR #550) to resolve canonical user IDs via
resolveOrCreateUser.scmUserId: String(sender.id)alongside existingscmLoginin all 4 handlers (review requested, PR opened, issue comment, review comment)getUserInfoto resolve display name + email, forwardsactorUserId,actorDisplayName,actorEmailto session creationfetchUser(added in pre-step 7) to resolve display name + email, forwardsactorUserId,actorDisplayName,actorEmailto session creationscmUserId: user.id— no changes needed (Phase 2 wired this)All identity resolution is best-effort:
getUserInfo/fetchUserfailures are swallowed, and the session is created with whatever fields are available. The control plane'sresolveProviderIdentityhandles missing fields by returningnull(session getsuser_id = NULL).Test plan
scmUserIdmatchessender.idactorUserId,actorDisplayName,actorEmailforwarded fromgetUserInfofetchUseralready tested (4 tests inlinear-client.test.ts)Summary by CodeRabbit
New Features
Tests