From 18e508f30299ea38acf5b71f83cb17fbfdc60abd Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:46:15 -0400 Subject: [PATCH 1/2] fix: deterministic analytics opt-out test, caret assertion, non-blocking Wayland clipboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs ADE-142, ADE-143, ADE-144 ADE-142: the cross-process opt-out test raced a real fs watcher via a real-timer `vi.waitFor`. `getStatus()` already runs `reconcileOptOutMarker()` synchronously — the same code path the watcher callback runs — so drive it explicitly and assert synchronously. Applied to its twin (explicit opt-in after a shared opt-out), which had the identical shape. ADE-143: the caret is an inverse-video cell after the last character, and `PROMPT_ROW_CHROME_CELLS` (7) already reserves a column for it, so the row does fit ONE terminal line — production was right. The assertion used `.trim()`, which cannot strip a space wrapped in SGR escapes, so it could never match. Strip the escapes before comparing and additionally assert the caret cell is present on that same line. ADE-144: `wl-copy` (and xclip/xsel) fork a daemon that owns the selection until the clipboard is overwritten. With spawnSync's default piped stdio the daemon inherits our stdout/stderr, so spawnSync waits for those pipes to close and `ade report-issue --open` / TUI `/report-issue` hang. Discard stdout/stderr and pass a bounded 3s timeout so the call is always finite; callers already fall back to printing the text/URL on `false`. The `which`/`where` probe gets the same bound. New unit test runs a real never-exiting child through production's own spawn options and asserts the call returns with the fallback result. Co-Authored-By: Claude Fable 5 --- apps/ade-cli/src/lib/clipboard.test.ts | 64 +++++++++++++++++++ apps/ade-cli/src/lib/clipboard.ts | 41 +++++++++++- .../tuiClient/__tests__/promptWrap.test.tsx | 13 +++- .../analytics/productAnalyticsService.test.ts | 18 +++--- 4 files changed, 122 insertions(+), 14 deletions(-) create mode 100644 apps/ade-cli/src/lib/clipboard.test.ts diff --git a/apps/ade-cli/src/lib/clipboard.test.ts b/apps/ade-cli/src/lib/clipboard.test.ts new file mode 100644 index 000000000..31d746648 --- /dev/null +++ b/apps/ade-cli/src/lib/clipboard.test.ts @@ -0,0 +1,64 @@ +import { spawnSync } from "node:child_process"; +import { describe, expect, it } from "vitest"; + +import { copyToClipboard, type CopyToClipboardSpawnOptions } from "./clipboard"; + +describe("copyToClipboard", () => { + it("discards the helper's stdout/stderr so a daemonizing wl-copy cannot hold our pipes", () => { + const seen: CopyToClipboardSpawnOptions[] = []; + const ok = copyToClipboard("ade://lane/x", { + platform: "linux", + commandExists: (cmd) => cmd === "wl-copy", + spawn: (cmd, args, opts) => { + expect(cmd).toBe("wl-copy"); + expect(args).toEqual([]); + seen.push(opts); + return { status: 0 }; + }, + }); + expect(ok).toBe(true); + expect(seen[0]?.stdio).toEqual(["pipe", "ignore", "ignore"]); + expect(seen[0]?.timeout).toBeGreaterThan(0); + expect(seen[0]?.input).toBe("ade://lane/x"); + }); + + it("returns the fallback result instead of hanging when the helper never exits", () => { + // The real Wayland failure: the helper keeps running after taking the text. + // Production hands spawnSync a bounded timeout, so the call must come back + // (as `false`, which callers surface as "here's the URL") rather than block. + const started = Date.now(); + const ok = copyToClipboard("ade://lane/x", { + platform: "linux", + commandExists: () => true, + timeoutMs: 300, + // Real child, real spawnSync, production's own options: reads stdin then + // lingers the way a clipboard daemon does. + spawn: (_cmd, _args, opts) => spawnSync("sh", ["-c", "cat >/dev/null; sleep 30"], opts), + }); + expect(ok).toBe(false); + expect(Date.now() - started).toBeLessThan(10_000); + }); + + it("reports failure when no Linux clipboard tool is installed", () => { + expect( + copyToClipboard("x", { + platform: "linux", + commandExists: () => false, + spawn: () => { + throw new Error("must not spawn"); + }, + }), + ).toBe(false); + }); + + it("treats a throwing spawn as a copy failure", () => { + expect( + copyToClipboard("x", { + platform: "darwin", + spawn: () => { + throw new Error("EPERM"); + }, + }), + ).toBe(false); + }); +}); diff --git a/apps/ade-cli/src/lib/clipboard.ts b/apps/ade-cli/src/lib/clipboard.ts index 1a76f9de7..108b28760 100644 --- a/apps/ade-cli/src/lib/clipboard.ts +++ b/apps/ade-cli/src/lib/clipboard.ts @@ -3,17 +3,38 @@ // // Picks the right system clipboard binary for darwin (pbcopy), win32 (clip), // or Linux (wl-copy / xclip). Returns `false` when no usable binary is found -// instead of throwing — callers decide how to surface the failure. +// or when the helper does not finish in time, instead of throwing — callers +// decide how to surface the failure (they print the text/URL instead). +// +// Wayland caveat: `wl-copy` forks a daemon that owns the selection until the +// clipboard is overwritten. If that daemon inherits our stdout/stderr pipes, +// `spawnSync` waits for those pipes to close and blocks for as long as the +// selection lives — `ade report-issue --open` and the TUI `/report-issue` +// keybinds would hang forever. Discarding stdout/stderr (so the daemon holds +// no pipe of ours) plus a bounded timeout keeps the call finite on every +// platform. `xclip`/`xsel` daemonize the same way, so they share the shape. // --------------------------------------------------------------------------- import { spawnSync } from "node:child_process"; +/** Upper bound on how long a clipboard helper may run before we give up. */ +export const CLIPBOARD_TIMEOUT_MS = 3_000; + +export type CopyToClipboardSpawnOptions = { + input: string; + windowsHide?: boolean; + /** stdin is piped so we can hand over the text; stdout/stderr are discarded. */ + stdio?: Array<"pipe" | "ignore">; + /** Bounded runtime; spawnSync kills the child and reports an error past it. */ + timeout?: number; +}; + export type CopyToClipboardOptions = { /** * Test seam: override the spawn function. The override must return the * same shape as `spawnSync` (status + error). Defaults to `spawnSync`. */ - spawn?: (cmd: string, args: string[], options: { input: string; windowsHide?: boolean }) => { + spawn?: (cmd: string, args: string[], options: CopyToClipboardSpawnOptions) => { error?: Error; status?: number | null; }; @@ -24,12 +45,15 @@ export type CopyToClipboardOptions = { commandExists?: (cmd: string) => boolean; /** Test seam: override `process.platform`. */ platform?: NodeJS.Platform; + /** Test seam: shorten the bounded timeout. */ + timeoutMs?: number; }; export function copyToClipboard(text: string, options: CopyToClipboardOptions = {}): boolean { const platform = options.platform ?? process.platform; const spawn = options.spawn ?? ((cmd, args, opts) => spawnSync(cmd, args, opts)); const commandExists = options.commandExists ?? defaultCommandExists; + const timeout = options.timeoutMs ?? CLIPBOARD_TIMEOUT_MS; let cmd: string; let args: string[]; @@ -50,7 +74,17 @@ export function copyToClipboard(text: string, options: CopyToClipboardOptions = return false; } } - const r = spawn(cmd, args, { input: text, windowsHide: true }); + let r: { error?: Error; status?: number | null }; + try { + r = spawn(cmd, args, { + input: text, + windowsHide: true, + stdio: ["pipe", "ignore", "ignore"], + timeout, + }); + } catch { + return false; + } if (r.error || (typeof r.status === "number" && r.status !== 0)) return false; return true; } @@ -59,6 +93,7 @@ function defaultCommandExists(cmd: string): boolean { const r = spawnSync(process.platform === "win32" ? "where" : "which", [cmd], { stdio: "ignore", windowsHide: true, + timeout: CLIPBOARD_TIMEOUT_MS, }); return !r.error && r.status === 0; } diff --git a/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx b/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx index 43cf3889f..6a98cb482 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx +++ b/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx @@ -56,6 +56,11 @@ function PromptRows({ ); } +/** Strips SGR escapes (e.g. the inverse-video caret cell) for text compares. */ +function stripAnsi(text: string): string { + return text.replace(/\u001B\[[0-9;]*m/g, ""); +} + /** Body lines of the rendered box, with the border/padding stripped. */ function boxBodyLines(frame: string): string[] { return frame @@ -93,8 +98,12 @@ describe("prompt wrap budget", () => { ); const body = boxBodyLines(lastFrame() ?? ""); expect(body).toHaveLength(1); - // Every character survives; the old budget dropped the final one. - expect(body[0]!.trim()).toBe(`› ${text}`); + // The caret is an inverse-video cell rendered *after* the last character; + // PROMPT_ROW_CHROME_CELLS reserves a column for it, so the row still fits + // ONE terminal line. The old assertion used `.trim()`, which cannot strip a + // space wrapped in SGR escapes, so it never matched — strip the escapes. + expect(stripAnsi(body[0]!).trim()).toBe(`› ${text}`); + expect(body[0]).toContain("\u001B[7m \u001B[27m"); }); it("reports when a trailing hint still fits beside a short row", () => { diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts index 1debac419..3dde9e6fb 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts @@ -844,9 +844,11 @@ describe("productAnalyticsService", () => { const second = makeHarness({ root: first.root }); expect(second.service.setEnabled(false)).toMatchObject({ enabled: false, effective: false }); - await vi.waitFor(() => { - expect(first.shutdownArgs).toEqual([[1_500, { flush: false }]]); - }, { timeout: 3_000, interval: 20 }); + // Drive the reconcile explicitly instead of racing the cross-process fs + // watcher: getStatus() re-reads the shared opt-out marker synchronously via + // the same reconcileOptOutMarker() the watcher callback runs. + expect(first.service.getStatus()).toMatchObject({ enabled: false, effective: false }); + expect(first.shutdownArgs).toEqual([[1_500, { flush: false }]]); await expect(first.service.flush()).resolves.toBe(true); await first.service.shutdown(); @@ -860,14 +862,12 @@ describe("productAnalyticsService", () => { const second = makeHarness({ root: first.root }); expect(second.service.setEnabled(false)).toMatchObject({ enabled: false, effective: false }); - await vi.waitFor(() => { - expect(first.shutdownArgs).toEqual([[1_500, { flush: false }]]); - }, { timeout: 3_000, interval: 20 }); + // Same synchronous reconcile as above rather than a real-timer watcher race. + expect(first.service.getStatus()).toMatchObject({ enabled: false, effective: false }); + expect(first.shutdownArgs).toEqual([[1_500, { flush: false }]]); expect(second.service.setEnabled(true)).toMatchObject({ enabled: true, effective: true }); - await vi.waitFor(() => { - expect(first.service.getStatus()).toMatchObject({ enabled: true, effective: true }); - }, { timeout: 3_000, interval: 20 }); + expect(first.service.getStatus()).toMatchObject({ enabled: true, effective: true }); expect(first.service.capture({ event: "ade_screen_viewed", surface: "desktop" })).toEqual({ accepted: true, reason: "accepted", From ee9cde0960edfd9f52effa12f2ff2b77079a87b8 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:59:05 -0400 Subject: [PATCH 2/2] =?UTF-8?q?ship:=20iteration=201=20=E2=80=94=20quality?= =?UTF-8?q?=20revalidation=20(color-agnostic=20caret=20assertion,=20Window?= =?UTF-8?q?s-safe=20clipboard=20child)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - promptWrap.test.tsx: drop the raw SGR assertion. It pinned chalk's inverse-video bytes, which only exist when chalk detects color support, so it failed under CI's non-TTY worker (test-ade-cli was red for exactly this). The stripAnsi compare already proves the caret cell fits on one terminal line. - clipboard.test.ts: spawn the lingering child via process.execPath instead of `sh -c`, so the test runs on Windows and spawnSync's timeout kills the process that actually lingers (a shell forked and orphaned the sleeper). - clipboard.test.ts: drop the no-Linux-tool case, already covered in deeplinkKeybind.test.ts. - clipboard.ts: CLIPBOARD_TIMEOUT_MS has no external consumer; unexport it. Fix the header comment, which named xsel as a fallback the code never tries. - productAnalyticsService.test.ts: rename the two tests so they describe the status-read reconcile they now assert rather than implying watcher coverage. --- apps/ade-cli/src/lib/clipboard.test.ts | 20 ++++++------------- apps/ade-cli/src/lib/clipboard.ts | 4 ++-- .../tuiClient/__tests__/promptWrap.test.tsx | 6 ++++-- .../analytics/productAnalyticsService.test.ts | 4 ++-- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/apps/ade-cli/src/lib/clipboard.test.ts b/apps/ade-cli/src/lib/clipboard.test.ts index 31d746648..dd50d92c4 100644 --- a/apps/ade-cli/src/lib/clipboard.test.ts +++ b/apps/ade-cli/src/lib/clipboard.test.ts @@ -32,25 +32,17 @@ describe("copyToClipboard", () => { commandExists: () => true, timeoutMs: 300, // Real child, real spawnSync, production's own options: reads stdin then - // lingers the way a clipboard daemon does. - spawn: (_cmd, _args, opts) => spawnSync("sh", ["-c", "cat >/dev/null; sleep 30"], opts), + // lingers the way a clipboard daemon does. Spawned via `process.execPath` + // rather than `sh -c` so it runs on Windows too, and so the process + // spawnSync's timeout kills IS the lingering one (a shell would fork the + // sleeper and orphan it). + spawn: (_cmd, _args, opts) => + spawnSync(process.execPath, ["-e", "process.stdin.resume(); setTimeout(() => {}, 30_000);"], opts), }); expect(ok).toBe(false); expect(Date.now() - started).toBeLessThan(10_000); }); - it("reports failure when no Linux clipboard tool is installed", () => { - expect( - copyToClipboard("x", { - platform: "linux", - commandExists: () => false, - spawn: () => { - throw new Error("must not spawn"); - }, - }), - ).toBe(false); - }); - it("treats a throwing spawn as a copy failure", () => { expect( copyToClipboard("x", { diff --git a/apps/ade-cli/src/lib/clipboard.ts b/apps/ade-cli/src/lib/clipboard.ts index 108b28760..98a905826 100644 --- a/apps/ade-cli/src/lib/clipboard.ts +++ b/apps/ade-cli/src/lib/clipboard.ts @@ -12,13 +12,13 @@ // selection lives — `ade report-issue --open` and the TUI `/report-issue` // keybinds would hang forever. Discarding stdout/stderr (so the daemon holds // no pipe of ours) plus a bounded timeout keeps the call finite on every -// platform. `xclip`/`xsel` daemonize the same way, so they share the shape. +// platform. `xclip`, the one X11 fallback we try, daemonizes the same way. // --------------------------------------------------------------------------- import { spawnSync } from "node:child_process"; /** Upper bound on how long a clipboard helper may run before we give up. */ -export const CLIPBOARD_TIMEOUT_MS = 3_000; +const CLIPBOARD_TIMEOUT_MS = 3_000; export type CopyToClipboardSpawnOptions = { input: string; diff --git a/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx b/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx index 6a98cb482..fef313355 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx +++ b/apps/ade-cli/src/tuiClient/__tests__/promptWrap.test.tsx @@ -101,9 +101,11 @@ describe("prompt wrap budget", () => { // The caret is an inverse-video cell rendered *after* the last character; // PROMPT_ROW_CHROME_CELLS reserves a column for it, so the row still fits // ONE terminal line. The old assertion used `.trim()`, which cannot strip a - // space wrapped in SGR escapes, so it never matched — strip the escapes. + // space wrapped in SGR escapes — so it passed only when chalk detected no + // color support and failed whenever Ink actually emitted escapes. That + // inconsistency was the flake. Strip the escapes so the assertion holds + // either way; asserting the raw SGR bytes would just invert the coupling. expect(stripAnsi(body[0]!).trim()).toBe(`› ${text}`); - expect(body[0]).toContain("\u001B[7m \u001B[27m"); }); it("reports when a trailing hint still fits beside a short row", () => { diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts index 3dde9e6fb..01d276776 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts @@ -838,7 +838,7 @@ describe("productAnalyticsService", () => { fs.rmSync(harness.root, { recursive: true, force: true }); }); - it("cancels another process's queued client when the shared opt-out marker appears", async () => { + it("cancels another process's queued client on the next status read after the shared opt-out marker appears", async () => { const first = makeHarness(); expect(first.service.capture({ event: "ade_app_opened", surface: "desktop" }).accepted).toBe(true); @@ -856,7 +856,7 @@ describe("productAnalyticsService", () => { fs.rmSync(first.root, { recursive: true, force: true }); }); - it("honors another process's explicit opt-in after a shared opt-out", async () => { + it("honors another process's explicit opt-in after a shared opt-out on the next status read", async () => { const first = makeHarness(); expect(first.service.capture({ event: "ade_app_opened", surface: "desktop" }).accepted).toBe(true);