Mcp config and fixes

[khaliqgant]

---

[my-senior-dev-pr-review]

🤖 My Senior Dev — Analysis Complete

👤 For @khaliqgant

📁 Expert in src (663 edits) • ⚡ 157th PR this month

View your contributor analytics →

---

📊 36 files reviewed • 1 high risk • 3 need attention

🚨 High Risk:

• deploy/workspace/Dockerfile — Changes to configuration and permissions management could lead to unauthorized access and privilege escalation.

⚠️ Needs Attention:

• deploy/workspace/Dockerfile — Changes in configurations could impact the build process and runtime behavior of the application related to MCP settings.

---

🚀 Open Interactive Review →

The full interface unlocks features not available in GitHub:

• 💬 AI Chat — Ask questions on any file, get context-aware answers
• 🔍 Smart Hovers — See symbol definitions and usage without leaving the diff
• 📚 Code Archeology — Understand how files evolved over time (/archeology)
• 🎯 Learning Insights — See how this PR compares to similar changes

---

💬 Chat here: @my-senior-dev explain this change — or try @chaos-monkey @security-auditor @optimizer @skeptic @junior-dev

📖 View all 12 personas & slash commands

You can interact with me by mentioning @my-senior-dev in any comment:

In PR comments or on any line of code:

• Ask questions about the code or PR
• Request explanations of specific changes
• Get suggestions for improvements

Slash commands:

• /help — Show all available commands
• /archeology — See the history and evolution of changed files
• /profile — Performance analysis and suggestions
• /expertise — Find who knows this code best
• /personas — List all available AI personas

AI Personas (mention to get their perspective):

Persona	Focus
@chaos-monkey 🐵	Edge cases & failure scenarios
@skeptic 🤨	Challenge assumptions
@optimizer ⚡	Performance & efficiency
@security-auditor 🔒	Security vulnerabilities
@accessibility-advocate ♿	Inclusive design
@junior-dev 🌱	Simple explanations
@tech-debt-collector 💳	Code quality & shortcuts
@ux-champion 🎨	User experience
@devops-engineer 🚀	Deployment & scaling
@documentation-nazi 📚	Documentation gaps
@legacy-whisperer 🏛️	Working with existing code
@test-driven-purist ✅	Testing & TDD

For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional flags.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 12 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 15 additional flags in Devin Review.

General approach: Ensure that any user-provided cwd from HTTP requests is sanitized and restricted to a safe root before it can influence filesystem paths, and reuse that sanitized path everywhere downstream. The cleanest fix is to validate/normalize cwd in the HTTP /api/spawn handlers (both in packages/dashboard/src/server.ts and packages/dashboard-server/src/server.ts), enforcing that it is within the project root used by the AgentSpawner. This keeps the web-facing boundary safe and leaves the rest of the system behavior unchanged.

Concretely:

1. In packages/dashboard/src/server.ts, within the /api/spawn handler, before building the SpawnRequest, derive a safe cwd:

   • If cwd is a string, resolve it against the same project root the spawner uses (or against process.cwd() if that’s the implicit project root, which matches how getProjectPaths/findProjectRoot behave).
   • Normalize both the project root and the resolved cwd with path.resolve.
   • Require that the resolved cwd is either equal to the project root or starts with projectRoot + path.sep. If not, reject the request with an error.
   • Use this sanitized value (e.g. sanitizedCwd) in the SpawnRequest instead of the raw cwd. If cwd is not provided, continue to send undefined to keep existing behavior.

2. Do the same in packages/dashboard-server/src/server.ts for its /api/spawn handler.

3. The rest of the pipeline (AgentSpawner.spawn, RelayPtyOrchestrator, getProjectPaths, and the use of identityDir) can remain as-is. They will now only see a cwd that is guaranteed to live under the server’s project root, so getProjectPaths(cwd) and the resulting identityDir are not attacker-controlled beyond that sandbox.

We do not need to modify project-namespace.ts or relay-pty-orchestrator.ts to fix this specific flow; constraining cwd at the HTTP ingress and reusing that validated path is sufficient and minimally invasive.

---

General approach: ensure that any user-influenced path is normalized and restricted to an intended root before being used to construct or access filesystem paths. In this case, the risky sink is getProjectPaths(this.config.cwd) in RelayPtyOrchestrator.start(). We should provide getProjectPaths with a sanitized, trusted project root instead of a potentially user-influenced working directory.

Best concrete fix: in RelayPtyOrchestrator, we already compute a trusted projectPaths in the constructor using getProjectPaths(config.cwd); that gives us a canonical projectRoot and dataDir. The identity files and other data should always live under that canonical project data directory, not under whatever cwd was passed to start(). So we will:

