fix(kiloclaw): harden Composio onboarding#3400
Conversation
pandemicsyn
changed the title
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe lock-free identity reservation and popup state machine are well-designed; the main concern is a behavioral gap in Overview
Issue Details (click to expand)WARNING
SUGGESTION
DetailsWARNING 1 — In if (params.instance && !params.sandboxHasComposioSecrets) {
return { enabled: true, status: 'disconnected', ... };
}This guard fires when a sandbox is provisioned ( WARNING 2 — In function isRetryablePatchSecretsError(error: unknown): boolean {
if (error instanceof KiloClawApiError) return error.statusCode >= 500;
return true; // covers network errors, TypeErrors, etc.
}This is the intended design (retry transport errors), and the existing tests cover SUGGESTION — In Files Reviewed (14 files)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-4.6 · 1,843,512 tokens Review guidance: REVIEW.md from base branch |
| return { enabled: true, status: 'error', connectedAccountId: null, sandboxConfigSource }; | ||
| } | ||
|
|
||
| if (params.instance && !params.sandboxHasComposioSecrets) { |
There was a problem hiding this comment.
Moderate, Functional: With the 15s refetchInterval removed from getComposioOnboardingStatus, the awaiting-confirmation state in useComposioPopup relies on a single refetchStatus() at callback time plus focus/visibilitychange events. This guard reports disconnected whenever a provisioned, managed sandbox has not yet reported sandboxHasComposioSecrets. If that flag is not true on the one post-callback refetch and the user never blurs and refocuses the tab, the 45s confirmation timer expires and surfaces "Could not confirm Google Calendar" even though the connection succeeded.
The callback route writes patchSecrets, then the connected-account id, then composio_config_source='managed' before returning success, so the only remaining variable is whether sandboxHasComposioSecrets flips to true synchronously once the worker patchSecrets call returns. If the worker config is read through any cache or eventually-consistent path, please either keep a short poll while the popup is in awaiting-confirmation or have the state machine refetch on its own interval during that state.
|
|
||
| return { | ||
| enabled: true, | ||
| status: 'connected', |
There was a problem hiding this comment.
Low, Functional: getManagedComposioGoogleCalendarStatus no longer calls listComposioConnectedAccounts, so connected is now derived purely from durable state (composio_config_source, the stored google_calendar_connected_account_id, and sandboxHasComposioSecrets). This is reasonable and removes a network call from a hot path, but it means a Google Calendar connection revoked on Google's side keeps showing "Connected" in this surface. If revocation is expected to be handled elsewhere (worker, reconnect prompt), this is fine as scoped. Flagging it so the tradeoff is intentional rather than incidental.
| const instance = organizationId | ||
| ? await getActiveOrgInstance(user.id, organizationId) | ||
| : await getActiveInstance(user.id); | ||
| const verified = await completeManagedComposioGoogleCalendarConnection({ |
There was a problem hiding this comment.
Low, Functional: Dropping withKiloclawProvisionContextLock here means the getComposioInstanceConfigSource(...) === 'manual' check inside completeManagedComposioGoogleCalendarConnection and its subsequent patchSecrets plus composio_config_source='managed' write are no longer atomic against provision/updateConfig, which still take that lock. The manual patchSecrets save path never took this lock even before, so the manual-versus-managed race is pre-existing and the durable source marker is the intended arbiter. Not a regression introduced here, just noting the lock no longer provides any mutual exclusion for this path.
| return body.slice(0, 500); | ||
| } | ||
|
|
||
| function isRetryablePatchSecretsError(error: unknown): boolean { |
There was a problem hiding this comment.
Low, Testing: isRetryablePatchSecretsError returns true for any non-KiloClawApiError, and logTerminalManagedComposioPatchSecretsError has a distinct status: 'transport_error' branch. The tests cover the KiloClawApiError 5xx-retry and 4xx-terminal paths but not a plain Error/TypeError (fetch abort, DNS failure). Add a test that rejects patchSecrets with a plain Error and asserts the retry budget is consumed and the log records status: 'transport_error', so a regression in that branch does not ship silently.
2 participants
Summary
ACTIVE, but treat workerpatchSecretsfailures as real infra failures with bounded retry and no connected-account DB write on failure.useComposioPopupstate machine that preservespostMessage,localStorage, andBroadcastChannelcallback channels, including late callback delivery after popup-close races.Verification
N/A
Visual Changes
ClawOnboardingFlow.useComposioPopup.Reviewer Notes
google_calendar_connected_account_idis only persisted after worker secrets are patched for an existing sandbox; status also verifies the current sandbox source/secrets before reporting connected.popup_closedobservation and reads the stored callback result on focus/visibility to recover missed browser events.