New detectors daemon for detection and quarantine#29
Open
dimitriosGX wants to merge 2 commits into
Open
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found across 22 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="scripts/build-detectord.sh">
<violation number="1" location="scripts/build-detectord.sh:36">
P3: This adds a near copy of the universal Rust build pipeline already implemented in `build-stdiod.sh`, so future build-flow fixes (targets, flags, signing, validation) can drift between daemons. Consider extracting shared logic into one helper script/function that takes daemon path/binary name parameters.</violation>
</file>
<file name="src/main/detectord/socket.ts">
<violation number="1" location="src/main/detectord/socket.ts:27">
P3: `DetectordError` doesn't set `this.name`, so instances report `name === 'Error'` instead of `'DetectordError'`, which makes log filtering and error-type checks less reliable.
Subclassing `Error` without overriding `name` is a common gotcha — JavaScript's `Error.prototype.name` defaults to `'Error'`. Add a constructor to set `this.name = 'DetectordError'`.</violation>
<violation number="2" location="src/main/detectord/socket.ts:66">
P2: After a daemon disconnect in the middle of a line, the next connection can prepend its first reply to stale buffered bytes, causing JSON.parse to drop that reply and leave the new request pending. Clearing `this.buf` when the socket closes keeps reconnects from inheriting partial frames from the old stream.</violation>
</file>
<file name="src/renderer/src/components/OrgKeyCard.tsx">
<violation number="1" location="src/renderer/src/components/OrgKeyCard.tsx:160">
P2: Detector enrollment failures can be silently treated as success here because `detectord:setSecret` resolves `{ ok: false }` for expected failures rather than throwing. Consider checking the returned `ok` value inside this non-fatal block so the failure path is at least logged or surfaced consistently.</violation>
</file>
<file name="src/main/detectord/lifecycle.ts">
<violation number="1" location="src/main/detectord/lifecycle.ts:45">
P2: Concurrent bootstrap/enroll paths can run `edison-detectord service install` more than once because the session guard is not reserved until after `await installService(enforce)`. Since install bootstraps/restarts the LaunchAgent, overlapping calls can bounce the daemon while another caller is connecting or using the shared socket; consider serializing install with a shared in-flight promise or setting a guarded install state before awaiting.</violation>
</file>
<file name="src/main/detectord/approvalDialog.ts">
<violation number="1" location="src/main/detectord/approvalDialog.ts:71">
P2: A failed Keep Quarantined disposition is shown as completed, so daemon/socket errors can drop the user's decision without any retry or error in the dialog. The skip handler should return a failure when `client.disposition(..., 'skip', ...)` rejects, and the renderer should only mark the row resolved after an ok response.</violation>
</file>
<file name="src/main/ipc/ipcHandlers.ts">
<violation number="1" location="src/main/ipc/ipcHandlers.ts:218">
P2: Login-triggered detector enrollment can fail silently while the renderer still gets `{ ok: true }`, so the UI/workflow may proceed as if daemon enrollment succeeded when it did not. Returning failure on catch would let callers surface retry/error state.</violation>
</file>
<file name="src/main/index.ts">
<violation number="1" location="src/main/index.ts:531">
P1: Detection/quarantine can be fully disabled on startup when detectord bootstrap fails. The new flow stands down the TS pipeline based only on `detectordPrimary()` and does not gate that decision on a successful daemon bootstrap, so a failed install/connect path has no active fallback.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:15">
P2: `build:detectord` is defined but not wired into `build:mac` or `build:mac:release`, and `bin/edison-detectord` isn't listed in macOS `extraResources` in `electron-builder.yml`. The daemon will not be built or bundled unless these are added.</violation>
</file>
<file name="src/main/ipc/ipcHandlersMcpSubmit.ts">
<violation number="1" location="src/main/ipc/ipcHandlersMcpSubmit.ts:179">
P2: Resubmit can fail in primary mode when the IPC caller omits `client`, because the handler now passes an empty string agent to the daemon instead of leaving agent unspecified. Preserving optional-agent behavior here (or resolving client from cache before calling) would avoid regressions for existing `mcp:resubmitServer` callers that relied on `client` being optional.</violation>
</file>
<file name="src/main/detectord/bootstrap.ts">
<violation number="1" location="src/main/detectord/bootstrap.ts:71">
P1: A transient enroll failure can make the running daemon invisible to the app: the bootstrap exits before subscribing to daemon events. Consider still subscribing/listing when `status().enrolled` is true, and only skipping enrollment-specific work when credentials are unavailable or refresh fails.</violation>
<violation number="2" location="src/main/detectord/bootstrap.ts:124">
P1: Existing users without `configuredApps` can lose all daemon-managed MCP integrations because this enrolls an empty agent list instead of the repository's established all-apps fallback. Consider matching the existing `ALL_SUPPORTED_APPS` fallback before mapping to daemon agent names.</violation>
</file>
<file name="src/main/detectord/submit.ts">
<violation number="1" location="src/main/detectord/submit.ts:44">
P1: `submitServersViaDetectord` calls `client.connect()` outside the try-catch that wraps the per-server disposition loop. If the daemon socket is unavailable (daemon not running, socket path missing, or connection dropped between calls), the connection rejection propagates as an unhandled error through all three IPC callers (`handleSubmitWithTemplates`, `handleSubmitAutoApprovedServers`) — none of which have their own try-catch for this call. Electron will surface a generic IPC error to the renderer instead of a structured failure.
Unlike `discoverViaDetectord` which wraps its entire body in try-catch and returns `null` on failure, this function has no fallback path. Consider wrapping `connect()` in the existing try-catch or adding a catch block that returns a summary with all servers listed as failures.</violation>
<violation number="2" location="src/main/detectord/submit.ts:95">
P1: `resubmitServerViaDetectord` calls `c.connect()` before the try-catch block. If the daemon socket is unreachable (not running, path missing, connection refused), the rejection propagates as an unhandled error. The function's contract promises `{ success: false, error?: string }` on failure, but a connect rejection bypasses that contract and throws to the IPC handler (`mcp:resubmitServer`). The caller in `ipcHandlersMcpSubmit.ts` does not wrap the call either.
Moving `c.connect()` inside the existing try-catch would let the function fulfill its error-return contract even when the daemon is unreachable.</violation>
</file>
<file name="src/preload/index.ts">
<violation number="1" location="src/preload/index.ts:245">
P3: The preload types `setSecret` as returning only `{ ok: boolean }`, but the actual response includes `outcome` (with `valid` status) and `reason` fields. Although the extra fields are present at runtime (IPC passes through the full object), the narrow type prevents the renderer from using `outcome.valid` to confirm the secret was accepted by the daemon. Widen the preload type to match the actual return shape.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Install + launch the detector daemon on EVERY client run (not gated on | ||
| // setup): the daemon owns detection/quarantine/install/hooks. Enrolls if | ||
| // credentials exist yet; otherwise the setup:complete handler enrolls on login. | ||
| bootstrapDetectord().catch((err) => console.error('[detectord] bootstrap failed:', err)) |
There was a problem hiding this comment.
P1: Detection/quarantine can be fully disabled on startup when detectord bootstrap fails. The new flow stands down the TS pipeline based only on detectordPrimary() and does not gate that decision on a successful daemon bootstrap, so a failed install/connect path has no active fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/index.ts, line 531:
<comment>Detection/quarantine can be fully disabled on startup when detectord bootstrap fails. The new flow stands down the TS pipeline based only on `detectordPrimary()` and does not gate that decision on a successful daemon bootstrap, so a failed install/connect path has no active fallback.</comment>
<file context>
@@ -518,23 +525,35 @@ app.whenReady().then(async () => {
+ // Install + launch the detector daemon on EVERY client run (not gated on
+ // setup): the daemon owns detection/quarantine/install/hooks. Enrolls if
+ // credentials exist yet; otherwise the setup:complete handler enrolls on login.
+ bootstrapDetectord().catch((err) => console.error('[detectord] bootstrap failed:', err))
+
if (isSetupComplete()) {
</file context>
| // the existing one). So we always (re-)enroll whenever credentials are | ||
| // available rather than guarding on prior state; agent/key *additions* still | ||
| // come through here (union) and removals go through unenroll. | ||
| if (!(await enrollDaemon(client, primary, creds))) return |
There was a problem hiding this comment.
P1: A transient enroll failure can make the running daemon invisible to the app: the bootstrap exits before subscribing to daemon events. Consider still subscribing/listing when status().enrolled is true, and only skipping enrollment-specific work when credentials are unavailable or refresh fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/detectord/bootstrap.ts, line 71:
<comment>A transient enroll failure can make the running daemon invisible to the app: the bootstrap exits before subscribing to daemon events. Consider still subscribing/listing when `status().enrolled` is true, and only skipping enrollment-specific work when credentials are unavailable or refresh fails.</comment>
<file context>
@@ -0,0 +1,234 @@
+ // the existing one). So we always (re-)enroll whenever credentials are
+ // available rather than guarding on prior state; agent/key *additions* still
+ // come through here (union) and removals go through unenroll.
+ if (!(await enrollDaemon(client, primary, creds))) return
+
+ if (!eventsSubscribed) {
</file context>
| // hasn't reached the app-selection step yet) => a base enroll with no agents: | ||
| // the daemon installs edison-watch on nothing until onboarding adds them. | ||
| // Agents are additive daemon-side, so an empty set never removes any. | ||
| const appIds = setup.configuredApps ?? [] |
There was a problem hiding this comment.
P1: Existing users without configuredApps can lose all daemon-managed MCP integrations because this enrolls an empty agent list instead of the repository's established all-apps fallback. Consider matching the existing ALL_SUPPORTED_APPS fallback before mapping to daemon agent names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/detectord/bootstrap.ts, line 124:
<comment>Existing users without `configuredApps` can lose all daemon-managed MCP integrations because this enrolls an empty agent list instead of the repository's established all-apps fallback. Consider matching the existing `ALL_SUPPORTED_APPS` fallback before mapping to daemon agent names.</comment>
<file context>
@@ -0,0 +1,234 @@
+ // hasn't reached the app-selection step yet) => a base enroll with no agents:
+ // the daemon installs edison-watch on nothing until onboarding adds them.
+ // Agents are additive daemon-side, so an empty set never removes any.
+ const appIds = setup.configuredApps ?? []
+ const agents = appIds.map(toAgent)
+ // Arm auto-quarantine only once onboarding is complete. While a new user is
</file context>
Comment on lines
+95
to
+103
| const c = getDetectordClient() | ||
| await c.connect() | ||
| try { | ||
| await c.disposition(name, 'send_to_ew', toAgent(client), newName) | ||
| return { success: true } | ||
| } catch (err) { | ||
| return { success: false, error: err instanceof Error ? err.message : String(err) } | ||
| } | ||
| } |
There was a problem hiding this comment.
P1: resubmitServerViaDetectord calls c.connect() before the try-catch block. If the daemon socket is unreachable (not running, path missing, connection refused), the rejection propagates as an unhandled error. The function's contract promises { success: false, error?: string } on failure, but a connect rejection bypasses that contract and throws to the IPC handler (mcp:resubmitServer). The caller in ipcHandlersMcpSubmit.ts does not wrap the call either.
Moving c.connect() inside the existing try-catch would let the function fulfill its error-return contract even when the daemon is unreachable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/detectord/submit.ts, line 95:
<comment>`resubmitServerViaDetectord` calls `c.connect()` before the try-catch block. If the daemon socket is unreachable (not running, path missing, connection refused), the rejection propagates as an unhandled error. The function's contract promises `{ success: false, error?: string }` on failure, but a connect rejection bypasses that contract and throws to the IPC handler (`mcp:resubmitServer`). The caller in `ipcHandlersMcpSubmit.ts` does not wrap the call either.
Moving `c.connect()` inside the existing try-catch would let the function fulfill its error-return contract even when the daemon is unreachable.</comment>
<file context>
@@ -0,0 +1,103 @@
+ newName: string,
+ client: string
+): Promise<{ success: boolean; error?: string }> {
+ const c = getDetectordClient()
+ await c.connect()
+ try {
</file context>
Suggested change
| const c = getDetectordClient() | |
| await c.connect() | |
| try { | |
| await c.disposition(name, 'send_to_ew', toAgent(client), newName) | |
| return { success: true } | |
| } catch (err) { | |
| return { success: false, error: err instanceof Error ? err.message : String(err) } | |
| } | |
| } | |
| export async function resubmitServerViaDetectord( | |
| name: string, | |
| newName: string, | |
| client: string | |
| ): Promise<{ success: boolean; error?: string }> { | |
| const c = getDetectordClient() | |
| try { | |
| await c.connect() | |
| await c.disposition(name, 'send_to_ew', toAgent(client), newName) | |
| return { success: true } | |
| } catch (err) { | |
| return { success: false, error: err instanceof Error ? err.message : String(err) } | |
| } | |
| } |
| export async function submitServersViaDetectord( | ||
| servers: DiscoveredMcpServer[] | ||
| ): Promise<DetectordSubmitSummary> { | ||
| const client = getDetectordClient() |
There was a problem hiding this comment.
P1: submitServersViaDetectord calls client.connect() outside the try-catch that wraps the per-server disposition loop. If the daemon socket is unavailable (daemon not running, socket path missing, or connection dropped between calls), the connection rejection propagates as an unhandled error through all three IPC callers (handleSubmitWithTemplates, handleSubmitAutoApprovedServers) — none of which have their own try-catch for this call. Electron will surface a generic IPC error to the renderer instead of a structured failure.
Unlike discoverViaDetectord which wraps its entire body in try-catch and returns null on failure, this function has no fallback path. Consider wrapping connect() in the existing try-catch or adding a catch block that returns a summary with all servers listed as failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/detectord/submit.ts, line 44:
<comment>`submitServersViaDetectord` calls `client.connect()` outside the try-catch that wraps the per-server disposition loop. If the daemon socket is unavailable (daemon not running, socket path missing, or connection dropped between calls), the connection rejection propagates as an unhandled error through all three IPC callers (`handleSubmitWithTemplates`, `handleSubmitAutoApprovedServers`) — none of which have their own try-catch for this call. Electron will surface a generic IPC error to the renderer instead of a structured failure.
Unlike `discoverViaDetectord` which wraps its entire body in try-catch and returns `null` on failure, this function has no fallback path. Consider wrapping `connect()` in the existing try-catch or adding a catch block that returns a summary with all servers listed as failures.</comment>
<file context>
@@ -0,0 +1,103 @@
+export async function submitServersViaDetectord(
+ servers: DiscoveredMcpServer[]
+): Promise<DetectordSubmitSummary> {
+ const client = getDetectordClient()
+ await client.connect()
+ const status = await client.status().catch(() => null)
</file context>
| "build:demo": "npm run typecheck && electron-vite build --mode demo", | ||
| "build:release": "npm run typecheck && electron-vite build --mode release", | ||
| "build:stdiod": "bash scripts/build-stdiod.sh", | ||
| "build:detectord": "bash scripts/build-detectord.sh", |
There was a problem hiding this comment.
P2: build:detectord is defined but not wired into build:mac or build:mac:release, and bin/edison-detectord isn't listed in macOS extraResources in electron-builder.yml. The daemon will not be built or bundled unless these are added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 15:
<comment>`build:detectord` is defined but not wired into `build:mac` or `build:mac:release`, and `bin/edison-detectord` isn't listed in macOS `extraResources` in `electron-builder.yml`. The daemon will not be built or bundled unless these are added.</comment>
<file context>
@@ -12,6 +12,7 @@
"build:demo": "npm run typecheck && electron-vite build --mode demo",
"build:release": "npm run typecheck && electron-vite build --mode release",
"build:stdiod": "bash scripts/build-stdiod.sh",
+ "build:detectord": "bash scripts/build-detectord.sh",
"build:mac": "npm run build:stdiod && npm run build:demo && electron-builder --mac",
"build:mac:release": "npm run build:stdiod && npm run build:release && electron-builder --mac",
</file context>
|
|
||
| // Primary mode: resubmit-under-new-name is a daemon disposition with rename. | ||
| if (detectordPrimary()) { | ||
| return resubmitServerViaDetectord(params.originalName, params.newName, params.client ?? ""); |
There was a problem hiding this comment.
P2: Resubmit can fail in primary mode when the IPC caller omits client, because the handler now passes an empty string agent to the daemon instead of leaving agent unspecified. Preserving optional-agent behavior here (or resolving client from cache before calling) would avoid regressions for existing mcp:resubmitServer callers that relied on client being optional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/ipc/ipcHandlersMcpSubmit.ts, line 179:
<comment>Resubmit can fail in primary mode when the IPC caller omits `client`, because the handler now passes an empty string agent to the daemon instead of leaving agent unspecified. Preserving optional-agent behavior here (or resolving client from cache before calling) would avoid regressions for existing `mcp:resubmitServer` callers that relied on `client` being optional.</comment>
<file context>
@@ -172,6 +174,11 @@ export function registerMcpSubmitHandlers(): void {
+ // Primary mode: resubmit-under-new-name is a daemon disposition with rename.
+ if (detectordPrimary()) {
+ return resubmitServerViaDetectord(params.originalName, params.newName, params.client ?? "");
+ }
+
</file context>
|
|
||
| mkdir -p "$OUT_DIR" | ||
|
|
||
| for target in aarch64-apple-darwin x86_64-apple-darwin; do |
There was a problem hiding this comment.
P3: This adds a near copy of the universal Rust build pipeline already implemented in build-stdiod.sh, so future build-flow fixes (targets, flags, signing, validation) can drift between daemons. Consider extracting shared logic into one helper script/function that takes daemon path/binary name parameters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/build-detectord.sh, line 36:
<comment>This adds a near copy of the universal Rust build pipeline already implemented in `build-stdiod.sh`, so future build-flow fixes (targets, flags, signing, validation) can drift between daemons. Consider extracting shared logic into one helper script/function that takes daemon path/binary name parameters.</comment>
<file context>
@@ -0,0 +1,57 @@
+
+mkdir -p "$OUT_DIR"
+
+for target in aarch64-apple-darwin x86_64-apple-darwin; do
+ if ! rustup target list --installed | grep -q "^${target}\$"; then
+ echo "Installing rustup target $target ..."
</file context>
| } from './protocol' | ||
|
|
||
| /** A daemon Reply that carried an { reply: 'error' } becomes a thrown Error. */ | ||
| export class DetectordError extends Error {} |
There was a problem hiding this comment.
P3: DetectordError doesn't set this.name, so instances report name === 'Error' instead of 'DetectordError', which makes log filtering and error-type checks less reliable.
Subclassing Error without overriding name is a common gotcha — JavaScript's Error.prototype.name defaults to 'Error'. Add a constructor to set this.name = 'DetectordError'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/detectord/socket.ts, line 27:
<comment>`DetectordError` doesn't set `this.name`, so instances report `name === 'Error'` instead of `'DetectordError'`, which makes log filtering and error-type checks less reliable.
Subclassing `Error` without overriding `name` is a common gotcha — JavaScript's `Error.prototype.name` defaults to `'Error'`. Add a constructor to set `this.name = 'DetectordError'`.</comment>
<file context>
@@ -0,0 +1,179 @@
+} from './protocol'
+
+/** A daemon Reply that carried an { reply: 'error' } becomes a thrown Error. */
+export class DetectordError extends Error {}
+
+type Pending = { resolve: (r: Reply) => void; reject: (e: Error) => void }
</file context>
| edisonSecretKey?: string | ||
| }): Promise<{ ok: boolean }> => ipcRenderer.invoke('detectord:enroll', input), | ||
| /** Register/adopt the org secret key when the user enters or changes it. */ | ||
| setSecret: (key: string): Promise<{ ok: boolean }> => |
There was a problem hiding this comment.
P3: The preload types setSecret as returning only { ok: boolean }, but the actual response includes outcome (with valid status) and reason fields. Although the extra fields are present at runtime (IPC passes through the full object), the narrow type prevents the renderer from using outcome.valid to confirm the secret was accepted by the daemon. Widen the preload type to match the actual return shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/preload/index.ts, line 245:
<comment>The preload types `setSecret` as returning only `{ ok: boolean }`, but the actual response includes `outcome` (with `valid` status) and `reason` fields. Although the extra fields are present at runtime (IPC passes through the full object), the narrow type prevents the renderer from using `outcome.valid` to confirm the secret was accepted by the daemon. Widen the preload type to match the actual return shape.</comment>
<file context>
@@ -232,6 +232,23 @@ const api = {
+ edisonSecretKey?: string
+ }): Promise<{ ok: boolean }> => ipcRenderer.invoke('detectord:enroll', input),
+ /** Register/adopt the org secret key when the user enters or changes it. */
+ setSecret: (key: string): Promise<{ ok: boolean }> =>
+ ipcRenderer.invoke('detectord:setSecret', key),
+ /** Uninstall the daemon; purge=true also deletes all its data + logs. */
</file context>
Suggested change
| setSecret: (key: string): Promise<{ ok: boolean }> => | |
| setSecret: (key: string): Promise<{ ok: boolean; outcome?: { valid?: boolean }; reason?: string }> => |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Introduce a bundled detector daemon (
edison-detectord) that takes over MCP discovery, quarantine, and onboarding flows, with a new batched approval dialog and automatic bootstrap/enroll on app start. This centralizes policy, adds stdio support, and lets the client mirror daemon activity into logs while standing down its own pipeline in primary mode.New Features
EW_DETECTORD_PRIMARY.list_servers(supports stdio) and falls back to local scan if unreachable/not enrolled.window.api.detectord.enroll,.setSecret, and.uninstall; renderer pushes credentials after sign-in, and Org Key changes are adopted by the daemon.scripts/build-detectord.shandnpm run build:detectordto produce an arm64 macOS binary; mac packaging targetsarm64(no x86_64/universal); clear-data teardown purges the daemon too.Migration
npm run build:detectord(orcargo build --release --target aarch64-apple-darwinindetectord/formcp_detector_daemon).EW_DETECTORD_PRIMARY=0to fall back to the existing TypeScript hooks/quarantine pipeline.Written for commit f651675. Summary will update on new commits.