From 3cea63cf293bbf59500d716cd8d308b670ccfc2e Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 17 Apr 2026 16:44:49 +0200 Subject: [PATCH] fix(coding-agent): resolve captured commands on close closes #3027 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/core/package-manager.ts | 44 +++++++++++------- .../coding-agent/test/package-manager.test.ts | 45 +++++++++++++++++++ 3 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 91111d88e..51935979f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed flaky git package update notifications by waiting for captured git command stdio to fully drain before comparing local and remote commit SHAs ([#3027](https://github.com/badlogic/pi-mono/issues/3027)) - Fixed auto-retry transient error detection to treat `Network connection lost.` as retryable, so dropped provider connections retry instead of terminating the agent ([#3317](https://github.com/badlogic/pi-mono/issues/3317)) - Fixed compact interactive extension startup summaries to disambiguate package extensions and repeated local `index.ts` entries by using package-aware labels and the minimal parent path needed to make local entries unique ([#3308](https://github.com/badlogic/pi-mono/issues/3308)) - Fixed git package dependency installation to use production installs (`npm install --omit=dev`) during both install and update flows, so extension runtime dependencies must come from `dependencies` and not `devDependencies` ([#3009](https://github.com/badlogic/pi-mono/issues/3009)) diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 32f147b6d..0100e247a 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -1,8 +1,9 @@ -import { spawn, spawnSync } from "node:child_process"; +import { type ChildProcess, type ChildProcessByStdio, spawn, spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { basename, dirname, join, relative, resolve, sep } from "node:path"; +import type { Readable } from "node:stream"; import ignore from "ignore"; import { minimatch } from "minimatch"; import { CONFIG_DIR_NAME } from "../config.js"; @@ -2177,18 +2178,34 @@ export class DefaultPackageManager implements PackageManager { }; } + private spawnCommand(command: string, args: string[], options?: { cwd?: string }): ChildProcess { + return spawn(command, args, { + cwd: options?.cwd, + stdio: isStdoutTakenOver() ? ["ignore", 2, 2] : "inherit", + shell: process.platform === "win32", + }); + } + + private spawnCaptureCommand( + command: string, + args: string[], + options?: { cwd?: string; env?: Record }, + ): ChildProcessByStdio { + return spawn(command, args, { + cwd: options?.cwd, + stdio: ["ignore", "pipe", "pipe"], + shell: process.platform === "win32", + env: options?.env ? { ...process.env, ...options.env } : process.env, + }); + } + private runCommandCapture( command: string, args: string[], options?: { cwd?: string; timeoutMs?: number; env?: Record }, ): Promise { return new Promise((resolvePromise, reject) => { - const child = spawn(command, args, { - cwd: options?.cwd, - stdio: ["ignore", "pipe", "pipe"], - shell: process.platform === "win32", - env: options?.env ? { ...process.env, ...options.env } : process.env, - }); + const child = this.spawnCaptureCommand(command, args, options); let stdout = ""; let stderr = ""; let timedOut = false; @@ -2206,11 +2223,11 @@ export class DefaultPackageManager implements PackageManager { child.stderr?.on("data", (data) => { stderr += data.toString(); }); - child.on("error", (error) => { + child.once("error", (error) => { if (timeout) clearTimeout(timeout); reject(error); }); - child.on("exit", (code) => { + child.once("close", (code, signal) => { if (timeout) clearTimeout(timeout); if (timedOut) { reject(new Error(`${command} ${args.join(" ")} timed out after ${options?.timeoutMs}ms`)); @@ -2220,18 +2237,15 @@ export class DefaultPackageManager implements PackageManager { resolvePromise(stdout.trim()); return; } - reject(new Error(`${command} ${args.join(" ")} failed with code ${code}: ${stderr || stdout}`)); + const exitStatus = code === null ? `signal ${signal ?? "unknown"}` : `code ${code}`; + reject(new Error(`${command} ${args.join(" ")} failed with ${exitStatus}: ${stderr || stdout}`)); }); }); } private runCommand(command: string, args: string[], options?: { cwd?: string }): Promise { return new Promise((resolvePromise, reject) => { - const child = spawn(command, args, { - cwd: options?.cwd, - stdio: isStdoutTakenOver() ? ["ignore", 2, 2] : "inherit", - shell: process.platform === "win32", - }); + const child = this.spawnCommand(command, args, options); child.on("error", reject); child.on("exit", (code) => { if (code === 0) { diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index 0cda883c4..e8521ac92 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -1,6 +1,8 @@ +import { EventEmitter } from "node:events"; import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, relative } from "node:path"; +import { PassThrough } from "node:stream"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { DefaultPackageManager, type ProgressEvent, type ResolvedResource } from "../src/core/package-manager.js"; import { SettingsManager } from "../src/core/settings-manager.js"; @@ -13,6 +15,16 @@ function pathEndsWith(actualPath: string, suffix: string): boolean { return normalizeForMatch(actualPath).endsWith(normalizeForMatch(suffix)); } +class MockSpawnedProcess extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + + kill(): boolean { + this.emit("close", null, "SIGTERM"); + return true; + } +} + // Helper to check if a resource is enabled const isEnabled = (r: ResolvedResource, pathMatch: string, matchFn: "endsWith" | "includes" = "endsWith") => { const normalizedPath = normalizeForMatch(r.path); @@ -1553,5 +1565,38 @@ export default function(api) { api.registerTool({ name: "test", description: "te expect.objectContaining({ cwd: tempDir }), ); }); + + it("should wait for close before resolving captured stdout", async () => { + const managerWithInternals = packageManager as unknown as { + spawnCaptureCommand( + command: string, + args: string[], + options?: { cwd?: string; env?: Record }, + ): MockSpawnedProcess; + runCommandCapture( + command: string, + args: string[], + options?: { cwd?: string; timeoutMs?: number; env?: Record }, + ): Promise; + }; + const child = new MockSpawnedProcess(); + vi.spyOn(managerWithInternals, "spawnCaptureCommand").mockReturnValue(child); + + let settled = false; + const capturePromise = managerWithInternals.runCommandCapture("git", ["rev-parse", "HEAD"]).then((value) => { + settled = true; + return value; + }); + + child.emit("exit", 0, null); + await Promise.resolve(); + expect(settled).toBe(false); + + child.stdout.write("abc123\n"); + child.stdout.end(); + child.emit("close", 0, null); + + await expect(capturePromise).resolves.toBe("abc123"); + }); }); });