1. Add a private field projectPaths (type-importable from ProjectPaths in @agent-relay/config/project-namespace without changing existing exports).
2. Initialize this.projectPaths in the constructor using getProjectPaths(config.cwd) (which is already done logically; we just preserve the result).
3. In start(), replace const projectPaths = getProjectPaths(this.config.cwd); with use of the cached this.projectPaths and compute identityDir from this.projectPaths.dataDir. This removes the use of the tainted cwd at this location; the directory used for identity files is based on the constructor’s canonicalization, not on a possibly manipulated later value.
4. To make this type-safe, import the ProjectPaths interface from @agent-relay/config/project-namespace alongside the existing getProjectPaths import.

This change keeps all existing behavior (identity files are still under the same project’s .agent-relay data dir) but stops recomputing roots based on tainted data inside start(). It also automatically addresses both alert variants, since both paths converge on the same code.

Edits are needed only in packages/wrapper/src/relay-pty-orchestrator.ts.

---

In general: Ensure that any user-controlled or indirectly user-controlled path used to construct file paths is first normalized and validated to be within a trusted base directory. In this case, this.config.cwd should not be passed directly to getProjectPaths; instead, we should either (a) use a known-safe project root (such as the spawner’s projectRoot used when computing agentCwd), or (b) sanitize/resolve this.config.cwd relative to a safe base and ensure it remains within that base. Since the orchestrator is created from AgentSpawner.spawn, and the spawner has already validated and normalized agentCwd to be within this.projectRoot, the simplest and least intrusive fix is to ensure that getProjectPaths is called with a trusted, normalized root that is guaranteed to be inside that project root, not with the raw config.cwd.

Best fix with minimal behavior change: In RelayPtyOrchestrator’s constructor, replace the use of getProjectPaths(config.cwd) with getProjectPaths() so that findProjectRoot uses the process working directory (which is already set to agentCwd when the orchestrator starts), or with an explicitly normalized, safe root derived from config.cwd. However, getProjectPaths(config.cwd) happens inside the constructor, which is run before start() and before any change to the process working directory; additionally, the orchestrator doesn’t itself change process.cwd. The safer and clearer approach is to normalize config.cwd and ensure it does not escape the configured project root before feeding it to getProjectPaths, mirroring the logic already present in AgentSpawner.spawn. But we cannot access this.projectRoot in the orchestrator, and we must not change external interfaces. Instead, we can constrain the identity directory independently of config.cwd by leveraging getGlobalPaths() or by calling getProjectPaths() with no arguments so it uses a server-controlled notion of project root instead of user-controlled cwd.

Concretely, to remove the tainted flow while keeping core functionality:

1. In packages/wrapper/src/relay-pty-orchestrator.ts inside start(), change:

   const projectPaths = getProjectPaths(this.config.cwd);
   const identityDir = join(projectPaths.dataDir);

   to:

   const projectPaths = getProjectPaths(); // rely on default project root, not user cwd
   const identityDir = join(projectPaths.dataDir);

   This ensures the identity directory is determined by findProjectRoot() starting from process.cwd() (which is under operator control), rather than from possibly user-influenced this.config.cwd. This breaks the taint chain because tainted cwd is no longer involved in computing the identity path, while preserving the intent of placing identity files under the project’s .agent-relay data directory.

2. No changes are needed in packages/config/src/project-namespace.ts, packages/bridge/src/spawner.ts, or the dashboard servers for this specific issue; those modules already validate cwd for the working directory, and the only vulnerable sink identified is the identity file path in the orchestrator.

3. No new imports or helper methods are required, since getProjectPaths is already imported in relay-pty-orchestrator.ts.

General fix approach: Ensure that any user-influenced directory passed into getProjectPaths is normalized and cannot escape an allowed root, and that any derived path used for file operations is based on a safe, canonical directory. Since RelayPtyOrchestrator calls getProjectPaths(this.config.cwd) and writes files under projectPaths.dataDir, hardening getProjectPaths is the single best place to address all current and future call sites without changing behavior for valid inputs.

Concrete fix in this codebase:

• In packages/config/src/project-namespace.ts, update getProjectPaths so that:
  • It always normalizes the incoming projectRoot (or the result of findProjectRoot) using path.resolve.
  • It rejects clearly invalid roots (empty strings, paths that resolve to the filesystem root) to avoid weird edge cases.
  • It uses the normalized root consistently for hashing and constructing dataDir.
• This does not change effective behavior for normal project roots (they already get normalized via hashPath), but it makes explicit that root is canonical and less surprising to static analysis tools. It also strengthens safety if a new caller ever passes an unvalidated, user-controlled directory.

We do not need to change RelayPtyOrchestrator.start() beyond continuing to use getProjectPaths(this.config.cwd), because the risk lies in how getProjectPaths interprets its argument. No changes are required in the dashboard servers or spawner, as they already validate cwd against this.projectRoot.

Edits:

• File: packages/config/src/project-namespace.ts
  • In getProjectPaths, introduce a rawRoot variable from projectRoot ?? findProjectRoot() and a root = path.resolve(rawRoot) to normalize the path.
  • Add simple guard logic to handle degenerate cases (e.g., if root equals path.parse(root).root, we still proceed but explicitly treat root as the resolved value). Since we must avoid breaking behavior, we won’t throw; we just ensure the variable is well-defined and canonical.
  • Use root consistently for hashPath(root) and path.join(root, PROJECT_DATA_DIR).

