From d3d60593cf999a38569a3cbbddc275f7b0d4c9b1 Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Mon, 12 Oct 2026 13:25:20 +0000 Subject: [PATCH] fix(devkitd): parse ps rows with padded columns on Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep parser split each `ps -axo` row on single whitespace chars, so procps-padded columns produced empty fields and every row was dropped — `agent list` always reported "No running agents detected" on Linux. Split pid/ppid/tty on whitespace runs and keep the remainder as the command, warn when a sweep parses zero rows from non-empty output, and stop treating an empty daemon answer as authoritative when local detection finds agents. Closes #371 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 19 ++++- packages/agent-manager/src/AgentManager.ts | 26 ++++-- .../src/__tests__/AgentManager.test.ts | 33 +++++++- .../src/__tests__/daemon-sweep.test.ts | 81 +++++++++++++++++++ rust/Cargo.lock | 1 + rust/crates/devkit-core/Cargo.toml | 1 + rust/crates/devkit-core/src/discover.rs | 57 +++++++++++-- 7 files changed, 203 insertions(+), 15 deletions(-) create mode 100644 packages/daemon-client/src/__tests__/daemon-sweep.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02bc9a255..80e3746e4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,20 @@ jobs: - name: Build run: npm run build + # devkitd powers the daemon-client integration tests; without a + # binary they silently skip and the daemon path gets no CI coverage + # (that's how #371 shipped). + - name: Setup Rust toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Cache cargo artifacts + uses: Swatinem/rust-cache@v2 + with: + workspaces: rust + + - name: Build devkitd for daemon-client integration tests + run: cargo build -p devkitd --manifest-path rust/Cargo.toml + - name: Test run: npm run test @@ -65,4 +79,7 @@ jobs: run: cargo fmt --all -- --check - name: Clippy - run: cargo clippy --workspace --all-targets -- -D warnings \ No newline at end of file + run: cargo clippy --workspace --all-targets -- -D warnings + + - name: Test + run: cargo test --workspace \ No newline at end of file diff --git a/packages/agent-manager/src/AgentManager.ts b/packages/agent-manager/src/AgentManager.ts index 4b8b0dcc3..ea5852e8c 100644 --- a/packages/agent-manager/src/AgentManager.ts +++ b/packages/agent-manager/src/AgentManager.ts @@ -61,10 +61,12 @@ export interface AgentManagerOptions { onRuntimeDiscoveryError?: (error: unknown) => void; /** * Daemon-side enriched agents ({@link DaemonClient.listAgents} wire - * shape: `AgentInfo` with `lastActive` as an ISO string). A non-null + * shape: `AgentInfo` with `lastActive` as an ISO string). A non-empty * result is authoritative for every harness type — all local adapters - * are skipped. Returning null or throwing forces the local path; - * omitting it disables daemon consumption. + * are skipped. An empty result is cross-checked against local detection + * (local wins on disagreement, with a warning). Returning null or + * throwing forces the local path; omitting it disables daemon + * consumption. */ fetchEnrichedAgents?: () => Promise & { @@ -190,9 +192,14 @@ export class AgentManager { const allAgents: AgentInfo[] = []; const errors: Array<{ type: string; error: Error }> = []; - // A non-null daemon answer is authoritative for every harness type, so - // local adapters only run when the daemon is absent. + // A non-empty daemon answer is authoritative for every harness type, so + // local adapters only run when the daemon is absent — or when it + // returns an empty list. An empty answer must not be authoritative + // (issue #371): a daemon-side discovery bug would otherwise mask every + // running agent forever. When local adapters disagree with the empty + // answer the local result wins, with a warning. let daemonAgents: AgentInfo[] | null = null; + let daemonAnsweredEmpty = false; if (this.options.fetchEnrichedAgents) { try { const enriched = await this.options.fetchEnrichedAgents(); @@ -203,6 +210,8 @@ export class AgentManager { status: a.status as AgentStatus, lastActive: new Date(a.lastActive), })); + daemonAnsweredEmpty = daemonAgents.length === 0; + if (daemonAnsweredEmpty) daemonAgents = null; } } catch { daemonAgents = null; @@ -264,6 +273,13 @@ export class AgentManager { } } + if (daemonAnsweredEmpty && allAgents.length > 0) { + console.error( + `Warning: devkitd reported no agents but local detection found ${allAgents.length} — ` + + "the daemon's process sweep may be broken; showing local results.", + ); + } + // Log errors if any (but don't throw - partial results are useful) if (errors.length > 0) { console.error(`Warning: ${errors.length} adapter(s) failed:`); diff --git a/packages/agent-manager/src/__tests__/AgentManager.test.ts b/packages/agent-manager/src/__tests__/AgentManager.test.ts index 599995b81..c9c13e951 100644 --- a/packages/agent-manager/src/__tests__/AgentManager.test.ts +++ b/packages/agent-manager/src/__tests__/AgentManager.test.ts @@ -548,17 +548,42 @@ describe("AgentManager", () => { expect(agents.map((a) => a.name).sort()).toEqual(["daemon-claude", "daemon-codex"]); }); - it("treats an empty daemon answer as authoritative", async () => { + it("falls back to local adapters when the daemon returns an empty list and local finds agents", async () => { const local = new MockAdapter("claude", [createMockAgent({ name: "local-claude" })]); const detectSpy = vi.spyOn(local, "detectAgents"); + const warnSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const m = new AgentManager(new AgentRegistry(path.join(tmpDir, "agents.json")), undefined, { fetchEnrichedAgents: async () => [], }); m.registerAdapter(local); - const agents = await m.listAgents(); - expect(detectSpy).not.toHaveBeenCalled(); - expect(agents).toEqual([]); + try { + const agents = await m.listAgents(); + expect(detectSpy).toHaveBeenCalled(); + expect(agents.map((a) => a.name)).toEqual(["local-claude"]); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("daemon")); + } finally { + warnSpy.mockRestore(); + } + }); + + it("returns empty without a warning when the daemon and local adapters are both empty", async () => { + const local = new MockAdapter("claude"); + const detectSpy = vi.spyOn(local, "detectAgents"); + const warnSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const m = new AgentManager(new AgentRegistry(path.join(tmpDir, "agents.json")), undefined, { + fetchEnrichedAgents: async () => [], + }); + m.registerAdapter(local); + + try { + const agents = await m.listAgents(); + expect(detectSpy).toHaveBeenCalled(); + expect(agents).toEqual([]); + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } }); }); diff --git a/packages/daemon-client/src/__tests__/daemon-sweep.test.ts b/packages/daemon-client/src/__tests__/daemon-sweep.test.ts new file mode 100644 index 000000000..44dbc02d8 --- /dev/null +++ b/packages/daemon-client/src/__tests__/daemon-sweep.test.ts @@ -0,0 +1,81 @@ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { spawn, type ChildProcess } from "node:child_process"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { DaemonClient } from "../client.js"; + +const repoRoot = path.resolve(path.dirname(new URL(import.meta.url).pathname), "../../../.."); +const devBin = [ + path.join(repoRoot, "rust/target/release/devkitd"), + path.join(repoRoot, "rust/target/debug/devkitd"), +].find((p) => fs.existsSync(p)); +const bin = process.env.DEVKITD_BIN ?? process.env.AI_DEVKITD_BIN ?? devBin; + +/** + * Issue #371 regression coverage at the daemon boundary: a stub `ps` on + * PATH feeds devkitd synthetic procps output with PADDED columns — the + * exact shape that used to make every row unparseable, leaving + * `agent.list` permanently empty. Skipped when no devkitd binary exists + * (CI builds it in the test job). + */ +describe.runIf(bin != null && process.platform !== "win32")("devkitd sweep with stub ps", () => { + let home: string; + let stubDir: string; + let sock: string; + let daemon: ChildProcess | null = null; + + const PADDED_PS = " 424242 1 pts/1 claude\n 424243 424242 pts/2 gemini\n"; + + beforeAll(async () => { + home = fs.mkdtempSync(path.join(os.tmpdir(), "devkitd-ps-e2e-")); + stubDir = path.join(home, "stub-bin"); + fs.mkdirSync(stubDir); + const stub = path.join(stubDir, "ps"); + fs.writeFileSync(stub, `#!/bin/sh\nprintf '%s' '${PADDED_PS}'\n`); + fs.chmodSync(stub, 0o755); + + sock = path.join(home, ".ai-devkit", "daemon.sock"); + daemon = spawn(bin!, ["serve"], { + env: { + ...process.env, + HOME: home, + PATH: `${stubDir}${path.delimiter}${process.env.PATH}`, + RUST_LOG: "warn", + }, + stdio: "ignore", + }); + daemon.unref(); + + const deadline = Date.now() + 10_000; + while (!fs.existsSync(sock)) { + if (daemon.exitCode !== null) throw new Error("devkitd exited before binding its socket"); + if (Date.now() > deadline) throw new Error("timed out waiting for daemon socket"); + await new Promise((r) => setTimeout(r, 50)); + } + }, 15_000); + + afterAll(async () => { + if (daemon) { + const c = await DaemonClient.tryConnect(sock).catch(() => null); + if (c) { + await c.request("shutdown").catch(() => {}); + c.close(); + } + daemon.kill("SIGKILL"); + } + fs.rmSync(home, { recursive: true, force: true }); + }); + + it("agent.list reports agents from padded procps rows", async () => { + const client = await DaemonClient.connect(sock); + try { + const agents = await client.listAgents(); + const pids = agents.map((a) => a.pid); + expect(pids).toContain(424242); + expect(agents.find((a) => a.pid === 424242)?.type).toBe("claude"); + } finally { + client.close(); + } + }, 15_000); +}); diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 3e9939aa4..64e59c320 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -94,6 +94,7 @@ dependencies = [ "rusqlite", "serde", "serde_json", + "tracing", "ts-rs", ] diff --git a/rust/crates/devkit-core/Cargo.toml b/rust/crates/devkit-core/Cargo.toml index 0b52593b8..5c0970813 100644 --- a/rust/crates/devkit-core/Cargo.toml +++ b/rust/crates/devkit-core/Cargo.toml @@ -10,4 +10,5 @@ serde_json.workspace = true rusqlite.workspace = true anyhow.workspace = true libc.workspace = true +tracing.workspace = true ts-rs = "12" diff --git a/rust/crates/devkit-core/src/discover.rs b/rust/crates/devkit-core/src/discover.rs index 5cee4bc29..5cdb502c9 100644 --- a/rust/crates/devkit-core/src/discover.rs +++ b/rust/crates/devkit-core/src/discover.rs @@ -102,6 +102,12 @@ pub fn sweep() -> Option> { } let text = String::from_utf8_lossy(&out.stdout); let mut procs = parse_ps_output(&text, true); + if procs.is_empty() && !text.trim().is_empty() { + // A non-empty process table that yields zero rows means the parser + // regressed against this platform's ps format (see #371) — surface + // it instead of silently reporting an empty agent list forever. + tracing::warn!("sweep parsed 0 processes from non-empty ps output"); + } procs.sort_by_key(|p| p.pid); Some(procs) } @@ -112,11 +118,11 @@ pub fn sweep() -> Option> { fn parse_ps_output(text: &str, resolve_cwd: bool) -> Vec { text.lines() .filter_map(|line| { - let mut it = line.trim().splitn(4, char::is_whitespace); - let pid: i64 = it.next()?.parse().ok()?; - let ppid: i64 = it.next()?.parse().ok()?; - let tty = it.next().map(|s| s.to_string()); - let command = it.next().unwrap_or("").trim().to_string(); + let (pid, ppid, tty, command) = split_ps_row(line)?; + let pid: i64 = pid.parse().ok()?; + let ppid: i64 = ppid.parse().ok()?; + let tty = Some(tty.to_string()); + let command = command.trim().to_string(); if !is_agent_command(&command) { return None; } @@ -133,6 +139,22 @@ fn parse_ps_output(text: &str, resolve_cwd: bool) -> Vec { .collect() } +/// Split a `ps` row into pid/ppid/tty plus the raw command remainder. +/// procps pads columns with runs of spaces, so fields must be split on +/// whitespace RUNS — a per-char split yields empty fields that fail to +/// parse and drops the row (issue #371). The command keeps its original +/// inner spacing. +fn split_ps_row(line: &str) -> Option<(&str, &str, &str, &str)> { + let mut rest = line.trim_start(); + let mut fields = ["", "", ""]; + for f in &mut fields { + let end = rest.find(char::is_whitespace)?; + *f = &rest[..end]; + rest = rest[end..].trim_start(); + } + Some((fields[0], fields[1], fields[2], rest)) +} + fn cwd_of(pid: i64) -> Option { // /proc readlink — one syscall, no lsof/pwdx spawn. std::fs::read_link(format!("/proc/{pid}/cwd")) @@ -306,6 +328,31 @@ mod tests { // Bare name (pi) and full path (claude) both match via basename. } + #[test] + fn issue_371_padded_column_rows_all_parse() { + // The exact matrix from codeaholicguy/ai-devkit#371: Linux procps + // right-aligns pid/ppid and pads columns with runs of spaces, so a + // per-char whitespace split produces empty fields and drops the row. + for line in [ + "61524 60968 pts/1 claude", + " 61524 60968 pts/1 claude", + "61524 60968 pts/1 claude", + "61524 60968 pts/1 claude", + " 61524 60968 pts/1 claude", + ] { + assert_eq!(parse_ps_output(line, false).len(), 1, "row {line:?}"); + } + } + + #[test] + fn command_field_keeps_inner_spacing() { + let procs = parse_ps_output(" 61524 60968 pts/1 claude --resume abc123", false); + assert_eq!( + procs[0].command.as_deref(), + Some("claude --resume abc123") + ); + } + #[test] fn malformed_lines_are_skipped() { let procs = parse_ps_output("garbage\n abc x ? y\n", false);