DO-NOT-MERGE: Split #132 cloud workspace key work#146
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 implements workspace key propagation from local brokers to cloud sandboxes to ensure they share a single relay workspace, adding a tripwire to detect stale sandbox brokers. Key feedback points out that workspaceKeyForProject should await revivePromises to avoid returning undefined during session revivals, and classifyWorkspaceJoinFailure should robustly extract messages from plain objects to handle serialized IPC errors. Additionally, request coalescing for spawnPersona should be restored using a dedicated map, and defensive optional chaining should be used on spawned agent results to prevent potential runtime TypeError crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async workspaceKeyForProject(projectId: string): Promise<string | undefined> { | ||
| const normalizedProjectId = projectId.trim() | ||
| if (!normalizedProjectId) return undefined | ||
| const startPromise = this.startPromises.get(normalizedProjectId) | ||
| if (startPromise) await startPromise.catch(() => undefined) | ||
| const session = this.sessions.get(normalizedProjectId) | ||
| if (!session) return undefined | ||
| try { | ||
| const metadata = await session.client.getSession() | ||
| return metadata.workspace_key || undefined | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
In workspaceKeyForProject, only startPromises is awaited. However, if a local broker session is currently being revived (i.e., there is an active promise in revivePromises), this.sessions.get(normalizedProjectId) will return undefined because reviveSession drops the session immediately before restarting it.
This causes workspaceKeyForProject to prematurely return undefined instead of waiting for the revive to complete. Consequently, any concurrent cloud agent provisioning/warming will fail to obtain the local workspace key and will fallback to creating an isolated, keyless workspace.
We should await revivePromises just like we do in getOrAwaitSessionsForProject.
async workspaceKeyForProject(projectId: string): Promise<string | undefined> {
const normalizedProjectId = projectId.trim()
if (!normalizedProjectId) return undefined
const revivePromise = this.revivePromises.get(normalizedProjectId)
if (revivePromise) await revivePromise.catch(() => undefined)
const startPromise = this.startPromises.get(normalizedProjectId)
if (startPromise) await startPromise.catch(() => undefined)
const session = this.sessions.get(normalizedProjectId)
if (!session) return undefined
try {
const metadata = await session.client.getSession()
return metadata.workspace_key || undefined
} catch {
return undefined
}
}| export function classifyWorkspaceJoinFailure(err: unknown): 'rejected' | 'rate-limited' | null { | ||
| const message = toErrorMessage(err) | ||
| if (/explicit workspace key .* was rate-limited/iu.test(message)) return 'rate-limited' | ||
| if (/explicit workspace key .* was rejected/iu.test(message)) return 'rejected' | ||
| return null | ||
| } |
There was a problem hiding this comment.
In Electron applications, errors passed across IPC or serialized/deserialized boundaries often lose their prototype and are no longer instances of Error, but plain objects containing a message property. Since toErrorMessage only extracts .message from instanceof Error and falls back to String(err) (which yields "[object Object]" for plain objects), the regex checks in classifyWorkspaceJoinFailure will fail for these serialized errors.
We should robustly extract the message property from plain objects if it exists.
export function classifyWorkspaceJoinFailure(err: unknown): 'rejected' | 'rate-limited' | null {
const message = err && typeof err === 'object' && 'message' in err && typeof err.message === 'string'
? err.message
: toErrorMessage(err)
if (/explicit workspace key .* was rate-limited/iu.test(message)) return 'rate-limited'
if (/explicit workspace key .* was rejected/iu.test(message)) return 'rejected'
return null
}| private startPromises = new Map<string, Promise<boolean | void>>() | ||
| private revivePromises = new Map<string, Promise<boolean>>() | ||
| private inFlightSpawnRequests = new Map<string, Promise<BrokerSpawnResult>>() | ||
| private inFlightSpawnRequests = new Map<string, Promise<{ name: string; runtime: string }>>() |
There was a problem hiding this comment.
To support restoring the concurrent request coalescing/deduplication for spawnPersona (which was removed in this PR), we should declare a separate map specifically for in-flight persona spawn requests. This keeps the types clean and prevents duplicate concurrent spawns of the same persona.
private inFlightSpawnRequests = new Map<string, Promise<{ name: string; runtime: string }>>()
private inFlightPersonaSpawnRequests = new Map<string, Promise<{ name: string; runtime: string; cli?: string }>>()| const spawned = await session.client.spawnPty(nextInput) | ||
| const safeSpawned = normalizeSpawnPtyResult(spawned, nextInput.name) | ||
| const spawnedName = safeSpawned.name | ||
| const spawnedName = spawned.name || nextInput.name |
There was a problem hiding this comment.
Since the robust normalizeSpawnPtyResult helper was removed in this PR, we are now directly accessing spawned.name. If the broker returns an unexpected payload or if spawned is null/undefined at runtime, this will throw a TypeError and crash the spawn flow.
Using optional chaining spawned?.name is a safer, more defensive approach.
const spawnedName = spawned?.name || nextInput.name| const safeSpawned = normalizeSpawnPtyResult(spawned, nextInput.name, input.resolvedHarness) | ||
| this.rememberAgentSession(safeSpawned.name, sessionKeyFor(session)) | ||
| return safeSpawned | ||
| this.rememberAgentSession(spawned.name || nextInput.name, sessionKeyFor(session)) |
There was a problem hiding this comment.
Similar to spawnAgentOnce, we should use optional chaining spawned?.name here to defensively guard against potential null/undefined values or unexpected payloads returned by the broker.
this.rememberAgentSession(spawned?.name || nextInput.name, sessionKeyFor(session))
|
Reviewed PR #146 from I restored clone-safe broker spawn results and persona spawn coalescing in broker.ts. The PR had removed normalization, which could return raw broker objects with functions across IPC. I also restored regression coverage in broker.test.ts, including duplicate persona spawn coalescing. Validation run:
I also tried |
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #146 and fixed a validated regression in the changed broker spawn path. Changes made:
Verification run:
All passed locally. I did not verify remote CI status or GitHub mergeability from here. |
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #146 and fixed two validated issues in the current checkout:
Added regression coverage in src/main/broker.test.ts for mismatched pinned connection reuse, and extended the existing explicit-key test to assert the key is not logged in full. Verification run:
Build passed with existing Vite dynamic/static import chunk warnings. I’m not printing |
1 participant
DO-NOT-MERGE: merge hold remains in force; this PR is opened for review only.
Split from #132 /
fix/slack-integration-event-context-retry.Summary:
Restart-branch decision note:
Verification:
npx --yes vitest run src/main/broker.test.ts src/main/cloud-agent.test.ts(57/57)npm run buildCross-link: #132