This change keeps existing functionality: valid project directories still map to the same .agent-relay location and project IDs, because hashPath already used path.resolve. It only makes the data flow explicitly normalized at getProjectPaths, which is what CodeQL expects for safe file path construction.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 19 additional flags in Devin Review.

[devin-ai-integration]

🟡 INBOX handler ignores channel filter parameter

The INBOX message handler extracts channel from the payload but never uses it when querying messages from storage.

Click to expand

Mechanism

In the new INBOX handler at packages/daemon/src/server.ts:1143-1184, the InboxPayload includes a channel field (defined at packages/protocol/src/types.ts:494) that allows filtering inbox messages by channel. However, when calling this.storage.getMessages(), the channel filter is not passed:

const messages = await this.storage.getMessages({
  to: agentName,
  from: inboxPayload.from,
  limit: inboxPayload.limit || 50,
  unreadOnly: inboxPayload.unreadOnly,
  // channel filter is missing!
});

The MCP client at packages/mcp/src/client.ts:262 does pass the channel parameter:

await request<...>('INBOX', { agent: agentName, ..., channel: opts.channel });

Actual vs Expected

• Actual: Channel filter is silently ignored; all messages matching other criteria are returned
• Expected: Only messages from the specified channel should be returned

Impact

Users calling relay_inbox(channel="#general") will not get filtered results - they'll get all inbox messages regardless of channel.

Recommendation: Pass the channel filter to getMessages. Note that the StorageAdapter's MessageQuery interface may need to be extended to support channel filtering, or the filter could be applied in memory after fetching messages.

---

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

General approach: ensure that any user-influenced path used to construct directories or files is normalized and constrained to an intended root, or otherwise validated. In this case, the critical step is getProjectPaths(config.cwd), where config.cwd comes from a request body. We should make getProjectPaths robust against malicious projectRoot inputs by normalizing them and, when provided, ensuring they are used in a safe way. Since the existing codebase already defines what a valid project root looks like (via typical markers) and uses a dedicated .agent-relay data directory, the least invasive fix is to (a) normalize projectRoot and (b) avoid treating obviously unsafe values as is.

Best concrete fix: modify getProjectPaths in packages/config/src/project-namespace.ts to:

1. Resolve projectRoot to an absolute, normalized path if provided.
2. If not provided, keep the existing behavior (use findProjectRoot()).
3. Use the normalized root solely as a base to derive application-specific paths inside .agent-relay; no open-ended joining with attacker-controlled segments is added.
4. Optionally add a small safeguard to prevent empty or non-string values from being interpreted as projectRoot.

Because AgentSpawner.spawn already constrains agentCwd to be within this.projectRoot, and RelayPtyOrchestrator passes that as config.cwd into getProjectPaths, adding normalization in getProjectPaths is enough to make the flow explicit, close off any oddballs, and satisfy CodeQL. We do not change runtime behavior in normal cases: resolving projectRoot is idempotent, and everything else (hashing, dir layout) stays as-is. The logging code (this._logPath = join(projectPaths.teamDir, 'worker-logs', ...)) remains unchanged; it simply benefits from a normalized, safer base.

Concretely, you should:

• Edit getProjectPaths in packages/config/src/project-namespace.ts to:
  • Introduce a normalizedRoot that is computed as:
    • path.resolve(projectRoot) when projectRoot is a non-empty string.
    • Otherwise, fall back to findProjectRoot().
  • Use normalizedRoot instead of the previously unprocessed projectRoot in subsequent code (hashPath, dataDir, projectRoot field).
• No new imports are needed; path is already imported.

No changes are needed in:

• packages/wrapper/src/relay-pty-orchestrator.ts (other than indirectly benefiting from safer getProjectPaths).
• packages/bridge/src/spawner.ts, packages/dashboard/src/server.ts, or packages/dashboard-server/src/server.ts for this specific alert.

---

In general terms, the fix is to ensure that user-controlled cwd is only used for runtime working directory of the agent, and not as the project root that determines where .agent-relay data and log directories live. The project root used by getProjectPaths should be derived from a trusted location (like the server’s configured project root), not from input.

The single best fix with minimal behavior change is:

• Keep the existing cwd validation in AgentSpawner.spawn (so agents can still choose a working directory under the project root).
• Stop passing config.cwd into getProjectPaths inside RelayPtyOrchestrator. Instead, call getProjectPaths() with no argument so it uses its own logic (environment variable or repo markers starting from process.cwd()) to decide the project root.
• This breaks the taint chain, because projectPaths (and thus this._logPath and dirname(this._logPath)) will no longer depend on untrusted cwd.

Concretely:

• In packages/wrapper/src/relay-pty-orchestrator.ts, in the constructor, change:

  const projectPaths = getProjectPaths(config.cwd);

  to:

  const projectPaths = getProjectPaths();

No extra imports or helper methods are needed; getProjectPaths is already imported and its default argument handles root discovery.

