Primitives and channels swarm

[khaliqgant]

• SDK improvements
• CLI auth improvements
• Tests in advance of feat: Add GitHub Copilot CLI integration #313

---

In general, the fix is to replace uses of Math.random() in packages/utils/src/name-generator.ts with a cryptographically secure random source. Since this code runs in Node.js (CLI and bridge), we should use crypto.randomInt (or crypto.randomBytes with proper conversion) from Node’s built-in crypto module, which is designed for secure randomness. This avoids predictability while keeping the existing behavior (uniform random choice from arrays and a numeric suffix) intact.

Concretely, in packages/utils/src/name-generator.ts:

1. Add an import from Node’s crypto module, e.g. import { randomInt } from 'node:crypto';.
2. Replace Math.floor(Math.random() * ADJECTIVES.length) with randomInt(ADJECTIVES.length) and similarly for NOUNS.length. randomInt(max) returns a uniform integer in [0, max), matching the original intent without bias.
3. In generateUniqueAgentName, replace the fallback’s Math.floor(Math.random() * 1000) with randomInt(1000) so the suffix comes from the same secure source.

No changes are needed in src/cli/index.ts, packages/config/src/shadow-config.ts, or packages/bridge/src/spawner.ts, because once the source of randomness is secure, all flows from those generated names become secure as well.

---

In general, to fix insecure randomness you must replace Math.random() with a cryptographically secure random generator wherever the value can influence security-sensitive behavior. In Node.js this means using the crypto module (for example, crypto.randomInt or crypto.webcrypto.getRandomValues) instead of Math.random.

For this codebase, all taint originates from generateAgentName and the fallback in generateUniqueAgentName in packages/utils/src/name-generator.ts. The best targeted fix is to change those two uses of Math.random() to use crypto.randomInt (for index selection and integer suffix) while keeping the public API and behavior identical from the caller’s perspective. This does not require modifications in src/cli/index.ts, packages/config/src/shadow-config.ts, or packages/bridge/src/spawner.ts, because once the randomness is secure at the source, the entire taint path becomes safe.

Concretely:

• In packages/utils/src/name-generator.ts, import Node’s crypto module.
• Replace Math.floor(Math.random() * ADJECTIVES.length) with crypto.randomInt(ADJECTIVES.length).
• Replace Math.floor(Math.random() * NOUNS.length) with crypto.randomInt(NOUNS.length).
• Replace the fallback Math.floor(Math.random() * 1000) with crypto.randomInt(1000).

No other files need code changes for this particular issue.

General fix: Replace all uses of Math.random() in the name‑generation utilities with a cryptographically secure random source. In Node.js, the standard approach is to use the crypto module (randomInt for indices and simple bounded integers, or randomBytes and then derive indices).

Best concrete fix here:

• In packages/utils/src/name-generator.ts, add an import of Node’s crypto module.
• Change the random index selection for ADJECTIVES and NOUNS from Math.floor(Math.random() * array.length) to crypto.randomInt(array.length), which yields a uniform integer in [0, length).
• In the generateUniqueAgentName fallback, replace Math.floor(Math.random() * 1000) with crypto.randomInt(1000).
• No other files (src/cli/index.ts, packages/bridge/src/spawner.ts) need code changes; the taintedness will now originate from a CSPRNG instead of Math.random, resolving all alert variants along this path.
• This does not alter the public API or general behavior (still picks a random adjective, noun, and numeric suffix; just does so securely).

Concrete changes:

• File: packages/utils/src/name-generator.ts
  • Add import crypto from 'node:crypto'; at the top (keeping existing style).
  • Update line 32 to const adjective = ADJECTIVES[crypto.randomInt(ADJECTIVES.length)];
  • Update line 33 to const noun = NOUNS[crypto.randomInt(NOUNS.length)];
  • Update line 48 to return \${generateAgentName()}${crypto.randomInt(1000)}`;`

No changes are required in packages/bridge/src/spawner.ts or src/cli/index.ts per the constraint to only touch shown snippets and because fixing the source of randomness is sufficient.

---

[devin-ai-integration]

Devin Review found 1 potential issue.

🔴 1 issue in files not directly in the diff

🔴 BaseWrapper permanently dedupes relay commands even when send fails (channel path never checks success) (packages/wrapper/src/base-wrapper.ts:453-492)

BaseWrapper.sendRelayCommand() adds the message hash to sentMessageHashes before attempting to send, and the channel path doesn’t check the return value of sendChannelMessage().

Actual: if the client is READY but the underlying send fails (e.g., transient socket issue), the hash remains in sentMessageHashes, so subsequent retries of the same command are treated as duplicates and are skipped.

