fix(deploy): detect workspace-scoped integration fallback#157
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdated integration connectivity resolution to support scope-aware checks with fallback logic. The ChangesIntegration connectivity with scope fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism to the workspace scope for legacy default personas when checking or establishing integration connections. It refactors status checks and polling logic in connect.ts to utilize helper functions and fall back to the workspace scope if the initial deployer_user scope is not ready. Corresponding unit tests have been added to verify this fallback behavior. Feedback was provided to optimize the polling loop by resolving the workspace token once per iteration instead of multiple times.
| const status = await fetchIntegrationStatusForScope({ | ||
| fetchImpl, | ||
| statusUrl.toString(), | ||
| await resolveWorkspaceToken(opts.workspaceToken) | ||
| ); | ||
| if (isConnectedStatus(status)) { | ||
| const connectionId = readString(status, 'connectionId') | ||
| ?? readString(status, 'currentConnectionId') | ||
| apiUrl, | ||
| token: await resolveWorkspaceToken(opts.workspaceToken), | ||
| workspaceId, | ||
| provider, | ||
| source: effectiveSource, | ||
| ...(sessionId ? { connectionId: sessionId } : {}), | ||
| io | ||
| }); | ||
| if (statusIsConnectedForSource(status, provider, effectiveSource)) { | ||
| const connectionId = readConnectionId(status) | ||
| ?? sessionId | ||
| ?? provider; | ||
| io?.info(`${provider} connected.`); | ||
| return { connectionId }; | ||
| } | ||
|
|
||
| const fallbackSource = workspaceFallbackSource(effectiveSource); | ||
| if (fallbackSource) { | ||
| const fallbackStatus = await fetchIntegrationStatusForScope({ | ||
| fetchImpl, | ||
| apiUrl, | ||
| token: await resolveWorkspaceToken(opts.workspaceToken), | ||
| workspaceId, | ||
| provider, | ||
| source: fallbackSource, | ||
| io | ||
| }); |
There was a problem hiding this comment.
In the polling loop, resolveWorkspaceToken(opts.workspaceToken) is called twice per iteration (once for the primary status check and once for the fallback status check). Since resolving the workspace token can be an expensive asynchronous operation (e.g., performing network requests or I/O to refresh/retrieve credentials), it should be resolved once at the start of each loop iteration and reused.
const token = await resolveWorkspaceToken(opts.workspaceToken);
const status = await fetchIntegrationStatusForScope({
fetchImpl,
apiUrl,
token,
workspaceId,
provider,
source: effectiveSource,
...(sessionId ? { connectionId: sessionId } : {}),
io
});
if (statusIsConnectedForSource(status, provider, effectiveSource)) {
const connectionId = readConnectionId(status)
?? sessionId
?? provider;
io?.info(provider + " connected.");
return { connectionId };
}
const fallbackSource = workspaceFallbackSource(effectiveSource);
if (fallbackSource) {
const fallbackStatus = await fetchIntegrationStatusForScope({
fetchImpl,
apiUrl,
token,
workspaceId,
provider,
source: fallbackSource,
io
});
|
Cross-reference: sibling workspace-identity work across the stack. This bug is the third surface of a workspace-identity-resolution gap that's been chased today. All three are independent bugs but share the same root pattern: lookup keys / scope filters miss legitimate matches because the resolver expects an exact shape it doesn't always get.
The pattern is: a resolver that should fall back semantically instead enforces exact-key matching, so legitimate state (Slack connected, GitHub scope authorized, etc.) reads as "missing" to the caller. Worth knowing for future workspace-identity-resolver work — if anyone reports a similar "X is connected but doesn't show as connected" or "I have permission but I'm getting 401/403" → check whether the lookup path has the same exact-vs-semantic-match gap. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/deploy/src/connect.ts`:
- Around line 250-267: The fallback poll is currently unscoped and can pick up a
different ready row for the same provider; update the fallback logic in
connect() so the workspace-scope poll is pinned to the original row by passing
and enforcing the same discriminator used by the primary poll (e.g.,
expectedConfigKey or the original connectionId/sessionId) into
fetchIntegrationStatusForScope and in the subsequent check. Concretely: have
connect() carry the expectedConfigKey (from preflight) or the original sessionId
into the call to fetchIntegrationStatusForScope, then change the success check
around statusIsConnectedForSource and readConnectionId to verify the returned
status's configKey/connectionId matches the expectedConfigKey/sessionId before
returning { connectionId } so we never resolve against a different integration
row.
- Around line 882-888: The helper workspaceFallbackSource currently broadens any
deployer_user source to a workspace fallback; change it to only do that when the
source was implicit/default by adding a flag (e.g., wasImplicit: boolean) or
similar sentinel parameter and return { kind: 'workspace' } only if source.kind
=== 'deployer_user' && wasImplicit; update callers that previously normalized
omitted/explicit sources to pass true when the caller derived a default/implicit
deployer_user and false for explicitly authored sources so explicit user-scoped
integrations are not widened to workspace scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e8a8b05-7eaa-4e27-9591-c6d200c020a3
📒 Files selected for processing (2)
packages/deploy/src/connect.test.tspackages/deploy/src/connect.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/deploy/src/connect.ts">
<violation number="1" location="packages/deploy/src/connect.ts:235">
P2: `resolveWorkspaceToken` is awaited twice per poll iteration—once for the primary status check and again for the fallback. If the resolver performs I/O (e.g., a token refresh), this is wasteful and may hit rate limits. Resolve it once at the top of the loop and reuse the value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const connectionId = readString(status, 'connectionId') | ||
| ?? readString(status, 'currentConnectionId') | ||
| apiUrl, | ||
| token: await resolveWorkspaceToken(opts.workspaceToken), |
There was a problem hiding this comment.
P2: resolveWorkspaceToken is awaited twice per poll iteration—once for the primary status check and again for the fallback. If the resolver performs I/O (e.g., a token refresh), this is wasteful and may hit rate limits. Resolve it once at the top of the loop and reuse the value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/deploy/src/connect.ts, line 235:
<comment>`resolveWorkspaceToken` is awaited twice per poll iteration—once for the primary status check and again for the fallback. If the resolver performs I/O (e.g., a token refresh), this is wasteful and may hit rate limits. Resolve it once at the top of the loop and reuse the value.</comment>
<file context>
@@ -217,31 +229,43 @@ export function relayfileIntegrationResolver(opts: {
- const connectionId = readString(status, 'connectionId')
- ?? readString(status, 'currentConnectionId')
+ apiUrl,
+ token: await resolveWorkspaceToken(opts.workspaceToken),
+ workspaceId,
+ provider,
</file context>
|
Pushed review-feedback patch
Verification in clean #157 worktree:
Holding merge pending CI and reviewer re-check; no self-merge while these comments are still pending. |
1 participant
Summary
workspaceandworkspace_service_accountsource checks scoped to their exact tableThis addresses the typed persona deploy symptom where
slack: {}missed an already-connected workspace Slack row and then hung after opening the Nango connect URL. Picker prompts are expected to proceed once Slack is counted connected becauseconnectedIntegrationsnow includes the provider.Verification
corepack pnpm --filter @agentworkforce/persona-kit buildcorepack pnpm --filter @agentworkforce/runtime buildcorepack pnpm --filter @agentworkforce/deploy test -- connect.test.ts(142/142 passing)