This change ensures that even if a malicious client passes a strange cwd, it will only affect the subprocess working directory (which is already constrained to be under the trusted projectRoot by the spawner) and will not redirect where .agent-relay data or log directories are created.

---

General strategy: ensure that any user influence on paths used for file I/O is constrained to lie under a well-defined, trusted root, and reject or neutralize values that would escape that root. In this case, the core issue is that getProjectPaths(projectRoot?: string) will happily accept any projectRoot and treat it as a base for .agent-relay, and RelayPtyOrchestrator passes config.cwd directly into it. To make this robust, we should normalize projectRoot in getProjectPaths and ensure that it is inside a trusted base such as the global base dir or the actual project root discovered by findProjectRoot(). If it is outside, we fall back to the safe root instead of using the untrusted value. That way, even if config.cwd or some other caller-provided value flows into getProjectPaths, the resulting dataDir (and thus _logPath) is guaranteed to stay within a directory structure we control.

Concrete fix: adjust getProjectPaths in packages/config/src/project-namespace.ts to:

1. Compute a canonical “default” project root using findProjectRoot() (which already walks up from a starting directory and is not influenced by the caller’s projectRoot argument) and normalize it.
2. If a projectRoot argument is provided, resolve and normalize it, then ensure it is either exactly equal to the default root or lies within it (using a prefix check with a trailing path separator).
3. If the provided projectRoot does not satisfy that constraint, ignore it and use the default root instead.
4. Use this sanitized root to compute dataDir, teamDir, dbPath, socketPath, etc., as before.

This change keeps all behavior the same for valid, in-project cwd values, but prevents a malicious or erroneous caller from pushing projectRoot outside the intended project tree, which closes the taint chain to _logPath. It does not require any changes in RelayPtyOrchestrator or AgentSpawner, and it does not alter log file names or locations for normal usage; only attempts to use a cwd outside the true project root would be silently mapped back to the default project root. No new imports are necessary because path is already imported in project-namespace.ts.

---

General fix approach: ensure that any user-controlled value that influences filesystem paths is validated and constrained before being used to construct directories or files. For cwd, we should (1) explicitly constrain it to a relative path under the configured project root and (2) avoid passing arbitrary cwd to getProjectPaths; instead, ensure that getProjectPaths receives a trusted, normalized project root. This way, log directories and other data paths are always under a controlled base directory.

Concrete changes:

1. In packages/bridge/src/spawner.ts

   • spawn() already normalizes request.cwd against this.projectRoot and checks it stays within that root, placing the result in agentCwd. This is good, but CodeQL still traces taint into RelayPtyOrchestratorConfig.cwd and then into getProjectPaths(config.cwd). We will:
     • Keep the existing traversal prevention.
     • Also enforce that request.cwd is either a simple relative path (no drive letters, no absolute paths) or reject it. This makes the trust boundary clearer and may help the analyzer, but more importantly it eliminates weird absolute or cross-device paths while still allowing subdirectories.
     • Ensure we only ever pass a trusted project root to RelayPtyOrchestrator. Instead of passing cwd: agentCwd, we introduce a separate notion: projectRootForWrapper, which is always this.projectRoot, and we pass that into the wrapper as cwd (used as a project root for getProjectPaths). We still keep agentCwd for the actual child process working directory but pass it via a new env variable, e.g. AGENT_WORKING_DIR, which the Rust binary or surrounding logic can consume if needed. Since we must not change behavior, we must be careful: currently RelayPtyOrchestratorConfig.cwd is both the project root for getProjectPaths and potentially also the working directory. If the wrapper uses config.cwd as the child process working directory elsewhere (not shown), changing it would be a behavior change. To avoid that, a better option is:
       • Keep cwd: agentCwd in ptyConfig (preserving behavior).
       • But in RelayPtyOrchestrator we stop trusting config.cwd as a project root, and instead call getProjectPaths() with no argument (letting it use its own safe findProjectRoot() based on the process working directory / environment). This breaks the taint link from cwd into log directory paths, while leaving process working directory behavior untouched.
   • This means only RelayPtyOrchestrator needs to change how it calls getProjectPaths, not the spawner.

2. In packages/wrapper/src/relay-pty-orchestrator.ts

   • In the constructor, replace const projectPaths = getProjectPaths(config.cwd); with a call that does not depend directly on the tainted cwd, such as:
     • const projectPaths = getProjectPaths(); (letting it auto-detect based on process.cwd() and environment).
       This ensures projectPaths.dataDir (and thus _logPath and its dirname) are based on a trusted notion of project root, not on user-provided working directory.
   • Everything else that depends on projectPaths (outbox, teamDir, logPath) remains the same, preserving overall functionality except that logs/outbox are now tied to the server’s project root rather than the user-supplied cwd. Given that cwd is supposed to be a working directory for the spawned command, this is a sensible separation and security hardening.

3. In packages/config/src/project-namespace.ts

   • No code changes needed; getProjectPaths is already safe if we stop feeding it tainted arguments.

