From 17ef7771a66d21af51d26558a9a01ec7d01a227d Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 5 Aug 2026 11:08:56 +0800 Subject: [PATCH] fix(ci): stabilize TUI PTY proof ownership (#119420) * fix(ci): stabilize TUI PTY proof ownership Punchcard-Session: cobalt-brook-meadow-m9 * fix(test): align TUI PTY event typings Punchcard-Session: cobalt-brook-meadow-m9 --- .github/workflows/ci.yml | 9 +- docs/ci.md | 4 +- src/tui/tui-pty-local-test-support.test.ts | 96 ++++++++++++++++- src/tui/tui-pty-local-test-support.ts | 79 ++++++++++++++ src/tui/tui-pty-local.e2e.test.ts | 113 ++++++++++++++++----- test/scripts/ci-node-test-plan.test.ts | 3 +- test/scripts/ci-workflow-guards.test.ts | 7 +- 7 files changed, 276 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 239771a8e5e6..2a48b5d38e1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1291,15 +1291,18 @@ jobs: wait_checks fi - # The built local TUI lane owns a real Gateway fixture. Keep it - # exclusive so sibling artifact shards cannot starve its requests. + # The dedicated Node shard owns the full built-CLI PTY suite. Keep + # only the local roundtrip and Gateway-connect artifact canaries here. 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 + node scripts/run-vitest.mjs run \ + --config test/vitest/vitest.tui-pty.config.ts \ + src/tui/tui-pty-local.e2e.test.ts \ + --testNamePattern "launches openclaw (chat as local mode|tui against a real Gateway) through a real PTY" wait_checks fi diff --git a/docs/ci.md b/docs/ci.md index 45a439cf3f81..1ff042373957 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** 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. That built local lane runs after the other artifact checks because it owns a real Gateway fixture whose request deadlines must not compete with sibling shards. +- **TUI PTY** splits by proof ownership. The dedicated `core-runtime-tui-pty` Node shard owns the full real-backend suite against the exact-head built CLI. The `build-artifacts` job keeps only a local model roundtrip and a real Gateway connection canary, so every artifact boundary proves the built launcher without duplicating the full serial suite inside the build job. - **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. @@ -121,7 +121,7 @@ The slowest Node test families are split or balanced so each job stays small wit - The build-artifact job also persists content-fingerprinted `build-all` step outputs. CI's self-built plugin SDK declarations hash the complete repository-owned TypeScript/JSON source graph, exclude installed and generated directories, and restore both flat declarations and package bridges after `tsdown` clears `dist`. Documentation, workflow, plugin, and other changes outside that graph can reuse the declaration snapshot; source changes rebuild it before the export gate runs. - Full declaration builds split `tsdown` into AI, workspace-package, and unified groups. Each group caches declarations only, then still rebuilds runtime JavaScript before restoring those declarations. Core or plugin changes therefore invalidate only the large unified graph, while workspace-package changes conservatively invalidate every dependent declaration group. Public full builds generally use an immutable Actions cache; coarse restore keys seed partial changes, per-group content fingerprints reject stale data, and GitHub's cache quota evicts old generations. The weekly Node 22 lane instead publishes a 14-day artifact after successful `main` runs and restores only artifacts whose immutable producer identity resolves to that workflow on `main`, avoiding quota churn without allowing PR code to write a shared cache. Private-QA declarations are never persisted in Actions caches because cache namespaces are not confidentiality boundaries. - `check-additional-*` stripes the supplemental boundary guard list (`scripts/run-additional-boundary-checks.mjs`) into one prompt-heavy shard (`check-additional-boundaries-a`, which includes the Codex prompt snapshot drift check) and one combined shard for the remaining stripes (`check-additional-boundaries-bcd`), each running independent guards concurrently and printing per-check timings. Package-boundary compile/canary work stays together, and runtime topology architecture runs separately from the gateway watch coverage embedded in `build-artifacts`. -- On the 32-vCPU self-hosted build runner, Gateway watch, channel tests, and the core support-boundary shard start together inside `build-artifacts` after `dist/` and `dist-runtime/` are already built. GitHub-hosted fallback runs keep Gateway watch serial so low-core contention cannot consume its readiness deadline. Both paths then run built local TUI PTY alone so its real Gateway fixture has exclusive process and event-loop capacity. +- On the 32-vCPU self-hosted build runner, Gateway watch, channel tests, and the core support-boundary shard start together inside `build-artifacts` after `dist/` and `dist-runtime/` are already built. GitHub-hosted fallback runs keep Gateway watch serial so low-core contention cannot consume its readiness deadline. Both paths then run the two built TUI PTY artifact canaries alone; the dedicated Node shard owns the full serial suite. Once admitted, canonical Linux CI permits up to 28 concurrent Node test jobs and 12 for the smaller fast/check lanes; Windows and Android stay at two because diff --git a/src/tui/tui-pty-local-test-support.test.ts b/src/tui/tui-pty-local-test-support.test.ts index ac22bcbf0d16..1fab651d6300 100644 --- a/src/tui/tui-pty-local-test-support.test.ts +++ b/src/tui/tui-pty-local-test-support.test.ts @@ -1,12 +1,106 @@ import { describe, expect, it } from "vitest"; import { createDeferred } from "../test-utils/deferred.js"; -import { cleanupStartedFixture, createFreshSession } from "./tui-pty-local-test-support.js"; +import { + cleanupStartedFixture, + createChatTerminalObserver, + createFreshSession, + registerIdempotentCleanup, +} from "./tui-pty-local-test-support.js"; import type { PtyRun } from "./tui-pty-test-support.js"; const SUBMISSION_SETTLE_MS = 150; const SESSION_ROLLOVER_BUSY_MESSAGE = "abort the current run before /new"; describe("local TUI PTY fixture support", () => { + it("registers idempotent cleanup before fallible fixture setup", async () => { + const order: string[] = []; + let registeredCleanup: (() => Promise) | undefined; + const setupError = new Error("setup failed"); + let cleanupCalls = 0; + + await expect( + (async () => { + const cleanup = registerIdempotentCleanup( + (registered) => { + order.push("registered"); + registeredCleanup = registered; + }, + async () => { + cleanupCalls += 1; + order.push("cleanup"); + }, + ); + try { + order.push("setup"); + throw setupError; + } finally { + await cleanup(); + } + })(), + ).rejects.toBe(setupError); + await registeredCleanup!(); + + expect(order).toEqual(["registered", "setup", "cleanup"]); + expect(cleanupCalls).toBe(1); + }); + + it("waits for the matching successful terminal event before history observation", async () => { + const observer = createChatTerminalObserver(); + const terminal = observer.waitForFinal({ + runId: "run-history", + sessionKey: "agent:main:history", + timeoutMs: 1_000, + }); + + observer.onEvent({ + event: "chat", + payload: { + runId: "run-history", + sessionKey: "agent:main:history", + state: "delta", + }, + }); + observer.onEvent({ + event: "chat", + payload: { + runId: "run-other", + sessionKey: "agent:main:history", + state: "final", + }, + }); + observer.onEvent({ + event: "chat", + payload: { + runId: "run-history", + sessionKey: "agent:main:history", + state: "final", + }, + }); + + await expect(terminal).resolves.toMatchObject({ state: "final" }); + }); + + it("fails promptly when the observed chat run terminates with an error", async () => { + const observer = createChatTerminalObserver(); + observer.onEvent({ + event: "chat", + payload: { + errorMessage: "provider failed", + runId: "run-history", + sessionKey: "agent:main:history", + state: "error", + }, + }); + + await expect( + observer.waitForFinal({ + runId: "run-history", + sessionKey: "agent:main:history", + timeoutMs: 1_000, + }), + ).rejects.toThrow("chat run run-history ended as error: provider failed"); + }); + it("owns late fixture startup without swallowing cleanup failures", async () => { await expect(cleanupStartedFixture(Promise.reject(new Error("setup failed")))).resolves.toBe( undefined, diff --git a/src/tui/tui-pty-local-test-support.ts b/src/tui/tui-pty-local-test-support.ts index 626becbdafd2..4002e373cab5 100644 --- a/src/tui/tui-pty-local-test-support.ts +++ b/src/tui/tui-pty-local-test-support.ts @@ -3,6 +3,85 @@ import { waitFor, type PtyRun } from "./tui-pty-test-support.js"; const STARTUP_TIMEOUT_MS = 60_000; const OUTPUT_TIMEOUT_MS = 120_000; +type CleanupRegistrar = (cleanup: () => Promise) => void; + +export function createIdempotentCleanup(cleanup: () => Promise) { + let cleanupPromise: Promise | undefined; + return () => (cleanupPromise ??= cleanup()); +} + +// Register before setup starts so a timed-out test still owns partial resources. +export function registerIdempotentCleanup( + registerCleanup: CleanupRegistrar, + cleanup: () => Promise, +) { + const registeredCleanup = createIdempotentCleanup(cleanup); + registerCleanup(registeredCleanup); + return registeredCleanup; +} + +type ObservedChatTerminal = { + errorMessage?: string; + runId: string; + sessionKey: string; + state: "aborted" | "error" | "final"; +}; + +// A completed-history assertion must wait for the run's terminal event first. +// Polling history during the run can consume the RPC deadline and leak that run. +export function createChatTerminalObserver() { + const terminals = new Map(); + const keyFor = (sessionKey: string, runId: string) => `${sessionKey}\u0000${runId}`; + + return { + onEvent: ({ event, payload }: { event: string; payload?: unknown }) => { + if (event !== "chat" || !payload || typeof payload !== "object") { + return; + } + const chatEvent = payload as { + errorMessage?: unknown; + runId?: unknown; + sessionKey?: unknown; + state?: unknown; + }; + if ( + typeof chatEvent.runId !== "string" || + typeof chatEvent.sessionKey !== "string" || + (chatEvent.state !== "aborted" && + chatEvent.state !== "error" && + chatEvent.state !== "final") + ) { + return; + } + terminals.set(keyFor(chatEvent.sessionKey, chatEvent.runId), { + ...(typeof chatEvent.errorMessage === "string" + ? { errorMessage: chatEvent.errorMessage } + : {}), + runId: chatEvent.runId, + sessionKey: chatEvent.sessionKey, + state: chatEvent.state, + }); + }, + waitForFinal: async (params: { runId: string; sessionKey: string; timeoutMs: number }) => { + const terminal = await waitFor({ + timeoutMs: params.timeoutMs, + read: () => terminals.get(keyFor(params.sessionKey, params.runId)) ?? null, + onTimeout: () => + new Error(`chat run ${params.runId} did not reach a terminal event before history load`), + }); + terminals.delete(keyFor(params.sessionKey, params.runId)); + if (terminal.state !== "final") { + throw new Error( + `chat run ${params.runId} ended as ${terminal.state}${ + terminal.errorMessage ? `: ${terminal.errorMessage}` : "" + }`, + ); + } + return terminal; + }, + }; +} + export async function waitForOutputAfter( run: PtyRun, needle: string, diff --git a/src/tui/tui-pty-local.e2e.test.ts b/src/tui/tui-pty-local.e2e.test.ts index 421cfe5b7985..01e76b75a853 100644 --- a/src/tui/tui-pty-local.e2e.test.ts +++ b/src/tui/tui-pty-local.e2e.test.ts @@ -21,8 +21,11 @@ import { GatewayChatClient } from "./gateway-chat.js"; import { synchronizedFrameRows } from "./tui-pty-harness-assertion-test-support.js"; import { cleanupStartedFixture, + createChatTerminalObserver, + createIdempotentCleanup, createFreshSession, lastOutputIndexAfter, + registerIdempotentCleanup, waitForOutputAfter, } from "./tui-pty-local-test-support.js"; import { sleep, startPty, waitFor, type PtyRun } from "./tui-pty-test-support.js"; @@ -159,11 +162,6 @@ async function requestWithUnavailableRetry(request: () => Promise): Promis } } -function createIdempotentCleanup(cleanup: () => Promise) { - let cleanupPromise: Promise | undefined; - return () => (cleanupPromise ??= cleanup()); -} - type CleanupRegistrar = (cleanup: () => Promise) => void; async function readRequestBody(req: IncomingMessage): Promise { @@ -845,8 +843,37 @@ async function startGatewayModeTui( const rejectedRequestOffset = shared.mockModel.rejectedRequests().length; const sessionKey = `agent:${scenario.agentId}:tui-pty-${++gatewaySessionSequence}`; const sessionKeys = new Set([sessionKey]); - await shared.controlClient.createSession({ key: sessionKey, agentId: scenario.agentId }); - await shared.controlClient.patchSession({ + const controlClient = new GatewayChatClient({ + url: shared.gateway.url, + token: shared.gateway.gatewayToken, + allowInsecureLocalOperatorUi: false, + }); + let controlClientConnected = false; + controlClient.onConnected = () => { + controlClientConnected = true; + }; + // A timed-out RPC drops its pending response while leaving the socket open. + // Case-local ownership prevents that late work from crossing into the next test. + const cleanup = registerIdempotentCleanup(registerCleanup, async () => { + shared.mockModel.releaseFirstResponse(scenario.modelId); + try { + if (controlClientConnected) { + for (const key of sessionKeys) { + await controlClient.abortChat({ sessionKey: key }); + } + } + } finally { + await controlClient.stop(); + } + }); + controlClient.start(); + await waitFor({ + timeoutMs: LOCAL_STARTUP_TIMEOUT_MS, + read: () => (controlClientConnected ? true : null), + onTimeout: () => new Error("Gateway case control client did not connect"), + }); + await controlClient.createSession({ key: sessionKey, agentId: scenario.agentId }); + await controlClient.patchSession({ key: sessionKey, agentId: scenario.agentId, model: `tui-pty-mock/${scenario.modelId}`, @@ -865,13 +892,6 @@ async function startGatewayModeTui( onTimeout: () => new Error("adopted Gateway session did not reach an idle final screen"), }); const outputOffset = run.visibleOutput().length; - const cleanup = createIdempotentCleanup(async () => { - shared.mockModel.releaseFirstResponse(scenario.modelId); - for (const key of sessionKeys) { - await shared.controlClient.abortChat({ sessionKey: key }); - } - }); - registerCleanup(cleanup); return { kind: "gateway" as const, run, @@ -1593,27 +1613,62 @@ export default { const shared = await requireSharedGatewayFixture(); const agentId = SHARED_GATEWAY_AGENT_ID; const sessionKey = `agent:${agentId}:tui-pty-history`; + const runId = randomUUID(); const userMarker = "T02_HISTORY_USER"; const assistantMarker = GATEWAY_SCENARIOS.history.replyText; const model = `tui-pty-mock/${GATEWAY_SCENARIOS.history.modelId}`; - await shared.controlClient.createSession({ key: sessionKey, agentId }); - await shared.controlClient.patchSession({ key: sessionKey, agentId, model }); - await shared.controlClient.sendChat({ sessionKey, message: userMarker }); - await waitForHistoryMessages(shared.controlClient, sessionKey, ({ messages }) => - hasOrderedTurn(messages, userMarker, assistantMarker), - ); - const attached = await startIsolatedGatewayPty({ - gateway: shared.gateway, - registerCleanup: onTestFinished, - sessionKey, + const terminalObserver = createChatTerminalObserver(); + const historyClient = new GatewayChatClient({ + url: shared.gateway.url, + token: shared.gateway.gatewayToken, + allowInsecureLocalOperatorUi: false, }); + let historyClientConnected = false; + historyClient.onConnected = () => { + historyClientConnected = true; + }; + historyClient.onEvent = terminalObserver.onEvent; + const cleanup = registerIdempotentCleanup(onTestFinished, async () => { + try { + if (historyClientConnected) { + await historyClient.abortChat({ sessionKey, runId }); + } + } finally { + await historyClient.stop(); + } + }); + let attached: Awaited> | undefined; try { - await attached.run.waitForOutput(assistantMarker, LOCAL_STARTUP_TIMEOUT_MS); + historyClient.start(); + await waitFor({ + timeoutMs: LOCAL_STARTUP_TIMEOUT_MS, + read: () => (historyClientConnected ? true : null), + onTimeout: () => new Error("history Gateway client did not connect"), + }); + await historyClient.subscribeSessionEvents(); + await historyClient.createSession({ key: sessionKey, agentId }); + await historyClient.patchSession({ key: sessionKey, agentId, model }); + await historyClient.sendChat({ sessionKey, message: userMarker, runId }); + await terminalObserver.waitForFinal({ + runId, + sessionKey, + timeoutMs: LOCAL_OUTPUT_TIMEOUT_MS, + }); + await waitForHistoryMessages(historyClient, sessionKey, ({ messages }) => + hasOrderedTurn(messages, userMarker, assistantMarker), + ); + attached = await startIsolatedGatewayPty({ + gateway: shared.gateway, + registerCleanup: onTestFinished, + sessionKey, + }); + const attachedRun = attached.run; + await attachedRun.waitForOutput(assistantMarker, LOCAL_STARTUP_TIMEOUT_MS); const output = await waitFor({ timeoutMs: LOCAL_STARTUP_TIMEOUT_MS, read: () => { const screen = - synchronizedFrameRows(attached.run.output(), attached.run)[0]?.join("\n") ?? ""; + synchronizedFrameRows(attachedRun.output(), attachedRun)[0]?.join("\n") ?? ""; return screen.includes(userMarker) && screen.includes(assistantMarker) ? screen : null; }, onTimeout: () => new Error("history did not reach a final synchronized TUI screen"), @@ -1622,7 +1677,11 @@ export default { expect(output.split(assistantMarker)).toHaveLength(2); expect(output.indexOf(userMarker)).toBeLessThan(output.indexOf(assistantMarker)); } finally { - await attached.cleanup(); + try { + await attached?.cleanup(); + } finally { + await cleanup(); + } } }, LOCAL_TEST_TIMEOUT_MS, diff --git a/test/scripts/ci-node-test-plan.test.ts b/test/scripts/ci-node-test-plan.test.ts index 09dcb06c707c..e35c8f8181b2 100644 --- a/test/scripts/ci-node-test-plan.test.ts +++ b/test/scripts/ci-node-test-plan.test.ts @@ -727,7 +727,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { ]); }); - it("runs the TUI PTY local smoke against built CLI artifacts", () => { + it("keeps the full TUI PTY suite in its dedicated built-CLI shard", () => { const tuiPtyShard = createNodeTestShards().find( (shard) => shard.shardName === "core-runtime-tui-pty", ); @@ -741,6 +741,7 @@ describe("scripts/lib/ci-node-test-plan.mjs", () => { }, requiresDist: true, }); + expect(tuiPtyShard?.includePatterns).toBeUndefined(); }); it("covers every infra test exactly once across core runtime infra shards", () => { diff --git a/test/scripts/ci-workflow-guards.test.ts b/test/scripts/ci-workflow-guards.test.ts index f20abdc3ece1..b2e918064a6c 100644 --- a/test/scripts/ci-workflow-guards.test.ts +++ b/test/scripts/ci-workflow-guards.test.ts @@ -4890,7 +4890,7 @@ printf '%s\n' "\${CURL_SUCCESS_IP:-203.0.113.7}" expect(saveStep.with.key).toContain("dist-build-v2-"); }); - it("parallelizes gateway watch only on the large self-hosted runner and isolates TUI PTY", () => { + it("keeps the full built TUI PTY suite out of the artifact canary gate", () => { const workflow = readCiWorkflow(); const buildArtifactSteps = workflow.jobs["build-artifacts"].steps; const builtArtifactChecks = buildArtifactSteps.find( @@ -4922,6 +4922,11 @@ printf '%s\n' "\${CURL_SUCCESS_IP:-203.0.113.7}" expect(hostedGatewayWait).toBeGreaterThan(hostedGatewayWatch); expect(tuiPty).toBeGreaterThan(hostedGatewayWait); expect(tuiPtyWait).toBeGreaterThan(tuiPty); + expect(run.slice(tuiPty, tuiPtyWait)).toContain("src/tui/tui-pty-local.e2e.test.ts"); + expect(run.slice(tuiPty, tuiPtyWait)).toContain("--testNamePattern"); + expect(run.slice(tuiPty, tuiPtyWait)).toContain( + "launches openclaw (chat as local mode|tui against a real Gateway) through a real PTY", + ); expect(run).toContain("wait_checks()"); expect(run.match(/wait_checks$/gmu)).toHaveLength(3); });