Expected: only mark as “sent” after the send succeeds, or remove the hash on failure.

Impact: message loss (especially for channel replies, since that branch never checks success), which is user-visible and can break workflows.

Click to expand

In BaseWrapper.sendRelayCommand():

• Hash is recorded immediately:

  const hash = `${cmd.to}:${cmd.body.substring(0, 100)}`;
  ...
  this.sentMessageHashes.add(hash);

  packages/wrapper/src/base-wrapper.ts:453-465

• Channel messages are sent without checking the return value:

  this.client.sendChannelMessage(cmd.to, cmd.body, { ... });

  packages/wrapper/src/base-wrapper.ts:484-492

So a failed send still “poisons” dedupe and prevents future sends.

Recommendation: Only add hash to sentMessageHashes after a successful send. For the channel branch, capture and validate the boolean return of sendChannelMessage() (and similarly handle the send result for sendAndWait()/sendMessage()), removing the hash on failure.

View issue and 9 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 2 new potential issues.

🔴 1 issue in files not directly in the diff

🔴 BaseWrapper permanently dedupes relay commands even when send fails (channel path never checks success) (packages/wrapper/src/base-wrapper.ts:453-492)

BaseWrapper.sendRelayCommand() adds the message hash to sentMessageHashes before attempting to send, and the channel path doesn’t check the return value of sendChannelMessage().

Actual: if the client is READY but the underlying send fails (e.g., transient socket issue), the hash remains in sentMessageHashes, so subsequent retries of the same command are treated as duplicates and are skipped.

Expected: only mark as “sent” after the send succeeds, or remove the hash on failure.

Impact: message loss (especially for channel replies, since that branch never checks success), which is user-visible and can break workflows.

Click to expand

In BaseWrapper.sendRelayCommand():

• Hash is recorded immediately:

  const hash = `${cmd.to}:${cmd.body.substring(0, 100)}`;
  ...
  this.sentMessageHashes.add(hash);

  packages/wrapper/src/base-wrapper.ts:453-465

• Channel messages are sent without checking the return value:

  this.client.sendChannelMessage(cmd.to, cmd.body, { ... });

  packages/wrapper/src/base-wrapper.ts:484-492

So a failed send still “poisons” dedupe and prevents future sends.

Recommendation: Only add hash to sentMessageHashes after a successful send. For the channel branch, capture and validate the boolean return of sendChannelMessage() (and similarly handle the send result for sendAndWait()/sendMessage()), removing the hash on failure.

View issues and 15 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 4 new potential issues.

View issues and 27 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 33 additional flags in Devin Review.

[devin-ai-integration]

🟡 SDK query/spawn/release promises are not rejected when daemon responds with ERROR (request ID not correlated)

When the daemon rejects a request (e.g., spawnManager disabled, invalid query, etc.), it sends an ERROR envelope using a new id (not the request’s id) (packages/daemon/src/server.ts:1560-1576). The SDK’s RelayClient only resolves pending operations when it receives a typed *_RESPONSE / *_RESULT with an ID it recognizes (handleQueryResponse uses envelope.id; spawn/release use payload.replyTo) (packages/sdk/src/client.ts:1196-1228).

Actual: the SDK logs the error but leaves pendingQueries / pendingSpawns / pendingReleases unresolved until the timeout fires, surfacing a misleading timeout error.

Expected: the SDK should reject the corresponding pending promise immediately when the daemon indicates an error for that operation.

Click to expand

Example trigger

• Daemon configured without spawnManager.
• Client calls await client.spawn(...).
• Daemon executes sendErrorEnvelope(connection, 'SpawnManager not enabled...') and returns (packages/daemon/src/server.ts:1259-1264).
• SDK receives ERROR and only logs it in handleErrorFrame, without rejecting the pending spawn (packages/sdk/src/client.ts:1240-1256).
• client.spawn() rejects after 30s with Spawn timeout... (misleading).

Root cause

• sendErrorEnvelope generates a fresh envelope ID (generateId()) and does not include replyTo/correlation fields (packages/daemon/src/server.ts:1560-1576).
• RelayClient does not attempt to correlate ERROR to pending operations (packages/sdk/src/client.ts:1240-1256).

(Refers to lines 859-1265)

Recommendation: Implement error correlation and fail-fast behavior:

• Preferably update daemon to send ERROR with id: originalRequest.id (or add replyTo/correlationId in ErrorPayload).
• In the SDK, on receiving ERROR, reject the matching pending query/spawn/release when correlation is available; otherwise consider rejecting all pending queries (or expose the daemon error to callers).

---

Was this helpful? React with 👍 or 👎 to provide feedback.