4. In packages/dashboard/src/server.ts and packages/dashboard-server/src/server.ts

   • Optionally (defense in depth), we could sanitize or validate cwd from req.body to only allow strings and e.g. disallow NUL and certain control characters. However, the critical traversal handling is already in the spawner, and the main CodeQL issue arises from getProjectPaths(config.cwd). So we can leave these unchanged for minimal intrusion.

Net effect: user-specified cwd can still control the working directory of spawned agents (bounded to the project root), but cannot influence where the relay wrapper stores its logs and data, eliminating this uncontrolled data → path sink that CodeQL flagged.

---

General approach: Ensure that user-controlled cwd cannot cause logs or other project data to be written outside the intended project area. The cleanest way, without changing external behavior, is to treat the projectRoot argument to getProjectPaths as a starting directory and always normalize it via the existing findProjectRoot logic. That way, even if projectRoot comes from user input, the actual root used for dataDir/teamDir/socketPath is the canonical project root derived from trusted markers, not the arbitrary path. This also makes the API semantics clearer.

Concrete changes:

1. In packages/config/src/project-namespace.ts, update getProjectPaths(projectRoot?: string):

   • Instead of using projectRoot ?? findProjectRoot(), always call findProjectRoot with projectRoot as the starting directory when provided: const root = findProjectRoot(projectRoot ?? process.cwd());.
   • This ensures that even tainted values passed as projectRoot only influence the starting point of the search for .git/package.json/..., but the resulting root is a directory on the upward path that contains one of the markers (or the normalized starting directory if no markers are found). That prevents someone from e.g. passing /tmp/../../etc and having dataDir point at /etc/.agent-relay.

2. No changes are required in relay-pty-orchestrator.ts itself: once getProjectPaths is hardened, the resulting projectPaths.teamDir (and thus _logPath and its dirname) cannot be redirected outside the project just by controlling cwd. The existing name validation and cwd-within-project checks in spawner.ts remain valid and complementary.

3. No new imports or dependencies are needed; we only adjust the logic of getProjectPaths using existing findProjectRoot.

This change:

• Preserves existing behavior for all current callers that either pass nothing (still uses findProjectRoot() effectively) or pass a directory inside the actual project.
• Hardens the function against future misuse where an unvalidated absolute path might otherwise be passed directly and used to construct an arbitrary .agent-relay directory.

---

In general, to fix uncontrolled-path issues you must ensure any user-influenced path is either (a) constrained to live under a trusted root directory using normalization and prefix checks, or (b) reduced to a safe filename using sanitization. In this codebase, config.cwd from the spawn request is already constrained to be within the spawner’s projectRoot. The remaining gap is that getProjectPaths(projectRoot) currently accepts any string and treats it as a project root without checking that it’s reasonable (e.g., not /, not C:\, etc.). That allows a malicious cwd that equals the spawner’s projectRoot but that project root itself might be something sensitive in other contexts, and other future callers could pass unsafe values without realizing they must re-implement the same validations.

The single best minimal fix is to harden getProjectPaths in packages/config/src/project-namespace.ts so that when a projectRoot argument is provided it is normalized, and then rejected if it is the filesystem root. This ensures that all data directories (dataDir, teamDir, and thereby _logPath) will always be located beneath some non-root directory, preventing logs (and other data) from being written directly under / or another root path derived from untrusted input. Concretely:

• In getProjectPaths, compute const resolvedRoot = path.resolve(projectRoot) when projectRoot is passed.
• Determine the filesystem root with const filesystemRoot = path.parse(resolvedRoot).root.
• If resolvedRoot === filesystemRoot, throw an error indicating an invalid project root.
• Use resolvedRoot for all subsequent path constructions.

This change is localized to project-namespace.ts and does not alter behavior for normal, valid project roots. All existing callers — including RelayPtyOrchestrator — will now benefit from this extra guard, and CodeQL’s taint path to _logPath will pass through a clear validation barrier.

---

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 24 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 31 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View issues and 37 additional flags in Devin Review.

[devin-ai-integration]

🟡 MCP RelayClient.sendAndWait returns incorrect from because ACK sender is at envelope level

RelayClient.sendAndWait() calls request('SEND', ...) and then returns { from: r.from ?? to, content: r.response ?? '' } (packages/mcp/src/client.ts:323-340).

For blocking sends, the daemon replies with an ACK envelope where the responder identity is encoded at the envelope level (envelope.from), not inside the ACK payload. The request helper resolves with response.payload only (packages/mcp/src/client.ts:281-299), so r.from will be undefined and the method will report the recipient (to) as the sender.

Actual vs expected: caller receives a from that is always the original to rather than the actual acknowledging agent.

Impact: downstream tools/UI may misattribute acknowledgements (especially if future flows allow intermediaries, channels, or delegated acknowledgements).

Code pointers

• sendAndWait uses r.from ?? to: packages/mcp/src/client.ts:323-340
• request() resolves response.payload (drops envelope.from): packages/mcp/src/client.ts:281-299

