From cd4bd7c6c0ca860babbcbf0b68a066f3627ad547 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 1 Aug 2026 05:00:05 -0700 Subject: [PATCH] fix(tui): cancel buffered submissions on shutdown [AI-assisted] (#117331) * fix(tui): cancel buffered submissions on shutdown * refactor(tui): preserve callable submit coalescer * test(tui): run local PTY smoke against built CLI --- .github/workflows/ci.yml | 16 ++- docs/ci.md | 2 +- docs/reference/test.md | 2 +- scripts/lib/ci-node-test-plan.mjs | 3 +- src/tui/tui-pty-harness.e2e.test.ts | 34 +++++ src/tui/tui-pty-local.e2e.test.ts | 192 +++++++++++++------------ src/tui/tui-pty-test-support.test.ts | 59 +++++++- src/tui/tui-pty-test-support.ts | 55 ++++--- src/tui/tui-submit.ts | 16 ++- src/tui/tui.submit-handler.test.ts | 33 ++++- src/tui/tui.ts | 8 +- test/scripts/ci-node-test-plan.test.ts | 10 +- 12 files changed, 301 insertions(+), 129 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 272a8ab9eb82..f13c1b58cd51 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1063,6 +1063,7 @@ jobs: channels-result: ${{ steps.built_artifact_checks.outputs['channels-result'] }} core-support-boundary-result: ${{ steps.built_artifact_checks.outputs['core-support-boundary-result'] }} gateway-watch-result: ${{ steps.built_artifact_checks.outputs['gateway-watch-result'] }} + tui-pty-result: ${{ steps.built_artifact_checks.outputs['tui-pty-result'] }} steps: - *linux_node_checkout_step - name: Ensure secrets base commit (PR fast path) @@ -1193,6 +1194,7 @@ jobs: RUN_CHANNELS: ${{ needs.preflight.outputs.run_checks }} RUN_CORE_SUPPORT_BOUNDARY: ${{ needs.preflight.outputs.run_checks_node_core_dist }} RUN_GATEWAY_WATCH: ${{ needs.preflight.outputs.run_check_additional }} + RUN_TUI_PTY: ${{ needs.preflight.outputs.run_checks_node_core_dist }} shell: bash run: | set -uo pipefail @@ -1204,6 +1206,7 @@ jobs: ["channels"]="skipped" ["core-support-boundary"]="skipped" ["gateway-watch"]="skipped" + ["tui-pty"]="skipped" ) start_check() { @@ -1254,6 +1257,15 @@ jobs: node scripts/run-vitest.mjs run --config test/vitest/vitest.full-core-support-boundary.config.ts fi + if [ "$RUN_TUI_PTY" = "true" ]; then + start_check "tui-pty" env \ + NODE_OPTIONS=--max-old-space-size=8192 \ + OPENCLAW_TUI_PTY_INCLUDE_LOCAL=1 \ + OPENCLAW_TUI_PTY_USE_BUILT_CLI=1 \ + OPENCLAW_VITEST_MAX_WORKERS=2 \ + node scripts/run-vitest.mjs run --config test/vitest/vitest.tui-pty.config.ts + fi + if [ "$RUN_GATEWAY_WATCH" = "true" ] && [ "$PARALLEL_GATEWAY_WATCH" = "true" ]; then start_check "gateway-watch" \ node scripts/check-gateway-watch-regression.mjs --skip-build @@ -1269,12 +1281,12 @@ jobs: wait_checks fi - for name in channels core-support-boundary gateway-watch; do + for name in channels core-support-boundary gateway-watch tui-pty; do echo "${name}-result=${results[$name]}" >> "$GITHUB_OUTPUT" done failures=0 - for name in channels core-support-boundary gateway-watch; do + for name in channels core-support-boundary gateway-watch tui-pty; do if [ "${results[$name]}" = "failure" ]; then echo "::error title=${name} failed::${name} failed" failures=1 diff --git a/docs/ci.md b/docs/ci.md index c4bb814831db..3aee18b7b525 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -101,7 +101,7 @@ Separate iOS and macOS Periphery workflows enforce a zero-findings dead-code pol - **CI workflow edits** validate the Node CI graph, workflow linting, and the Windows lane (`ci.yml` executes it), but do not force iOS, Android, or macOS native builds by themselves; those platform lanes stay scoped to platform source changes. - **Workflow Sanity** runs `actionlint`, `zizmor` over all workflow YAML files, the composite-action interpolation guard, and the conflict-marker guard. The PR-scoped `security-fast` job also runs `zizmor` over changed workflow files so workflow security findings fail early in the main CI graph. - **Docs on `main` pushes** are checked by the standalone `Docs` workflow with the same ClawHub docs mirror used by CI, so mixed code+docs pushes do not also queue the CI `check-docs` shard. Pull requests and manual CI still run `check-docs` from CI when docs changed. -- **TUI PTY** runs in the `checks-node-core-runtime-tui-pty` Linux Node shard for TUI changes. The shard runs `test/vitest/vitest.tui-pty.config.ts` with `OPENCLAW_TUI_PTY_INCLUDE_LOCAL=1`, so it covers both the deterministic `TuiBackend` fixture lane and the slower `tui --local` smoke that mocks only the external model endpoint. +- **TUI PTY** splits by runtime ownership. The Linux Node shard runs the deterministic source-level `TuiBackend` fixture lane. The `build-artifacts` job reruns `test/vitest/vitest.tui-pty.config.ts` with `OPENCLAW_TUI_PTY_INCLUDE_LOCAL=1` and `OPENCLAW_TUI_PTY_USE_BUILT_CLI=1`, so the slower `tui --local` smoke exercises the exact-head built CLI while mocking only the external model endpoint. - **CI routing-only edits, the small set of core-test fixtures the fast task runs directly, and narrow plugin contract helper edits** use a fast Node-only manifest path: `preflight`, `security-fast`, and only the fast lanes the change touches — a single `checks-fast-core` CI-routing task, the two plugin contract shards, or both. That path skips build artifacts, Node 22 compatibility, channel contracts, full core shards, bundled-plugin shards, and additional guard matrices. - **Windows Node checks** are scoped to Windows-specific process/path wrappers, npm/pnpm/UI runner helpers, package manager config, and the CI workflow surfaces that execute that lane; unrelated source, plugin, install-smoke, and test-only changes stay on the Linux Node lanes. diff --git a/docs/reference/test.md b/docs/reference/test.md index 4d4c6fdb0e73..f08383399194 100644 --- a/docs/reference/test.md +++ b/docs/reference/test.md @@ -100,7 +100,7 @@ Test wrapper runs end with a short `[test] passed|failed|skipped ... in ...` sum ## Control UI, TUI, and extension lanes - **Control UI mocked E2E:** `pnpm test:ui:e2e` runs the Vitest + Playwright lane that starts the Vite Control UI and drives a real Chromium page against a mocked Gateway WebSocket. Tests live in `ui/src/**/*.e2e.test.ts`; shared mocks/controls live in `ui/src/test-helpers/control-ui-e2e.ts`. `pnpm test:e2e` includes this lane. Agent runs default to Testbox/Crabbox, including targeted proof; use `node scripts/run-vitest.mjs run --config test/vitest/vitest.ui-e2e.config.ts --configLoader runner ui/src/ui/e2e/chat-flow.e2e.test.ts` only for an explicit local fallback. -- **TUI PTY tests:** `node scripts/run-vitest.mjs run --config test/vitest/vitest.tui-pty.config.ts` runs the fast fake-backend PTY lane. `OPENCLAW_TUI_PTY_INCLUDE_LOCAL=1` or `pnpm tui:pty:test:watch --mode local` runs the slower `tui --local` smoke, which mocks only the external model endpoint. Assert stable visible text or fixture calls, not raw ANSI snapshots. +- **TUI PTY tests:** `node scripts/run-vitest.mjs run --config test/vitest/vitest.tui-pty.config.ts` runs the fast fake-backend PTY lane. `OPENCLAW_TUI_PTY_INCLUDE_LOCAL=1` or `pnpm tui:pty:test:watch --mode local` runs the slower `tui --local` smoke, which mocks only the external model endpoint. CI also sets `OPENCLAW_TUI_PTY_USE_BUILT_CLI=1` after building `dist/`; use that flag only when exact-head built artifacts already exist. Assert stable visible text or fixture calls, not raw ANSI snapshots. - `pnpm test:extensions` and `pnpm test extensions` run all extension/plugin shards. Heavy channel plugins, the browser plugin, and OpenAI run as dedicated shards; other plugin groups stay batched. `pnpm test extensions/` runs one bundled plugin lane. - Source files with sibling tests map to that sibling before falling back to wider directory globs. Helper edits under `src/channels/plugins/contracts/test-helpers`, `src/plugin-sdk/test-helpers`, and `src/plugins/contracts` use a local import graph to run importing tests instead of broad-running every shard when the dependency path is precise. - Contract directory targets fan out to their contract lanes: `pnpm test src/channels/plugins/contracts` runs the four channel contract configs and `pnpm test src/plugins/contracts` runs the plugin contracts config, since the generic `channels`/`plugins` projects exclude `contracts/**`. diff --git a/scripts/lib/ci-node-test-plan.mjs b/scripts/lib/ci-node-test-plan.mjs index 08df0b6ab03a..9b578d0ccfda 100644 --- a/scripts/lib/ci-node-test-plan.mjs +++ b/scripts/lib/ci-node-test-plan.mjs @@ -1158,8 +1158,9 @@ const SPLIT_NODE_SHARDS = new Map([ configs: ["test/vitest/vitest.tui-pty.config.ts"], env: { OPENCLAW_TUI_PTY_INCLUDE_LOCAL: "1", + OPENCLAW_TUI_PTY_USE_BUILT_CLI: "1", }, - requiresDist: false, + requiresDist: true, runner: "blacksmith-4vcpu-ubuntu-2404", }, { diff --git a/src/tui/tui-pty-harness.e2e.test.ts b/src/tui/tui-pty-harness.e2e.test.ts index 49505e46b2de..c69f33d28d4b 100644 --- a/src/tui/tui-pty-harness.e2e.test.ts +++ b/src/tui/tui-pty-harness.e2e.test.ts @@ -309,6 +309,40 @@ describe.sequential("TUI PTY harness", () => { STARTUP_TEST_TIMEOUT_MS, ); + it( + "cancels a buffered submit before Ctrl+D shutdown", + async () => { + const bufferedFixture = await startTuiFixture({ + env: { OPENCLAW_TUI_PTY_SUBMIT_BURST_WINDOW_MS: "500" }, + }); + try { + const message = "buffered shutdown proof"; + await bufferedFixture.run.waitForOutput("local ready", STARTUP_TIMEOUT_MS); + await bufferedFixture.run.write(`${message}\r`, { delay: false }); + await bufferedFixture.waitForLogEntry( + (entry) => + entry.method === "submitBurstCaptured" && objectFieldEquals(entry, "value", message), + ); + + await bufferedFixture.run.write("\u0004", { delay: false }); + expect((await bufferedFixture.run.waitForExit()).exitCode).toBe(0); + + const entries = await readFixtureLog(bufferedFixture.logPath); + expect(entries).toEqual( + expect.arrayContaining([expect.objectContaining({ method: "stop" })]), + ); + expect( + entries.some( + (entry) => entry.method === "sendChat" && objectFieldEquals(entry, "message", message), + ), + ).toBe(false); + } finally { + await bufferedFixture.cleanup(); + } + }, + STARTUP_TEST_TIMEOUT_MS, + ); + it.each([ { name: "Ctrl+D", input: "\u0004" }, { name: "/exit", input: "/exit\r" }, diff --git a/src/tui/tui-pty-local.e2e.test.ts b/src/tui/tui-pty-local.e2e.test.ts index 528feb5e8873..79ffd7c503d0 100644 --- a/src/tui/tui-pty-local.e2e.test.ts +++ b/src/tui/tui-pty-local.e2e.test.ts @@ -432,45 +432,11 @@ function buildTuiCliScript(args: string[]) { ].join("\n"); } -function buildLocalValidationTuiScript() { - const agentEventsModuleUrl = pathToFileURL( - path.join(process.cwd(), "src/infra/agent-events.ts"), - ).href; - const embeddedBackendModuleUrl = pathToFileURL( - path.join(process.cwd(), "src/tui/embedded-backend.ts"), - ).href; - const tuiModuleUrl = pathToFileURL(path.join(process.cwd(), "src/tui/tui.ts")).href; - // A PTY-side abort can race the validation retry into another provider turn. - // Abort through the real local backend after its listener records the second - // tool error; the Gateway case below still covers keyboard-driven aborts. - return [ - `import { onAgentEvent } from ${JSON.stringify(agentEventsModuleUrl)};`, - `import { EmbeddedTuiBackend } from ${JSON.stringify(embeddedBackendModuleUrl)};`, - `import { runTui } from ${JSON.stringify(tuiModuleUrl)};`, - `const backend = new EmbeddedTuiBackend();`, - `const sessionKey = "agent:main:main";`, - `let validationErrorCount = 0;`, - `onAgentEvent((event) => {`, - ` if (event.stream !== "tool" || event.data?.phase !== "result" || typeof event.data?.toolErrorSummary !== "string") return;`, - ` validationErrorCount += 1;`, - ` if (validationErrorCount !== 2) return;`, - ` queueMicrotask(() => {`, - ` void backend.abortChat({ sessionKey }).then((result) => {`, - ` if (!result.aborted) {`, - ` console.error("local validation test failed to abort its active run");`, - ` process.exit(1);`, - ` }`, - ` }).catch((error) => {`, - ` console.error(error);`, - ` process.exit(1);`, - ` });`, - ` });`, - `});`, - `runTui({ local: true, backend, session: sessionKey, deliver: false, historyLimit: 200, forceProcessExitOnReturn: true }).catch((error) => {`, - ` console.error(error);`, - ` process.exit(1);`, - `});`, - ].join("\n"); +function buildTuiProcessArgs(args: string[]) { + if (process.env.OPENCLAW_TUI_PTY_USE_BUILT_CLI === "1") { + return [path.join(process.cwd(), "openclaw.mjs"), ...args]; + } + return ["--import", "tsx", "--eval", buildTuiCliScript(args)]; } function buildMockModelProvider(baseUrl: string, modelIds: string[]): ModelProviderConfig { @@ -539,6 +505,28 @@ function buildLocalModeConfig(params: { } satisfies OpenClawConfig; } +async function cleanupLocalModeResources(params: { + run?: PtyRun; + mockModel: MockModelServer; + tempDir: string; +}) { + const settled = await Promise.allSettled([ + ...(params.run ? [params.run.dispose()] : []), + params.mockModel.stop(), + ]); + const failures = settled.flatMap((result) => + result.status === "rejected" ? [result.reason] : [], + ); + try { + await rm(params.tempDir, { recursive: true, force: true }); + } catch (error) { + failures.push(error); + } + if (failures.length > 0) { + throw new AggregateError(failures, "local TUI PTY fixture cleanup failed"); + } +} + async function startLocalModeTui( registerCleanup: CleanupRegistrar, opts: { @@ -566,41 +554,60 @@ async function startLocalModeTui( providerBaseUrl: mockModel.baseUrl, toolsProfile: opts.invalidEditLoop ? "coding" : "minimal", }); - const script = opts.invalidEditLoop - ? buildLocalValidationTuiScript() - : buildTuiCliScript(["tui", "--local"]); - await Promise.all([ - mkdir(workspaceDir, { recursive: true }), - mkdir(homeDir, { recursive: true }), - mkdir(stateDir, { recursive: true }), - mkdir(xdgConfigHome, { recursive: true }), - mkdir(xdgDataHome, { recursive: true }), - mkdir(xdgCacheHome, { recursive: true }), - writeFile(configPath, `${JSON.stringify(config, null, 2)}\n`, "utf8"), - ]); + let run: PtyRun; + try { + await Promise.all([ + mkdir(workspaceDir, { recursive: true }), + mkdir(homeDir, { recursive: true }), + mkdir(stateDir, { recursive: true }), + mkdir(xdgConfigHome, { recursive: true }), + mkdir(xdgDataHome, { recursive: true }), + mkdir(xdgCacheHome, { recursive: true }), + writeFile(configPath, `${JSON.stringify(config, null, 2)}\n`, "utf8"), + ]); - const run = startPty(process.execPath, ["--import", "tsx", "--eval", script], { - cwd: process.cwd(), - env: { - HOME: homeDir, - OPENCLAW_HOME: homeDir, - OPENCLAW_CONFIG_PATH: configPath, - OPENCLAW_STATE_DIR: stateDir, - XDG_CONFIG_HOME: xdgConfigHome, - XDG_DATA_HOME: xdgDataHome, - XDG_CACHE_HOME: xdgCacheHome, - OPENCLAW_THEME: "dark", - OPENCLAW_CODEX_DISCOVERY_LIVE: "0", - NO_COLOR: undefined, - }, - exitTimeoutMs: LOCAL_EXIT_TIMEOUT_MS, - outputTimeoutMs: LOCAL_OUTPUT_TIMEOUT_MS, - }); + run = startPty(process.execPath, buildTuiProcessArgs(["tui", "--local"]), { + cwd: process.cwd(), + env: { + HOME: homeDir, + OPENCLAW_HOME: homeDir, + OPENCLAW_CONFIG_PATH: configPath, + OPENCLAW_STATE_DIR: stateDir, + OPENCLAW_AGENT_DIR: undefined, + OPENCLAW_SKIP_PROVIDERS: undefined, + XDG_CONFIG_HOME: xdgConfigHome, + XDG_DATA_HOME: xdgDataHome, + XDG_CACHE_HOME: xdgCacheHome, + OPENCLAW_THEME: "dark", + OPENCLAW_CODEX_DISCOVERY_LIVE: "0", + NO_COLOR: undefined, + }, + exitTimeoutMs: LOCAL_EXIT_TIMEOUT_MS, + outputTimeoutMs: LOCAL_OUTPUT_TIMEOUT_MS, + }); + } catch (error) { + let cleanupFailure: unknown; + try { + await cleanupLocalModeResources({ mockModel, tempDir }); + } catch (cleanupError) { + cleanupFailure = cleanupError; + } + if (cleanupFailure !== undefined) { + const cleanupDetail = + cleanupFailure instanceof Error + ? cleanupFailure.message + : typeof cleanupFailure === "string" + ? cleanupFailure + : "unknown cleanup error"; + throw new Error(`local TUI PTY fixture cleanup failed: ${cleanupDetail}`, { + cause: error, + }); + } + throw error; + } const cleanup = createIdempotentCleanup(async () => { - await run.dispose(); - await mockModel.stop(); - await rm(tempDir, { recursive: true, force: true }); + await cleanupLocalModeResources({ run, mockModel, tempDir }); }); registerCleanup(cleanup); return { @@ -790,25 +797,28 @@ async function startGatewayModeTui( const requestOffset = shared.mockModel.requests(scenario.modelId).length; const sessionKey = `agent:${scenario.agentId}:tui-pty-${++gatewaySessionSequence}`; const sessionKeys = new Set([sessionKey]); - const script = buildTuiCliScript([ - "tui", - "--url", - shared.gateway.url, - "--token", - shared.gateway.gatewayToken, - "--session", - sessionKey, - ]); - const run = startPty(process.execPath, ["--import", "tsx", "--eval", script], { - cwd: process.cwd(), - env: { - ...shared.gateway.env, - OPENCLAW_THEME: "dark", - NO_COLOR: undefined, + const run = startPty( + process.execPath, + buildTuiProcessArgs([ + "tui", + "--url", + shared.gateway.url, + "--token", + shared.gateway.gatewayToken, + "--session", + sessionKey, + ]), + { + cwd: process.cwd(), + env: { + ...shared.gateway.env, + OPENCLAW_THEME: "dark", + NO_COLOR: undefined, + }, + exitTimeoutMs: LOCAL_EXIT_TIMEOUT_MS, + outputTimeoutMs: LOCAL_OUTPUT_TIMEOUT_MS, }, - exitTimeoutMs: LOCAL_EXIT_TIMEOUT_MS, - outputTimeoutMs: LOCAL_OUTPUT_TIMEOUT_MS, - }); + ); const cleanup = createIdempotentCleanup(async () => { shared.mockModel.releaseFirstResponse(scenario.modelId); await run.dispose(); @@ -1077,9 +1087,7 @@ describe("TUI PTY real backends", () => { ), }); } - if (mode === "gateway") { - await fixture.run.write("\u001b", { delay: false }); - } + await fixture.run.write("\u001b", { delay: false }); await fixture.run.waitForOutput( "run aborted: edit tool validation failed:", LOCAL_OUTPUT_TIMEOUT_MS, diff --git a/src/tui/tui-pty-test-support.test.ts b/src/tui/tui-pty-test-support.test.ts index 9ff6801231af..367e83be808f 100644 --- a/src/tui/tui-pty-test-support.test.ts +++ b/src/tui/tui-pty-test-support.test.ts @@ -32,6 +32,29 @@ describe("TUI PTY test support", () => { beforeEach(() => { nodePtyMocks.spawn.mockReset(); processTreeMocks.signalProcessTree.mockClear(); + vi.unstubAllEnvs(); + }); + + it("removes explicitly cleared inherited environment variables", () => { + vi.stubEnv("OPENCLAW_TUI_PTY_PARENT_ONLY", "parent"); + nodePtyMocks.spawn.mockReturnValue(createMockPty()); + + startPty("node", [], { + cwd: process.cwd(), + env: { + OPENCLAW_TUI_PTY_PARENT_ONLY: undefined, + OPENCLAW_TUI_PTY_CHILD_ONLY: "child", + }, + exitTimeoutMs: 1_000, + outputTimeoutMs: 1_000, + }); + + const spawnOptions = nodePtyMocks.spawn.mock.calls[0]?.[2]; + expect(spawnOptions?.env).not.toHaveProperty("OPENCLAW_TUI_PTY_PARENT_ONLY"); + expect(spawnOptions?.env).toMatchObject({ + OPENCLAW_TUI_PTY_CHILD_ONLY: "child", + TERM: "xterm-256color", + }); }); it("applies fixture-specific terminal dimensions", () => { @@ -198,16 +221,48 @@ describe("TUI PTY test support", () => { const disposal = run.dispose(); expect(run.dispose()).toBe(disposal); - expect(order).toEqual(["data-dispose", "kill"]); + expect(order).toEqual(["kill"]); order.push("exit"); exitListener?.({ exitCode: 0 }); await disposal; - expect(order).toEqual(["data-dispose", "kill", "exit", "exit-dispose"]); + expect(order).toEqual(["kill", "exit", "data-dispose", "exit-dispose"]); expect(kill).toHaveBeenCalledWith("SIGTERM"); }); + it("force-kills the owned process tree when graceful disposal times out", async () => { + const order: string[] = []; + const kill = vi.fn(() => order.push("term")); + nodePtyMocks.spawn.mockReturnValue( + createMockPty({ + pid: 4242, + kill, + onData: vi.fn(() => ({ dispose: () => order.push("data-dispose") })), + onExit: vi.fn(() => ({ dispose: () => order.push("exit-dispose") })), + }), + ); + processTreeMocks.signalProcessTree.mockImplementation((_pid, _signal, opts) => { + order.push("force"); + opts?.onComplete?.(); + }); + + const run = startPty("node", [], { + cwd: process.cwd(), + env: {}, + exitTimeoutMs: 1, + outputTimeoutMs: 1_000, + }); + + await run.dispose(); + + expect(kill).toHaveBeenCalledWith("SIGTERM"); + expect(processTreeMocks.signalProcessTree).toHaveBeenCalledWith(4242, "SIGKILL", { + onComplete: expect.any(Function), + }); + expect(order).toEqual(["term", "force", "data-dispose", "exit-dispose"]); + }); + it("force-kills the PTY and waits for its exit event", async () => { const order: string[] = []; let exitListener: ((event: { exitCode: number; signal?: number }) => void) | undefined; diff --git a/src/tui/tui-pty-test-support.ts b/src/tui/tui-pty-test-support.ts index a7e77dee2832..bebdb9acbac4 100644 --- a/src/tui/tui-pty-test-support.ts +++ b/src/tui/tui-pty-test-support.ts @@ -115,11 +115,17 @@ export function startPty( let visibleOutput = ""; let exitEvent: PtyExitEvent | null = null; const ansiStripper = new AnsiSequenceStripper(); - const ptyEnv = { + const mergedEnv = { ...process.env, ...opts.env, TERM: "xterm-256color", }; + const ptyEnv: Record = {}; + for (const [key, value] of Object.entries(mergedEnv)) { + if (value !== undefined) { + ptyEnv[key] = value; + } + } const pty = nodePty.spawn(command, args, { name: "xterm-256color", cols: readPtyDimensionEnv("OPENCLAW_TUI_PTY_COLS", 100, ptyEnv), @@ -151,6 +157,28 @@ export function startPty( let forceKillPromise: Promise | undefined; let disposePromise: Promise | undefined; + let subscriptionsDisposed = false; + + const disposeSubscriptions = () => { + if (subscriptionsDisposed) { + return; + } + subscriptionsDisposed = true; + dataSubscription.dispose(); + exitSubscription.dispose(); + }; + + const forceKillPty = async () => { + if (!exitEvent) { + // The PTY owns a process group; killing only its shell can leave the TUI child alive. + await new Promise((resolve) => { + signalProcessTree(pty.pid, "SIGKILL", { onComplete: resolve }); + }); + // Native PTY backends do not consistently emit onExit after a forced tree kill. + await sleep(PTY_EXIT_SETTLE_MS); + exitEvent ??= { exitCode: 137, signal: 9 }; + } + }; const run: PtyRun = { output: () => output, @@ -176,18 +204,9 @@ export function startPty( forceKill: () => { forceKillPromise ??= (async () => { try { - if (!exitEvent) { - // The PTY owns a process group; killing only its shell can leave the TUI child alive. - await new Promise((resolve) => { - signalProcessTree(pty.pid, "SIGKILL", { onComplete: resolve }); - }); - // Native PTY backends do not consistently emit onExit after a forced tree kill. - await sleep(PTY_EXIT_SETTLE_MS); - exitEvent ??= { exitCode: 137, signal: 9 }; - } + await forceKillPty(); } finally { - dataSubscription.dispose(); - exitSubscription.dispose(); + disposeSubscriptions(); } })(); return forceKillPromise; @@ -197,17 +216,21 @@ export function startPty( return forceKillPromise; } disposePromise ??= (async () => { - dataSubscription.dispose(); try { if (!exitEvent) { - pty.kill("SIGTERM"); + try { + pty.kill("SIGTERM"); + await waitForExit(); + } catch { + // Failure cleanup must not strand the PTY tree or replace the primary test error. + await forceKillPty(); + } } - await waitForExit(); // node-pty releases its native exit callback after onExit returns. // Give that release a turn before Vitest tears down the worker. await sleep(PTY_EXIT_SETTLE_MS); } finally { - exitSubscription.dispose(); + disposeSubscriptions(); } })(); return disposePromise; diff --git a/src/tui/tui-submit.ts b/src/tui/tui-submit.ts index 89aabd7891c6..547d70eb6d01 100644 --- a/src/tui/tui-submit.ts +++ b/src/tui/tui-submit.ts @@ -143,6 +143,7 @@ export function createSubmitBurstCoalescer(params: { let pending: { value: string; snapshot?: TuiChatSubmitSnapshot } | null = null; let pendingAt = 0; let flushTimer: ReturnType | null = null; + let disposed = false; const clearFlushTimer = () => { if (!flushTimer) { @@ -161,7 +162,7 @@ export function createSubmitBurstCoalescer(params: { }; const flushPending = () => { - if (!pending) { + if (disposed || !pending) { return; } const { value, snapshot } = pending; @@ -178,7 +179,10 @@ export function createSubmitBurstCoalescer(params: { }, windowMs); }; - return (value: string) => { + const submitBurst = (value: string) => { + if (disposed) { + return; + } if (!params.enabled) { submit(value, params.captureSnapshot?.()); return; @@ -211,4 +215,12 @@ export function createSubmitBurstCoalescer(params: { pendingAt = ts; scheduleFlush(); }; + + const dispose = () => { + disposed = true; + pending = null; + clearFlushTimer(); + }; + + return Object.assign(submitBurst, { dispose }); } diff --git a/src/tui/tui.submit-handler.test.ts b/src/tui/tui.submit-handler.test.ts index 2bd8c1471258..1f003bdb8249 100644 --- a/src/tui/tui.submit-handler.test.ts +++ b/src/tui/tui.submit-handler.test.ts @@ -179,18 +179,18 @@ describe("createSubmitBurstCoalescer", () => { vi.useFakeTimers(); const submit = vi.fn(); let now = 1_000; - const onSubmit = createSubmitBurstCoalescer({ + const submitBurst = createSubmitBurstCoalescer({ submit, enabled: true, burstWindowMs: 50, now: () => now, }); - onSubmit("Line 1"); + submitBurst("Line 1"); now += 10; - onSubmit("Line 2"); + submitBurst("Line 2"); now += 10; - onSubmit("Line 3"); + submitBurst("Line 3"); expect(submit).not.toHaveBeenCalled(); @@ -265,18 +265,37 @@ describe("createSubmitBurstCoalescer", () => { it("passes through immediately when disabled", () => { const submit = vi.fn(); - const onSubmit = createSubmitBurstCoalescer({ + const submitBurst = createSubmitBurstCoalescer({ submit, enabled: false, }); - onSubmit("Line 1"); - onSubmit("Line 2"); + submitBurst("Line 1"); + submitBurst("Line 2"); expect(submit).toHaveBeenCalledTimes(2); expect(submit).toHaveBeenNthCalledWith(1, "Line 1"); expect(submit).toHaveBeenNthCalledWith(2, "Line 2"); }); + + it("cancels pending and future submissions when disposed", () => { + vi.useFakeTimers(); + const submit = vi.fn(); + const submitBurst = createSubmitBurstCoalescer({ + submit, + enabled: true, + burstWindowMs: 50, + }); + + submitBurst("pending"); + submitBurst.dispose(); + submitBurst.dispose(); + submitBurst("after dispose"); + vi.advanceTimersByTime(50); + + expect(submit).not.toHaveBeenCalled(); + vi.useRealTimers(); + }); }); describe("shouldEnableWindowsGitBashPasteFallback", () => { diff --git a/src/tui/tui.ts b/src/tui/tui.ts index 6311730e3225..d5483f8f68f3 100644 --- a/src/tui/tui.ts +++ b/src/tui/tui.ts @@ -1513,6 +1513,8 @@ export async function runTui(opts: RunTuiOptions): Promise { }; const deferredFinish = createDeferredTuiFinish(); + // The backend can own requestExit before the editor/coalescer exists. + let disposeSubmitBurst = () => {}; const forceExit = () => { try { process.stderr.write("openclaw tui forcing exit\n"); @@ -1527,6 +1529,8 @@ export async function runTui(opts: RunTuiOptions): Promise { return; } exitRequested = true; + // Exit owns the input boundary before transport teardown can race a buffered submit. + disposeSubmitBurst(); connectionGeneration += 1; exitResult = { exitReason: result?.exitReason ?? "exit", @@ -1618,13 +1622,15 @@ export async function runTui(opts: RunTuiOptions): Promise { admitMessage: resolveMessageAdmission, onBlockedMessageSubmit: reportBlockedMessageSubmit, }); - editor.onSubmit = createSubmitBurstCoalescer({ + const submitBurst = createSubmitBurstCoalescer({ submit: submitHandler, captureSnapshot: captureMessageAdmission, enabled: opts.submitBurstWindowMs !== undefined || shouldEnableWindowsGitBashPasteFallback(), burstWindowMs: opts.submitBurstWindowMs, onCapture: opts.onSubmitBurstCaptured, }); + disposeSubmitBurst = submitBurst.dispose; + editor.onSubmit = submitBurst; editor.onEscape = () => { if (chatLog.hasVisibleBtw()) { diff --git a/test/scripts/ci-node-test-plan.test.ts b/test/scripts/ci-node-test-plan.test.ts index a4b1660368b4..38a9f3be4693 100644 --- a/test/scripts/ci-node-test-plan.test.ts +++ b/test/scripts/ci-node-test-plan.test.ts @@ -289,6 +289,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { .find((group) => group.shard_name === "core-runtime-tui-pty")?.env, ).toEqual({ OPENCLAW_TUI_PTY_INCLUDE_LOCAL: "1", + OPENCLAW_TUI_PTY_USE_BUILT_CLI: "1", // Timing-sensitive groups pin the worker budget while the job-level // default scales with the runner class. OPENCLAW_VITEST_MAX_WORKERS: "2", @@ -459,7 +460,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { .filter((shard) => shard.requiresDist) .map((shard) => shard.shardName); - expect(requiresDistShardNames).toEqual(["core-support-boundary"]); + expect(requiresDistShardNames).toEqual(["core-support-boundary", "core-runtime-tui-pty"]); }); it("splits tooling checks independently from built artifacts", () => { @@ -698,7 +699,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { }, { configs: ["test/vitest/vitest.tui-pty.config.ts"], - requiresDist: false, + requiresDist: true, runner: "blacksmith-4vcpu-ubuntu-2404", shardName: "core-runtime-tui-pty", }, @@ -747,7 +748,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { ]); }); - it("runs the TUI PTY local smoke inside the CI node shard", () => { + it("runs the TUI PTY local smoke against built CLI artifacts", () => { const tuiPtyShard = createNodeTestShards().find( (shard) => shard.shardName === "core-runtime-tui-pty", ); @@ -757,8 +758,9 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { configs: ["test/vitest/vitest.tui-pty.config.ts"], env: { OPENCLAW_TUI_PTY_INCLUDE_LOCAL: "1", + OPENCLAW_TUI_PTY_USE_BUILT_CLI: "1", }, - requiresDist: false, + requiresDist: true, }); });