fix(mcp): stop classifying devices as offline in list_devices#3299
Conversation
PR #2904 deliberately removed device heartbeat polling (desktop calls device.registerDevice once at startup) because executeOnDevice already handles unreachable devices via its command-poll timeout. PR #2988 reintroduced an isOnline field computed from a 60s lastSeenAt window without restoring heartbeats, so every device looked offline ~60s after app start — surfacing in the Slack integration agent as "status: offline". Revert the isOnline reintroduction (keep #2988's schema hardening), drop includeOffline from the input, update the Slack run-agent to stop rendering the removed status, and refresh the docs + stubbed CLI flag. Also fix a pre-existing mock-module bleed in list-devices.test.ts that was stripping executeOnDevice from start-agent-session.test.ts.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes a regression where devices were incorrectly shown as offline ~60 seconds after the desktop app started. PR #2904 removed the 30s heartbeat (desktop now calls Key changes:
Confidence Score: 4/5Safe to merge — the core bug fix is correct and well-tested; only documentation inaccuracies remain. The runtime bug fix (removing the 60s
|
| Filename | Overview |
|---|---|
| packages/mcp/src/tools/devices/list-devices/list-devices.ts | Removes isOnline computation and includeOffline filter from the device query; all registered devices are now returned regardless of lastSeenAt, matching the heartbeat-less design of PR #2904. |
| packages/mcp/src/tools/devices/list-devices/list-devices.test.ts | Fixes a process-global mock.module leak by spreading real exports of ../../utils before overriding getMcpContext, preventing executeOnDevice from being stripped in sibling test suites. |
| apps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.ts | Updates fetchAgentContext device rendering to drop the removed status field; device lines now show only name, id, and owner — no "offline" label. |
| packages/cli/src/commands/devices/list/command.ts | Removes the --include-offline stub option added alongside the removed isOnline field; display columns updated to drop the obsolete status column. |
| apps/api/MCP_TOOLS.md | Partially refreshed: isOnline/includeOffline removed, but the listDevicesOutput schema still shows deviceName and ownerName as non-nullable and is missing the ownerEmail field; Device Targeting section still references the removed "heartbeat within last 60s" constraint. |
| apps/docs/content/docs/mcp.mdx | Docs updated to remove isOnline/includeOffline references, but the example prompt "Show me who's online in my team" still implies device online-status visibility. |
Sequence Diagram
sequenceDiagram
participant Slack as Slack Bot
participant Agent as fetchAgentContext
participant MCP as MCP list_devices
participant DB as DB (devicePresence)
Slack->>Agent: fetch context for agent session
Agent->>MCP: callTool("list_devices", {})
MCP->>DB: SELECT deviceId, deviceName, deviceType, lastSeenAt, userId, name, email WHERE organizationId = ctx.organizationId ORDER BY lastSeenAt DESC
DB-->>MCP: all registered devices (no time filter)
MCP-->>Agent: { devices: [...] }
Note over Agent: Renders: "- DeviceName (id: ..., owner: ...)"
Note over Agent: (no status field — "offline" label removed)
Agent-->>Slack: context string injected into system prompt
Comments Outside Diff (2)
-
apps/api/MCP_TOOLS.md, line 15-18 (link)Stale "heartbeat within last 60s" language
This bullet still says a device must be online via heartbeat within the last 60 seconds to receive commands, but PR Remove device heartbeat polling to reduce Vercel costs #2904 deliberately removed the 30s heartbeat — the desktop now only calls
registerDeviceonce at startup. The "within last 60s" constraint is no longer enforced, andexecuteOnDevicehandles unreachable devices via its command-poll timeout instead. The description should be updated to reflect the actual behavior. -
apps/docs/content/docs/mcp.mdx, line 274 (link)The example prompt
"Show me who's online in my team"implies thatlist_devicesreturns online/offline status, but that's exactly what this PR removes. Since no status field is returned anymore, this example would produce a confusing result (or lead a user to believe the feature still exists). Consider updating to match current behavior.
Reviews (1): Last reviewed commit: "fix(mcp): stop classifying devices as of..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -176,7 +174,6 @@ const listDevicesOutput = z.object({ | |||
| ownerId: z.string().uuid().describe("User who owns this device"), | |||
| ownerName: z.string().describe("Name of device owner"), | |||
| lastSeenAt: z.string().datetime(), | |||
| isOnline: z.boolean(), | |||
| })), | |||
| }); | |||
| ``` | |||
There was a problem hiding this comment.
listDevicesOutput schema is out of sync with the implementation
The schema documented here no longer matches the actual Zod schema in packages/mcp/src/tools/devices/list-devices/list-devices.ts. Three differences:
deviceNameisz.string().nullable()in the implementation, but shown as non-nullable here.ownerNameisz.string().nullable()in the implementation, but shown as non-nullable here.ownerEmail: z.string()is present in the implementation but is missing entirely from this schema block.
The PR description says this file was refreshed, but the schema block was not updated to match.
| const listDevicesOutput = z.object({ | |
| devices: z.array(z.object({ | |
| deviceId: z.string(), | |
| deviceName: z.string().nullable(), | |
| deviceType: z.enum(["desktop", "mobile", "web"]), | |
| lastSeenAt: z.string().datetime(), | |
| ownerId: z.string(), | |
| ownerName: z.string().nullable(), | |
| ownerEmail: z.string(), | |
| })), | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/MCP_TOOLS.md (1)
163-179:⚠️ Potential issue | 🟡 MinorSync
listDevicesOutputdocs with the actual tool schema.The documented output is stale:
deviceName/ownerNameare nullable in the implementation, andownerEmailis returned but not documented.📝 Proposed doc fix
const listDevicesOutput = z.object({ devices: z.array(z.object({ deviceId: z.string(), - deviceName: z.string(), + deviceName: z.string().nullable(), deviceType: z.enum(["desktop", "mobile", "web"]), ownerId: z.string().uuid().describe("User who owns this device"), - ownerName: z.string().describe("Name of device owner"), + ownerName: z.string().nullable().describe("Name of device owner"), + ownerEmail: z.string().email().describe("Email of device owner"), lastSeenAt: z.string().datetime(), })), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/MCP_TOOLS.md` around lines 163 - 179, The docs for list_devices are out of sync with the tool schema: update the listDevicesOutput schema block so deviceName and ownerName are nullable (or optional) to match the implementation and add ownerEmail to the returned fields; specifically edit the listDevicesOutput definition used in the docs (referencing listDevicesOutput, deviceName, ownerName, ownerEmail) to reflect the actual types (nullable strings for deviceName/ownerName and include ownerEmail as a string/uuid/email as per implementation).apps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.ts (1)
406-420:⚠️ Potential issue | 🟠 MajorBound device context before injecting it into the system prompt.
Now that
list_devicesreturns all registered devices, this section can grow unbounded and degrade latency/cost or hit token limits in larger orgs.💡 Proposed fix (truncate + summarize)
+const MAX_AGENT_CONTEXT_DEVICES = 50; + async function fetchAgentContext({ mcpClient, userId, @@ if (devicesData?.devices?.length) { - const lines = devicesData.devices.map( + const totalDevices = devicesData.devices.length; + const visibleDevices = devicesData.devices.slice(0, MAX_AGENT_CONTEXT_DEVICES); + const lines = visibleDevices.map( (d) => `- ${d.deviceName ?? "Unknown"} (id: ${d.deviceId}, owner: ${d.ownerName ?? d.ownerEmail})`, ); - sections.push(`Devices:\n${lines.join("\n")}`); + const remainder = + totalDevices > visibleDevices.length + ? `\n- ...and ${totalDevices - visibleDevices.length} more` + : ""; + sections.push(`Devices (${totalDevices} total):\n${lines.join("\n")}${remainder}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.ts` around lines 406 - 420, The devices block currently injects an unbounded list from devicesResult.structuredContent (devicesData.devices) directly into sections, which can blow up prompt size; modify the logic in run-agent.ts that builds sections to truncate/summarize before pushing: if devicesData?.devices.length exceeds a safe limit (e.g., 20), include only a sample (first N) formatted with the existing template and append a summary line like "…and X more devices (total Y)"; alternatively call a new helper (e.g., summarizeDevices or summarizeDevicesList) to produce a short aggregate (owner counts or truncated list) and use that output instead of the full array, ensuring the final sections.push uses the bounded string.
🧹 Nitpick comments (1)
packages/mcp/src/tools/devices/list-devices/list-devices.test.ts (1)
131-167: Add a nullable-name fixture to lock schema hardening behavior.Current assertions cover only non-null names; adding a case for
deviceName: nullandownerName: nullwould protect the nullable output contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp/src/tools/devices/list-devices/list-devices.test.ts` around lines 131 - 167, Add a new test case (or extend the existing "returns every registered device regardless of lastSeenAt") that uses createTool()/handler to return a device record where deviceName and ownerName are null; ensure the mock selectMock payload includes device entries with deviceName: null and ownerName: null and then assert result.structuredContent?.devices contains those entries (with lastSeenAt still expect.any(String) and ownerName/deviceName === null). Also verify the output schema parsing still succeeds by reusing outputSchema.parse(result.structuredContent) as in the existing test so the nullable-name contract is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/MCP_TOOLS.md`:
- Around line 163-179: The docs for list_devices are out of sync with the tool
schema: update the listDevicesOutput schema block so deviceName and ownerName
are nullable (or optional) to match the implementation and add ownerEmail to the
returned fields; specifically edit the listDevicesOutput definition used in the
docs (referencing listDevicesOutput, deviceName, ownerName, ownerEmail) to
reflect the actual types (nullable strings for deviceName/ownerName and include
ownerEmail as a string/uuid/email as per implementation).
In `@apps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.ts`:
- Around line 406-420: The devices block currently injects an unbounded list
from devicesResult.structuredContent (devicesData.devices) directly into
sections, which can blow up prompt size; modify the logic in run-agent.ts that
builds sections to truncate/summarize before pushing: if
devicesData?.devices.length exceeds a safe limit (e.g., 20), include only a
sample (first N) formatted with the existing template and append a summary line
like "…and X more devices (total Y)"; alternatively call a new helper (e.g.,
summarizeDevices or summarizeDevicesList) to produce a short aggregate (owner
counts or truncated list) and use that output instead of the full array,
ensuring the final sections.push uses the bounded string.
---
Nitpick comments:
In `@packages/mcp/src/tools/devices/list-devices/list-devices.test.ts`:
- Around line 131-167: Add a new test case (or extend the existing "returns
every registered device regardless of lastSeenAt") that uses
createTool()/handler to return a device record where deviceName and ownerName
are null; ensure the mock selectMock payload includes device entries with
deviceName: null and ownerName: null and then assert
result.structuredContent?.devices contains those entries (with lastSeenAt still
expect.any(String) and ownerName/deviceName === null). Also verify the output
schema parsing still succeeds by reusing
outputSchema.parse(result.structuredContent) as in the existing test so the
nullable-name contract is locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6db04215-834b-4e1a-bb25-8b124a9b9d60
📒 Files selected for processing (6)
apps/api/MCP_TOOLS.mdapps/api/src/app/api/integrations/slack/events/utils/run-agent/run-agent.tsapps/docs/content/docs/mcp.mdxpackages/cli/src/commands/devices/list/command.tspackages/mcp/src/tools/devices/list-devices/list-devices.test.tspackages/mcp/src/tools/devices/list-devices/list-devices.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 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="apps/api/MCP_TOOLS.md">
<violation number="1" location="apps/api/MCP_TOOLS.md:167">
P2: `listDevicesOutput` schema in MCP_TOOLS.md is still out of sync with the implementation after this refresh: `deviceName` and `ownerName` should be `.nullable()`, and `ownerEmail: z.string()` is missing entirely. The implementation in `list-devices.ts` and the type annotation in `run-agent.ts` both confirm these three differences.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -161,12 +161,10 @@ const listTaskStatusesOutput = z.object({ | |||
| These tools write to `agent_commands` table and poll for results. | |||
There was a problem hiding this comment.
P2: listDevicesOutput schema in MCP_TOOLS.md is still out of sync with the implementation after this refresh: deviceName and ownerName should be .nullable(), and ownerEmail: z.string() is missing entirely. The implementation in list-devices.ts and the type annotation in run-agent.ts both confirm these three differences.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/MCP_TOOLS.md, line 167:
<comment>`listDevicesOutput` schema in MCP_TOOLS.md is still out of sync with the implementation after this refresh: `deviceName` and `ownerName` should be `.nullable()`, and `ownerEmail: z.string()` is missing entirely. The implementation in `list-devices.ts` and the type annotation in `run-agent.ts` both confirm these three differences.</comment>
<file context>
@@ -161,12 +161,10 @@ const listTaskStatusesOutput = z.object({
-const listDevicesInput = z.object({
- includeOffline: z.boolean().default(false).describe("Include recently offline devices"),
-});
+const listDevicesInput = z.object({});
const listDevicesOutput = z.object({
</file context>
…et-sh#3299) PR superset-sh#2904 deliberately removed device heartbeat polling (desktop calls device.registerDevice once at startup) because executeOnDevice already handles unreachable devices via its command-poll timeout. PR superset-sh#2988 reintroduced an isOnline field computed from a 60s lastSeenAt window without restoring heartbeats, so every device looked offline ~60s after app start — surfacing in the Slack integration agent as "status: offline". Revert the isOnline reintroduction (keep superset-sh#2988's schema hardening), drop includeOffline from the input, update the Slack run-agent to stop rendering the removed status, and refresh the docs + stubbed CLI flag. Also fix a pre-existing mock-module bleed in list-devices.test.ts that was stripping executeOnDevice from start-agent-session.test.ts.
Kitenite
deleted the
saddlepaddle/satya/fix/device-offline-worktree-selection
branch
1 participant
Summary
status: offlinefor his device.device.registerDeviceonce at startup) becauseexecuteOnDevicealready handles unreachable devices via its command-poll timeout. PR [codex] Fix list_devices MCP schema #2988 ("fix list_devices MCP schema") reintroduced anisOnlinefield computed from a 60slastSeenAtwindow without restoring heartbeats, so every device always looked offline after ~60s of runtime.isOnline/includeOfflinereintroduction inlist_devices(keep PR [codex] Fix list_devices MCP schema #2988's schema hardening —datetime,deviceTypeValuesenum, nullabledeviceName). Update the Slack integration'sfetchAgentContextto stop rendering the removed status, and refreshapps/api/MCP_TOOLS.md,apps/docs/content/docs/mcp.mdx, and the stubbedsuperset devices listCLI flag to match.packages/mcp/src/tools/devices/list-devices/list-devices.test.tshad amock.module("../../utils", ...)that replaced the module with a partial stub. Becausemock.moduleis process-global in bun test, it bled intostart-agent-session.test.tsand stripped theexecuteOnDeviceexport. The test file now spreads the real module before overridinggetMcpContext.Test plan
bun run lintbun run typecheckbun run test(all 8 turbo tasks green;@superset/mcp7/7 pass including the previously-brokenstart-agent-sessionsuite)Summary by cubic
Stop marking devices as offline.
list_devicesnow returns all registered devices without an online status, so Slack no longer shows “status: offline” after ~60s.isOnlineandincludeOfflinefromlist_devices; continue returninglastSeenAt.list_deviceswith{}and stop rendering status text.includeOfflineflag and the “status” column.mock.modulebleed by preserving real exports when overridinggetMcpContext.Written for commit 8e376e8. Summary will update on new commits.
Summary by CodeRabbit
Changes
Documentation