Recommendation: Have request() optionally return the whole response envelope (or at least include response.from), and in sendAndWait use that envelope-level from. If the intention is just to wait for ACK, consider returning { from: responseEnvelope.from, status: payload.response } (and rename content to status).

---

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

[devin-ai-integration]

Devin Review found 4 new potential issues.

View issues and 45 additional flags in Devin Review.

[devin-ai-integration]

🔴 Unauthenticated socket clients can call privileged daemon operations (INBOX/RELEASE/SPAWN/etc.) without HELLO

Connection.processFrame only enforces state for HELLO and (partially) SEND. All other message types are forwarded to onMessage regardless of connection state. This means a client can connect and immediately issue query/control frames (e.g. INBOX, LIST_AGENTS, HEALTH, METRICS, SPAWN, RELEASE) without completing the handshake.

Actual: Any local client that can reach the Unix socket can perform daemon operations without identifying itself.

Expected: Non-handshake frames should be rejected until state is ACTIVE, unless explicitly whitelisted and authenticated.

Click to expand

• Connection forwards all non-handshake frames: packages/daemon/src/connection.ts:194-217

  switch (envelope.type) {
    case 'HELLO': ...
    case 'SEND': ...
    case 'ACK': ...
    case 'PONG': ...
    case 'BYE': ...
    default:
      this.onMessage?.(envelope);
  }

• Daemon handles privileged operations like RELEASE and INBOX in its message handler: packages/daemon/src/server.ts:1123-1195.

Impact:

• Anyone with socket access can:
  • Spawn or release agents
  • Read inbox (see BUG-0003 for reading arbitrary agent inbox)
  • Enumerate agents / system metrics

(Refers to lines 194-217)

Recommendation: Gate non-handshake message types behind ACTIVE state (or a small authenticated allowlist). If MCP requires pre-HELLO access, add an explicit authenticated handshake/token mechanism for MCP connections.

---

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

[devin-ai-integration]

🟡 Heartbeat “processing exemption” does not reset lastPongReceived, causing immediate disconnect after long processing

When a connection exceeds the heartbeat timeout but isProcessing(agentName) returns true, the code comments say it will “reset the pong timer”, but it does not update lastPongReceived.

Actual: While processing, every heartbeat interval keeps seeing now - lastPongReceived > timeoutMs. Once isProcessing flips to false, the connection is immediately terminated on the next tick because lastPongReceived is still stale.

Expected: If a connection is exempted due to processing, it should extend the deadline (e.g., set lastPongReceived = now) to avoid killing the connection immediately when processing ends.

Click to expand

packages/daemon/src/connection.ts:323-334

if (this.lastPongReceived && now - this.lastPongReceived > timeoutMs) {
  if (this._agentName && this.config.isProcessing?.(this._agentName)) {
    // Agent is processing - reset the pong timer to avoid timeout
    console.log(...);
  } else {
    this.handleError(new Error(`Heartbeat timeout ...`));
    return;
  }
}

(Refers to lines 323-334)

Recommendation: When exempting due to processing, set this.lastPongReceived = now (or track a separate deadline) so the connection gets a fresh grace period after processing ends.

---

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

[devin-ai-integration]

🔴 Unauthenticated pre-HELLO SEND allows spoofing arbitrary sender/recipient (MCP bypass)

Connection.handleSend allows processing SEND frames while the connection is not in ACTIVE as long as the envelope has both from and to set (hasMcpSender). This lets any process that can reach the daemon socket send messages while impersonating any agent/user (since Router.route trusts envelope.from as the sender name).

Actual: a client can connect and immediately send {type:"SEND", from:"Alice", to:"Bob", ...} without a HELLO handshake.

Expected: either (a) require handshake before allowing SEND, or (b) for “MCP short-lived connections” do not trust arbitrary from—derive identity from an authenticated/handshaken connection or a capability token.

Click to expand

Relevant code:

• State bypass in connection layer: packages/daemon/src/connection.ts:293-307

  const hasMcpSender = !!envelope.from && !!envelope.to;
  if (this._state !== 'ACTIVE' && !hasMcpSender) {
    this.sendError('BAD_REQUEST', 'Not in ACTIVE state', false);
    return;
  }
  this.onMessage?.(envelope);

• Router chooses sender name from envelope.from when present: packages/daemon/src/router.ts:555-565

  const senderName = isCrossMachine ? envelope.from : (envelope.from || from.agentName);

Impact:

• Message spoofing / impersonation
• Potential privilege escalation if other message types rely on sender identity

(Refers to lines 293-307)

Recommendation: Do not allow unauthenticated SEND prior to HELLO, or require an auth token for MCP-mode connections and bind an immutable sender identity to that token (ignore/override envelope.from).

---

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

[devin-ai-integration]

🔴 INBOX query trusts user-supplied agent name, enabling reading other agents’ messages

The daemon’s INBOX handler uses inboxPayload.agent || connection.agentName to choose which inbox to read. With the new “no-HELLO” query path, connection.agentName may be unset, and the request can specify any agent value.

Actual: a client can request INBOX with payload.agent = "SomeOtherAgent" and receive that agent’s stored messages.

Expected: inbox queries should be restricted to the authenticated/handshaken connection identity (or otherwise authorized). If a client isn’t associated with an agent, it should not be able to query any inbox.

Click to expand

packages/daemon/src/server.ts:1153-1167

const inboxPayload = envelope.payload as InboxPayload;
const agentName = inboxPayload.agent || connection.agentName;
...
const messages = await this.storage.getMessages({
  to: agentName,
  from: inboxPayload.from,
  limit: inboxPayload.limit || 50,
  unreadOnly: inboxPayload.unreadOnly,
});

Impact:

• Confidentiality breach: any local process with socket access can read stored messages for any agent.

Recommendation: Ignore inboxPayload.agent unless the caller is authorized. Use connection.agentName only (and require HELLO/auth first), or implement explicit ACL/token-based authorization for cross-agent inbox access.

---

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

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 61 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View issues and 64 additional flags in Devin Review.

[devin-ai-integration]

🔴 INBOX handler can return all messages if payload.agent is missing/empty (privacy leak)

INBOX requests compute agentName as inboxPayload.agent || connection.agentName and then pass it to storage.getMessages({ to: agentName, ... }).

If a client sends a malformed INBOX frame with no payload.agent (or an empty string) and the connection has not completed HELLO (so connection.agentName is undefined), agentName becomes undefined/falsy and the query becomes effectively unscoped. In the in-memory adapter (and likely SQLite adapter) the to filter is only applied when query.to is truthy, so this can return all messages in storage.

Actual: unauthenticated / non-handshaken clients can potentially exfiltrate other agents’ messages.
Expected: INBOX should always be scoped to a specific agent and reject requests without a valid agent name.

Click to expand

Relevant code:

const inboxPayload = envelope.payload as InboxPayload;
const agentName = inboxPayload.agent || connection.agentName;
...
const messages = await this.storage.getMessages({
  to: agentName,
  ...
});

packages/daemon/src/server.ts:1153-1167

Recommendation: Validate inboxPayload.agent (non-empty string) and/or require a completed handshake for INBOX. If absent, send an ERROR response and do not query storage.

---

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

[devin-ai-integration]

🟡 INBOX handler ignores requested channel filter

The InboxPayload includes a channel field, and the MCP README/tooling advertises channel filtering, but the daemon’s INBOX handler never applies it when querying storage.

Actual: relay_inbox(channel=...) (or protocol INBOX with channel) returns messages from all channels.
Expected: channel filter should be honored (either via storage query or post-filtering).

Click to expand

packages/daemon/src/server.ts:1153-1167 constructs the query:

const messages = await this.storage.getMessages({
  to: agentName,
  from: inboxPayload.from,
  limit: inboxPayload.limit || 50,
  unreadOnly: inboxPayload.unreadOnly,
});

No channel is included, despite being present on InboxPayload.

Recommendation: Pass a channel/topic filter into getMessages if supported (e.g. topic), or filter returned rows by m.data.channel before responding.

---

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

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 66 additional flags in Devin Review.

[devin-ai-integration]

🟡 Default model logging inconsistency - logs 'opus' but actually uses 'sonnet'

The effectiveModel variable defaults to 'opus' but the actual CLI variant defaults to 'sonnet', causing misleading logs.

Click to expand

At packages/bridge/src/spawner.ts:839, the default model for logging was changed from 'sonnet' to 'opus':

const effectiveModel = modelFromProfile || 'opus';

However, the actual model selection at line 835-837 still defaults to sonnet:

const cliVariant = modelFromProfile
  ? mapModelToCli(modelFromProfile)
  : mapModelToCli(); // defaults to claude:sonnet (per comment)

This causes a mismatch: when no model is specified in the agent profile, the code logs model=opus (line 848) but actually spawns the agent with claude:sonnet. This inconsistency could lead to:

• Incorrect cost tracking (opus is more expensive than sonnet)
• Confusion when debugging model-related issues
• Misleading audit logs

The effectiveModel should match what's actually used. Since mapModelToCli() defaults to sonnet, the logging should also default to 'sonnet'.

Recommendation: Change the default from 'opus' back to 'sonnet' to match the actual model used: const effectiveModel = modelFromProfile || 'sonnet';

---

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

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 72 additional flags in Devin Review.

[devin-ai-integration]

🔴 MCP client sendAndWait now returns ACK status instead of a message reply (API contract change/break)

RelayClient.sendAndWait() used to call a dedicated SEND_AND_WAIT flow that returned {from, body} (a real reply). It now sends a blocking SEND and resolves with the daemon-forwarded ACK payload.

• Expected: sendAndWait() returns the content of a reply message (or at least a well-defined response payload), consistent with its signature { from, content }.
• Actual: the code maps AckPayload.response (an ACK status string) into content, defaulting to '' if not present. In typical ACKs, there is no message body, so callers get empty or status-like content rather than a reply.

Code

packages/mcp/src/client.ts:323-340

const r = await request<{ correlationId?: string; response?: string; from?: string }>('SEND', {
  kind: 'message',
  body: message,
  thread: opts.thread,
}, waitTimeout + 5000, {
  sync: { blocking: true, correlationId, timeoutMs: waitTimeout },
}, { from: agentName, to });
return { from: r.from ?? to, content: r.response ?? '', thread: opts.thread };

Impact: any consumers relying on sendAndWait() for a reply body will behave incorrectly (e.g., MCP tool relay_send(..., await_response=true)-style usage).

Recommendation: Either (a) restore a true request/response message flow (e.g., a SEND_AND_WAIT message type with a reply payload), or (b) rename/retype this method to reflect that it only waits for an ACK and returns an ACK status/metadata.

---

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

[devin-ai-integration]

Devin Review found 3 new potential issues.

View issues and 75 additional flags in Devin Review.

[devin-ai-integration]

🔴 Dashboard WebSocket handler blocks event loop with synchronous polling

The subscribeToAgent function performs busy-waiting with a while loop that blocks the WebSocket event loop for up to 3 seconds, preventing the connection from handling close/error events during that time.

Click to expand

How it gets triggered

At packages/dashboard-server/src/server.ts:2250-2267 and packages/dashboard/src/server.ts:2277-2294, when a WebSocket client subscribes to an agent that is daemon-connected but not yet in the spawner's activeWorkers:

if (!isSpawned && isDaemon && spawner) {
  const maxWaitMs = 3000;
  const pollIntervalMs = 100;
  const startTime = Date.now();

  while (Date.now() - startTime < maxWaitMs) {
    await new Promise(resolve => setTimeout(resolve, pollIntervalMs));
    isSpawned = spawner.hasWorker(agentName);
    // ...
  }
}

This while loop runs in the WebSocket message handler, blocking that handler thread.

Actual vs Expected

Actual: WebSocket handler is blocked for up to 3 seconds, during which:

• Cannot process other WebSocket messages
• Cannot detect if the WebSocket was closed
• Holds up the event loop

Expected: Use event-based notification or move polling to a separate async task

Impact

During the 3-second polling window:

• Other WebSocket operations on that connection are delayed
• If the client closes the connection, it won't be detected until polling completes
• Can cause perceived lag in the dashboard UI
• Multiple concurrent subscriptions could stack up and significantly delay the event loop

Recommendation: Use event-based notification from spawner when agents become available, or move polling to a background task that doesn't block the WebSocket handler

---

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

[devin-ai-integration]

🔴 fireAndForget resolves before write completes or on socket errors

The fireAndForget function resolves immediately when socket.end() is called, without waiting for the write operation to complete or handling post-end socket errors.

Click to expand

How it gets triggered

At packages/mcp/src/client.ts:218-221:

socket.on('connect', () => {
  socket.write(encodeFrame(envelope));
  socket.end();
  resolve();
});

The promise resolves immediately after calling socket.end(), but:

1. socket.write() is asynchronous - the data may not have been sent yet
2. socket.end() is also asynchronous - it initiates closing but doesn't wait for completion
3. Socket errors that occur after end() are not caught (error handler only registered after promise resolves)

Actual vs Expected

Actual: Function returns success even if:

• Write buffer is full and message is dropped
• Socket errors occur after end() but before data is transmitted
• Network failure prevents message delivery

Expected: Wait for write to complete or handle write callback to ensure message was buffered

Impact

MCP tools (send, spawn, release) may report success when the message was never actually delivered to the daemon. This leads to:

• Silent message loss
• Agents thinking they sent a message when it never arrived
• Spawn/release operations failing silently
• Difficult to debug communication failures

Recommendation: Wait for write callback or use socket.once('finish', ...) to ensure data was buffered before resolving

---

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

[devin-ai-integration]

🟡 Router duplicate connection handling doesn't verify error send succeeded

When detecting duplicate agent connections, the router sends a DUPLICATE_CONNECTION fatal error to the old connection but doesn't verify the send succeeded before closing the connection.

Click to expand

How it gets triggered

At packages/daemon/src/router.ts:273-286, when a new agent connection registers with a name that already exists:

existing.send(errorEnvelope);
existing.close();

The send() method returns a boolean indicating success, but the return value is ignored. If the old connection's write queue is full or the socket is already broken, the error won't be delivered.

Actual vs Expected

Actual: Old connection is closed immediately without verifying the fatal error was sent

Expected: Check if send() succeeded, or add a small delay to allow the error to be transmitted before closing

Impact

The old agent connection is closed without knowing why, which can cause:

• Unexpected disconnection without error message
• Agent attempts to reconnect, creating a reconnection loop
• Confusion in logs about why connection was terminated
• Poor user experience when debugging duplicate connection issues

Recommendation: Check the return value of existing.send() or add a small delay (e.g., setImmediate) before calling existing.close() to allow the error to be transmitted

---

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