diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f6f197382c..5ccd17700b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -290,10 +290,44 @@ jobs: retention-days: 7 overwrite: true + acp_lifecycle_smoke: + if: ${{ always() && !startsWith(github.ref, 'refs/tags/v') && (github.event_name != 'workflow_dispatch' || inputs.rehearsal == 'main-nontag' || inputs.rehearsal == 'nightly-release') && needs.main_plan.outputs.has_tasks == 'true' && needs.main_native.result != 'failure' && needs.main_native.result != 'cancelled' }} + needs: [main_plan, main_native] + runs-on: ubuntu-22.04 + timeout-minutes: 15 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: "24" + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: "1.3.14" + - name: Cache bun dependencies + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 + with: + path: ~/.bun/install/cache + key: bun-1.3.14-${{ runner.os }}-${{ hashFiles('**/bun.lock') }} + - run: bun install --frozen-lockfile + - name: Download native addon(s) + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 + with: + name: main-native-${{ github.run_id }} + path: packages/natives/native + - name: Run ACP lifecycle smoke + # bunfig `[test] pathIgnorePatterns` keeps this out of the default suite, and + # naming the path does NOT re-include it: `bun test ` filters files that + # were already discovered, so a pruned file never matches. Overriding the list + # is the only way in. The canonical argv — which restates every repository + # ignore pattern so the override does not widen discovery — lives in + # ci-dev-affected.ts (BUN_TEST_IGNORE_OVERRIDES / dedicatedTestCommand), so + # this job invokes the planner task instead of duplicating the pattern list. + run: bun scripts/ci-dev-affected.ts --task=acp-lifecycle-smoke + # Branch protection must keep requiring this stable aggregate status. test: if: ${{ always() && !startsWith(github.ref, 'refs/tags/v') && (github.event_name != 'workflow_dispatch' || inputs.rehearsal == 'main-nontag' || inputs.rehearsal == 'nightly-release') }} - needs: [main_plan, main_native, main_shards, acp_conformance] + needs: [main_plan, main_native, main_shards, acp_conformance, acp_lifecycle_smoke] runs-on: ubuntu-22.04 timeout-minutes: 5 steps: @@ -303,11 +337,13 @@ jobs: native='${{ needs.main_native.result }}' shards='${{ needs.main_shards.result }}' conformance='${{ needs.acp_conformance.result }}' - echo "main_plan=$plan main_native=$native main_shards=$shards acp_conformance=$conformance" + lifecycle='${{ needs.acp_lifecycle_smoke.result }}' + echo "main_plan=$plan main_native=$native main_shards=$shards acp_conformance=$conformance acp_lifecycle_smoke=$lifecycle" test "$plan" = success case "$native" in success|skipped) ;; *) echo "native gate failed"; exit 1;; esac test "$shards" = success test "$conformance" = success + test "$lifecycle" = success nightly_gate: if: ${{ always() && (github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && inputs.rehearsal == 'nightly-release')) }} diff --git a/bunfig.toml b/bunfig.toml index 104a4b2ca3..5439f40eb8 100644 --- a/bunfig.toml +++ b/bunfig.toml @@ -18,6 +18,13 @@ pathIgnorePatterns = [ "**/node_modules/**", ".wt/**", ".worktrees/**", + # Spawns a broker plus a session host per run and costs tens of seconds, so it + # stays out of the default suite. Naming the path on the command line does NOT + # re-include it -- `bun test ` is a filter over already-discovered files, + # so a pruned file can never match. The only way to run it is the canonical + # dedicated argv (ci-dev-affected.ts BUN_TEST_IGNORE_OVERRIDES override), which + # the planner emits; the fresh-process shard inventory also excludes it. + "**/test/acp/acp-lifecycle-smoke.test.ts", ] [run] diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 447cbab472..ca135655e1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,7 @@ ### Added - Added implicit discovery and keyless local support for oMLX (`http://127.0.0.1:8080/v1`) with `OMLX_BASE_URL` and `OMLX_API_KEY` configuration. +- ACP session lifecycle (`session/list`, `fork`, `resume`, `close`, `delete`) is now gated in CI. `initialize` advertised all five capabilities to every ACP client and all five worked, but the pinned upstream `acp-core-v1` corpus contains 21 cases and exercised none of them, so the advertised surface had no release-gate coverage. A new stdio smoke test drives the credential-free conformance fixture and asserts the five happy paths, that a forked session id is distinct from its source, that `session/list` returns the created session under `cwd` filtering, and that duplicate `close` is idempotent. Because it spawns a broker plus a session host it is a *dedicated-only* test: `bunfig.toml` prunes it from default `bun test` discovery, the fresh-process shard inventory excludes it (`DEDICATED_ONLY_TESTS`), and exactly one canonical argv — the full ignore-list override from `ci-dev-affected.ts` `BUN_TEST_IGNORE_OVERRIDES` — runs it. The planner routes every path: PR-mode targeted plans emit `test:` with that override, Main CI's full plan emits the `acp-lifecycle-smoke` task, the `acp_lifecycle_smoke` CI job invokes the planner task, and the aggregate `test` job fails closed unless it succeeds. A bunfig prune removes a file from *discovery*, so plain `bun test ` — a filter over already-discovered files — can never run a pruned file; any caller that schedules it without the override fails deterministically, which is what both initial CI failures were. Duplicate `close` intentionally pins the already-closed no-op, but the unknown-session *error* shape is deliberately left unasserted: `resume`/`prompt` reject unknown ids with `-32603` while `close`/`delete` no-op on unowned ones (`AcpAgent.closeSession` gates on connection ownership), and whether that asymmetry and that error code are correct is a separately filed open question. - Gajae Pet now renders in iTerm2 through a bounded inline GIF protected from ordinary TUI redraws by a reserved raster lease, with the same composer-side layout and lifecycle cleanup guarantees used by Kitty and Sixel. - Added the bundled `ouroboros` dark theme, translating the official navy, teal, green, and gold identity into terminal-safe semantic colors while retaining the pet's vivid `#AEE80E` lime and `#7092BE` cool-scale accents. Live previews now recolor the open `/theme` selector and Settings theme submenu instead of leaving their construction-time theme visible, and confirmation consistently settles the preview into the active appearance mapping. - Added the 16×16 `Ouroboros` terminal pet: a vivid lime snake with a cool `#7092BE` underside that rests in a soft coil, blinks, flicks its tongue upward, and occasionally sobs with `><` eyes. Its signature flex rolls the same silhouette through a symmetric circle into an exact 180-degree heart pose, blinks a small pink heart twice, and reuses the authored frames in reverse; agent work enters and exits a stable six-frame infinity loop through explicit unwind transitions. Pet skins now own their frame registry, source resolution, idle loop, work transitions, work loop, and signature burst; a saved skin removed by a later installation falls back to RedGajae while an explicit `off` remains off. diff --git a/packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts b/packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts new file mode 100644 index 0000000000..a1a988eb5c --- /dev/null +++ b/packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts @@ -0,0 +1,518 @@ +/** + * Lifecycle smoke over raw ACP stdio. + * + * `initialize` advertises `sessionCapabilities` for list/fork/resume/close/delete, + * but the pinned upstream `acp-core-v1` corpus exercises none of them, so the + * advertised surface has no release-gate coverage. This closes that hole by + * driving the credential-free conformance fixture over real JSON-RPC frames. + * + * Excluded from default `bun test` discovery via `bunfig.toml` + * `[test] pathIgnorePatterns` because it spawns a broker plus a session host and + * costs tens of seconds; it is a *dedicated-only* test (see ci-dev-affected.ts + * DEDICATED_ONLY_TESTS), so the fresh-process shard inventory skips it too. + * Naming this path on the command line does NOT re-include it -- `bun test + * ` filters files that were already discovered, so a pruned file can never + * match. The only way in is to override that list with `--path-ignore-patterns`, + * which is exactly what the canonical dedicated argv + * (ci-dev-affected.ts dedicatedTestCommand) does; every planner and CI route + * runs the suite through that argv, never a bare `bun test `. + * + * Deliberately NOT covered: the unknown-session error *shape*. `close`/`delete` on + * an unowned session no-op by design -- `AcpAgent.closeSession` documents "only + * connection-owned sessions may reach broker lifecycle control" -- while + * `resume`/`prompt` reject. Whether that asymmetry and its `-32603` code are + * right is undecided, and pinning it here would cement an unreviewed contract. + * The post-close prompt below therefore asserts only that the call is REJECTED, + * never its code or message, so renaming or re-coding that error stays free. + */ +import { afterAll, beforeAll, expect, setDefaultTimeout, test } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; + +setDefaultTimeout(180_000); + +const REPO_ROOT = path.resolve(import.meta.dir, "..", "..", "..", ".."); +const FIXTURE_AGENT = path.join(REPO_ROOT, "packages/coding-agent/scripts/acp-conformance-agent.ts"); +const REQUEST_TIMEOUT_MS = 120_000; +const DISPOSE_REQUEST_TIMEOUT_MS = 5_000; +const DISPOSE_EXIT_GRACE_MS = 2_000; +/** Enough fixture stderr to diagnose a startup or broker failure, not enough to flood CI logs. */ +const STDERR_TAIL_LIMIT = 4_000; +/** + * stdout and stderr are independent streams, so a malformed frame can reach the + * reader before the stderr chunk explaining it has been drained. Terminal + * failures wait this long for that drain, because an unexplained failure is the + * exact thing this client exists to avoid. + */ +const STDERR_DRAIN_MS = 2_000; + +interface RpcError { + code: number; + message: string; +} + +interface RpcFrame { + id?: number; + method?: string; + result?: unknown; + error?: RpcError; +} + +interface SessionCapabilities { + list?: unknown; + fork?: unknown; + resume?: unknown; + close?: unknown; + delete?: unknown; +} + +interface InitializeResult { + agentCapabilities?: { sessionCapabilities?: SessionCapabilities }; +} + +interface SessionRow { + sessionId?: unknown; + cwd?: unknown; + title?: unknown; + updatedAt?: unknown; +} + +interface PendingRequest { + resolve(frame: RpcFrame): void; + reject(error: Error): void; +} + +/** + * The peer answered with a JSON-RPC error frame. Distinct from transport, + * timeout, framing, and harness failures so a probe that expects a protocol + * rejection cannot be satisfied by the client simply falling over. + */ +class AcpPeerRejection extends Error { + readonly code: number; + + constructor(method: string, error: RpcError) { + super(`ACP request rejected: ${method}: ${error.code} ${error.message}`); + this.name = "AcpPeerRejection"; + this.code = error.code; + } +} + +/** + * Minimal newline-delimited JSON-RPC client. Only what the lifecycle surface + * needs: correlated requests, and a record of which notification methods + * arrived. + * + * Every way the peer can die -- malformed frame, closed stdout, process exit -- + * fails outstanding requests immediately with the captured stderr attached. + * Without that, a broken fixture surfaces as an opaque two-minute request + * timeout, which is a poor failure mode for a required CI gate. + */ +class AcpStdioClient { + readonly #child: Bun.Subprocess<"pipe", "pipe", "pipe">; + readonly #pending = new Map(); + readonly #notifications = new Set(); + /** Every session this client opened, so teardown can close broker-owned hosts it created. */ + readonly #opened = new Set(); + readonly #stderrDone: Promise; + #stderr = ""; + #nextId = 0; + #terminalError: Error | undefined; + #terminated = false; + + constructor(cwd: string, command: readonly string[] = ["bun", FIXTURE_AGENT]) { + this.#child = Bun.spawn([...command], { + cwd: REPO_ROOT, + env: { ...process.env, GJC_ACP_CONFORMANCE_CWD: cwd }, + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + this.#stderrDone = this.#readStderr(); + void this.#readFrames(); + void this.#watchExit(); + } + + get notifications(): string[] { + return [...this.#notifications].sort(); + } + + /** Records a session so `dispose` can reap it; ids already closed may be re-recorded harmlessly. */ + track(sessionId: string): void { + this.#opened.add(sessionId); + } + + #describe(summary: string): Error { + const tail = this.#stderr.trim(); + return new Error(tail.length > 0 ? `${summary}\n--- fixture stderr ---\n${tail}` : summary); + } + + /** First terminal cause wins; later ones are consequences of it. */ + async #terminate(summary: string): Promise { + if (this.#terminated) return; + this.#terminated = true; + // Nothing is waiting on a diagnostic during ordinary disposal, so do not + // stall teardown for a drain no one will read. + if (this.#pending.size > 0) await Promise.race([this.#stderrDone, Bun.sleep(STDERR_DRAIN_MS)]); + const error = this.#describe(summary); + this.#terminalError = error; + for (const request of this.#pending.values()) request.reject(error); + this.#pending.clear(); + } + + /** Never rejects: it is awaited as a drain barrier, and a broken stderr must not mask the real cause. */ + async #readStderr(): Promise { + const decoder = new TextDecoder(); + try { + for await (const chunk of this.#child.stderr) { + this.#stderr = (this.#stderr + decoder.decode(chunk, { stream: true })).slice(-STDERR_TAIL_LIMIT); + } + } catch (cause) { + this.#stderr = `${this.#stderr}\n`; + } + } + + async #watchExit(): Promise { + const code = await this.#child.exited; + await this.#terminate(`ACP fixture exited with code ${code} before the request settled`); + } + + async #readFrames(): Promise { + const decoder = new TextDecoder(); + let buffer = ""; + try { + for await (const chunk of this.#child.stdout) { + buffer += decoder.decode(chunk, { stream: true }); + let newline = buffer.indexOf("\n"); + while (newline >= 0) { + const line = buffer.slice(0, newline).trim(); + buffer = buffer.slice(newline + 1); + newline = buffer.indexOf("\n"); + if (!line) continue; + this.#dispatch(line); + } + } + await this.#terminate("ACP fixture closed stdout before the request settled"); + } catch (cause) { + await this.#terminate(`ACP framing failed: ${cause instanceof Error ? cause.message : String(cause)}`); + } + } + + #dispatch(line: string): void { + let frame: RpcFrame; + try { + frame = JSON.parse(line) as RpcFrame; + } catch { + throw new Error(`unparseable frame: ${line.slice(0, 200)}`); + } + const request = typeof frame.id === "number" ? this.#pending.get(frame.id) : undefined; + if (request) { + this.#pending.delete(frame.id as number); + request.resolve(frame); + } else if (frame.method) this.#notifications.add(frame.method); + } + + /** Resolves the RPC result, or throws with the peer's error attached. */ + async call(method: string, params: unknown, timeoutMs: number = REQUEST_TIMEOUT_MS): Promise { + if (this.#terminalError) throw this.#terminalError; + + const id = ++this.#nextId; + const { promise, resolve, reject } = Promise.withResolvers(); + this.#pending.set(id, { resolve, reject }); + try { + this.#child.stdin.write(`${JSON.stringify({ jsonrpc: "2.0", id, method, params })}\n`); + await this.#child.stdin.flush(); + } catch (cause) { + // The request never reached the peer, so nothing will ever settle it. Drop it + // and observe its promise before rethrowing, or it becomes an exit-race + // unhandled rejection. + this.#pending.delete(id); + promise.catch(() => undefined); + reject(new Error(`ACP request could not be sent: ${method}`)); + throw cause; + } + + const timeout = Bun.sleep(timeoutMs).then(() => { + this.#pending.delete(id); + throw this.#describe(`ACP request timed out after ${timeoutMs}ms: ${method}`); + }); + const frame = await Promise.race([promise, timeout]); + if (frame.error) throw new AcpPeerRejection(method, frame.error); + return frame.result; + } + + /** + * Killing the ACP client does not close broker-owned session hosts: the broker + * spawns one `sdk session-host-internal` per session and outlives this process. + * Anything still open must be closed explicitly or every run leaks a host, which + * accumulates permanently on a long-lived CI runner. Best-effort by design -- + * teardown must not convert a reaping failure into a test failure that hides the + * real one. + */ + async dispose(): Promise { + await Promise.all( + [...this.#opened].map(async sessionId => { + try { + await this.call("session/close", { sessionId }, DISPOSE_REQUEST_TIMEOUT_MS); + } catch { + // Already closed, already gone, timed out, or the transport is down; the kill below covers it. + } + }), + ); + + let exited = false; + const exit = this.#child.exited.then(() => { + exited = true; + }); + this.#child.kill("SIGTERM"); + await Promise.race([exit, Bun.sleep(DISPOSE_EXIT_GRACE_MS)]); + if (!exited) { + this.#child.kill("SIGKILL"); + await exit; + } + } +} + +function asObject(value: unknown): Record { + if (value === null || typeof value !== "object" || Array.isArray(value)) + throw new Error(`Expected a JSON object, received ${JSON.stringify(value)}`); + return value as Record; +} + +function sessionIdOf(value: unknown): string { + const id = asObject(value).sessionId; + if (typeof id !== "string" || id.length === 0) + throw new Error(`Expected a sessionId, received ${JSON.stringify(id)}`); + return id; +} + +function rowsOf(value: unknown): SessionRow[] { + const sessions = asObject(value).sessions; + return Array.isArray(sessions) ? (sessions as SessionRow[]) : []; +} + +/** Everything the lifecycle sequence observed, captured once and asserted per criterion. */ +interface LifecycleObservations { + sessionCapabilities: SessionCapabilities; + scratchCwd: string; + createdSessionId: string; + otherCwd: string; + otherSessionId: string; + listedRows: SessionRow[]; + otherCwdRows: SessionRow[]; + resumeResult: Record; + forkedSessionId: string; + forkResult: Record; + deleteForked: Record; + rowsAfterDelete: SessionRow[]; + closeCreated: Record; + closeCreatedAgain: Record; + promptAfterCloseRejected: boolean; + resumeAfterClose: Record; + closeAfterResume: Record; + notifications: string[]; +} + +let observed: LifecycleObservations; +const scratchDirs: string[] = []; + +async function makeScratch(): Promise { + // The ACP client enforces the session cwd root against the RESOLVED path, and + // on macOS `mktemp -d` hands back /tmp/... which resolves to /private/tmp/..., + // so an unresolved path fails client-authority checks. + const dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "gjc-acp-lifecycle-"))); + scratchDirs.push(dir); + return dir; +} + +afterAll(async () => { + await Promise.all(scratchDirs.map(dir => fs.rm(dir, { recursive: true, force: true }))); +}); + +beforeAll(async () => { + const scratchCwd = await makeScratch(); + // A second workspace exists solely so the cwd filter has something to exclude: + // with one session in the index, an implementation ignoring `cwd` entirely would + // still satisfy a contains-check. + const otherCwd = await makeScratch(); + const client = new AcpStdioClient(scratchCwd); + + try { + const init = (await client.call("initialize", { + protocolVersion: 1, + clientCapabilities: { fs: { readTextFile: true, writeTextFile: true }, terminal: true }, + })) as InitializeResult; + + const created = await client.call("session/new", { cwd: scratchCwd, mcpServers: [] }); + const createdSessionId = sessionIdOf(created); + client.track(createdSessionId); + + const other = await client.call("session/new", { cwd: otherCwd, mcpServers: [] }); + const otherSessionId = sessionIdOf(other); + client.track(otherSessionId); + + const listedRows = rowsOf(await client.call("session/list", { cwd: scratchCwd })); + const otherCwdRows = rowsOf(await client.call("session/list", { cwd: otherCwd })); + + const resumeResult = asObject( + await client.call("session/resume", { sessionId: createdSessionId, cwd: scratchCwd }), + ); + + const forkResult = asObject(await client.call("session/fork", { sessionId: createdSessionId, cwd: scratchCwd })); + const forkedSessionId = sessionIdOf(forkResult); + client.track(forkedSessionId); + + const deleteForked = asObject(await client.call("session/delete", { sessionId: forkedSessionId })); + // Re-list so delete is proven by its external postcondition rather than only + // by the shape of its own response. + const rowsAfterDelete = rowsOf(await client.call("session/list", { cwd: scratchCwd })); + + const closeCreated = asObject(await client.call("session/close", { sessionId: createdSessionId })); + const closeCreatedAgain = asObject(await client.call("session/close", { sessionId: createdSessionId })); + + // `session/list` still returns a closed session, so it cannot witness the close. + // Losing prompt eligibility can. Only a peer-level JSON-RPC rejection counts: + // a timeout or transport failure is rethrown rather than miscounted as proof, + // because a recoverable timeout leaves the client usable and would otherwise let + // this gate pass without close having done anything. The code and message are + // never inspected, so the disputed unknown-session error shape stays unpinned. + let promptAfterCloseRejected = false; + try { + await client.call("session/prompt", { + sessionId: createdSessionId, + prompt: [{ type: "text", text: "post-close liveness probe" }], + }); + } catch (cause) { + if (!(cause instanceof AcpPeerRejection)) throw cause; + promptAfterCloseRejected = true; + } + + // The real reattachment path: this session is now detached, so resume has to go + // back through the broker rather than hand back an already-attached handle. + const resumeAfterClose = asObject( + await client.call("session/resume", { sessionId: createdSessionId, cwd: scratchCwd }), + ); + const closeAfterResume = asObject(await client.call("session/close", { sessionId: createdSessionId })); + + observed = { + sessionCapabilities: init.agentCapabilities?.sessionCapabilities ?? {}, + scratchCwd, + createdSessionId, + otherCwd, + otherSessionId, + listedRows, + otherCwdRows, + resumeResult, + forkedSessionId, + forkResult, + deleteForked, + rowsAfterDelete, + closeCreated, + closeCreatedAgain, + promptAfterCloseRejected, + resumeAfterClose, + closeAfterResume, + notifications: client.notifications, + }; + } finally { + await client.dispose(); + } +}); + +test("initialize advertises every session lifecycle capability", () => { + expect(Object.keys(observed.sessionCapabilities).sort()).toEqual(["close", "delete", "fork", "list", "resume"]); +}); + +test("session/new returns a distinct session id per workspace", () => { + expect(observed.createdSessionId).toMatch(/\S/); + expect(observed.otherSessionId).toMatch(/\S/); + expect(observed.otherSessionId).not.toBe(observed.createdSessionId); +}); + +test("session/list filtered by cwd returns the created session with its identifying fields", () => { + const row = observed.listedRows.find(candidate => candidate.sessionId === observed.createdSessionId); + expect(row).toBeDefined(); + expect(row?.cwd).toBe(observed.scratchCwd); + expect(typeof row?.title).toBe("string"); + expect(typeof row?.updatedAt).toBe("string"); +}); + +test("session/list discriminates on cwd instead of returning every session", () => { + // Each listing must exclude the other workspace's session; a `cwd` parameter that + // is accepted and then ignored fails here but would pass a contains-only check. + expect(observed.listedRows.map(row => row.sessionId)).not.toContain(observed.otherSessionId); + expect(observed.otherCwdRows.map(row => row.sessionId)).toContain(observed.otherSessionId); + expect(observed.otherCwdRows.map(row => row.sessionId)).not.toContain(observed.createdSessionId); +}); + +test("session/resume returns live session state", () => { + expect(observed.resumeResult).toHaveProperty("configOptions"); + expect(observed.resumeResult).toHaveProperty("modes"); +}); + +test("session/close costs the session its prompt eligibility", () => { + expect(observed.promptAfterCloseRejected).toBe(true); +}); + +test("session/resume reattaches a session that was closed", () => { + expect(observed.resumeAfterClose).toHaveProperty("configOptions"); + expect(observed.resumeAfterClose).toHaveProperty("modes"); + expect(observed.closeAfterResume).toEqual({}); +}); + +test("session/fork mints a session id distinct from its source", () => { + expect(observed.forkedSessionId).toMatch(/\S/); + expect(observed.forkedSessionId).not.toBe(observed.createdSessionId); + expect(observed.forkResult).toHaveProperty("modes"); +}); + +test("session/delete removes the forked session from the listing", () => { + expect(observed.deleteForked).toEqual({}); + const remaining = observed.rowsAfterDelete.map(row => row.sessionId); + expect(remaining).not.toContain(observed.forkedSessionId); + expect(remaining).toContain(observed.createdSessionId); +}); + +test("session/close closes the created session", () => { + expect(observed.closeCreated).toEqual({}); +}); + +test("session/close is idempotent when repeated on the same session", () => { + expect(observed.closeCreatedAgain).toEqual({}); +}); + +test("the lifecycle sequence streams session updates", () => { + expect(observed.notifications).toContain("session/update"); +}); + +test("request timeout teardown force-kills a fixture that ignores termination", async () => { + const cwd = await makeScratch(); + const fixture = path.join(cwd, "hung-acp-fixture.ts"); + await Bun.write( + fixture, + [ + 'process.on("SIGTERM", () => undefined);', + 'process.stdout.write(JSON.stringify({ jsonrpc: "2.0", method: "fixture/ready" }) + "\\n");', + "await new Promise(() => undefined);", + ].join("\n"), + ); + const client = new AcpStdioClient(cwd, ["bun", fixture]); + let disposed = false; + + try { + for (let attempt = 0; attempt < 200 && !client.notifications.includes("fixture/ready"); attempt++) { + await Bun.sleep(10); + } + expect(client.notifications).toContain("fixture/ready"); + await expect(client.call("initialize", {}, 50)).rejects.toThrow("ACP request timed out after 50ms"); + + const disposeStarted = performance.now(); + await client.dispose(); + disposed = true; + const disposeElapsed = performance.now() - disposeStarted; + expect(disposeElapsed).toBeGreaterThanOrEqual(DISPOSE_EXIT_GRACE_MS - 100); + expect(disposeElapsed).toBeLessThan(DISPOSE_EXIT_GRACE_MS + 2_000); + } finally { + if (!disposed) await client.dispose(); + } +}); diff --git a/scripts/ci-dev-affected.test.ts b/scripts/ci-dev-affected.test.ts index 72b94ca381..0c73d9741b 100644 --- a/scripts/ci-dev-affected.test.ts +++ b/scripts/ci-dev-affected.test.ts @@ -2,7 +2,7 @@ import { afterAll, describe, expect, setDefaultTimeout, test } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { describeTasks, expandWithDependents, isDarwinArm64TabWorkerSmokePath, isWindowsSessionPathRegressionPath, loadBuildInventory, needsDarwinArm64TabWorkerSmoke, needsWindowsSessionPathRegression, normalizeChangedPaths, packageScriptCommand, planFullTasks, planTargetedTasks, planTasks, requiresCargoWorkspaceEmergency, resolvePackageCwd, runCommand, validateAffectedAggregate, type AffectedAggregateResults, type CargoInventoryUnit, type WorkspacePackage } from "./ci-dev-affected"; +import { BUN_TEST_IGNORE_OVERRIDES, DEDICATED_ONLY_TESTS, dedicatedTestCommand, describeTasks, expandWithDependents, isDarwinArm64TabWorkerSmokePath, isDedicatedOnlyTest, isFullPlanMatrixTask, isWindowsSessionPathRegressionPath, loadBuildInventory, needsDarwinArm64TabWorkerSmoke, needsWindowsSessionPathRegression, normalizeChangedPaths, packageScriptCommand, planFullTasks, planTargetedTasks, planTasks, requiresCargoWorkspaceEmergency, resolvePackageCwd, runCommand, validateAffectedAggregate, type AffectedAggregateResults, type CargoInventoryUnit, type WorkspacePackage } from "./ci-dev-affected"; import { runSdkProductionHostIsolated, sdkProductionHostIsolatedSuites, @@ -1730,3 +1730,107 @@ describe("planFullTasks — Main CI full mode (issue: shard main CI)", () => { expect(entries.find(entry => entry.key === "test:@gajae-code/coding-agent:shard-1-of-16")?.native).toBe(true); }); }); + +describe("dedicated-only tests — routing contract", () => { + const ACP_LIFECYCLE = "packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts"; + const DEDICATED_TASK_KEY = `test:${ACP_LIFECYCLE}`; + + test("the override argv restates every bunfig ignore pattern and appends the target", () => { + for (const pattern of BUN_TEST_IGNORE_OVERRIDES) { + expect(dedicatedTestCommand(ACP_LIFECYCLE)).toContain(pattern); + } + expect(dedicatedTestCommand(ACP_LIFECYCLE)).toEqual([ + "bun", + "test", + "--path-ignore-patterns", + "**/node_modules/**", + "--path-ignore-patterns", + ".wt/**", + "--path-ignore-patterns", + ".worktrees/**", + ACP_LIFECYCLE, + ]); + }); + + test("the planner override list stays in lockstep with bunfig.toml ignores", async () => { + const bunfig = await Bun.file("bunfig.toml").text(); + const section = /\[test\][\s\S]*?pathIgnorePatterns\s*=\s*\[([^\]]*)\]/.exec(bunfig); + expect(section).toBeDefined(); + const withoutComments = (section?.[1] ?? "") + .split("\n") + .map(line => line.replace(/^\s*#.*$/, "")) + .join("\n"); + const bunfigPatterns = withoutComments + .split(",") + .map(entry => entry.trim().replace(/^["']|["']$/g, "")) + .filter(entry => entry.length > 0); + // bunfig must equal the canonical overrides plus exactly one prune per + // dedicated-only test — any other drift widens or narrows discovery. + expect(bunfigPatterns.length).toBe(BUN_TEST_IGNORE_OVERRIDES.length + DEDICATED_ONLY_TESTS.size); + for (const pattern of BUN_TEST_IGNORE_OVERRIDES) { + expect(bunfigPatterns).toContain(pattern); + } + for (const dedicated of DEDICATED_ONLY_TESTS) { + // bunfig stores a glob (`**/`), so the glob's tail must cover the file's tail. + const tail = dedicated.slice(dedicated.indexOf("/test/") + 1); + const covered = bunfigPatterns.some(entry => entry.endsWith(tail)); + expect(covered).toBe(true); + } + }); + + test("a changed dedicated-only test plans exactly one runnable dedicated task with the override argv", () => { + const tasks = planTargetedTasks([ACP_LIFECYCLE], packages, [ACP_LIFECYCLE, "packages/coding-agent/test/edit/foo.test.ts"]); + const dedicated = tasks.find(task => task.key === DEDICATED_TASK_KEY); + expect(dedicated).toBeDefined(); + expect(dedicated?.command).toEqual(dedicatedTestCommand(ACP_LIFECYCLE)); + // The plain `bun test ` form is exactly what failed in CI: a pruned + // file is never discovered, so the filter matches nothing (exit 1). + expect(dedicated?.command).not.toEqual(["bun", "test", ACP_LIFECYCLE]); + expect(tasks.filter(task => task.key === DEDICATED_TASK_KEY)).toHaveLength(1); + // Ordinary files keep the plain invocation. + const ordinary = planTargetedTasks(["packages/coding-agent/test/edit/foo.test.ts"], packages, [ + "packages/coding-agent/test/edit/foo.test.ts", + ]); + expect(ordinary.find(task => task.key === "test:packages/coding-agent/test/edit/foo.test.ts")?.command).toEqual([ + "bun", + "test", + "packages/coding-agent/test/edit/foo.test.ts", + ]); + }); + + test("the Main CI full plan exposes the dedicated suite only to its named job", () => { + const tasks = planFullTasks(packages); + const dedicated = tasks.filter(task => task.key === "acp-lifecycle-smoke"); + expect(dedicated).toHaveLength(1); + expect(dedicated[0]?.command).toEqual(dedicatedTestCommand(ACP_LIFECYCLE)); + expect(dedicated[0] && isFullPlanMatrixTask(dedicated[0])).toBe(false); + expect(tasks.some(task => isFullPlanMatrixTask(task))).toBe(true); + expect(tasks.filter(task => task.key === DEDICATED_TASK_KEY)).toHaveLength(0); + }); + + test("the dedicated ACP smoke task is marked as needing the prebuilt native addon", () => { + for (const task of [...planFullTasks(packages), ...planTargetedTasks([ACP_LIFECYCLE], packages, [ACP_LIFECYCLE])]) { + if (task.key === "acp-lifecycle-smoke" || task.key === DEDICATED_TASK_KEY) { + expect(describeTasks([task])[0]?.native).toBe(true); + } + } + }); + + test("Main CI invokes the dedicated task through the planner, not a duplicated ignore list", async () => { + const workflow = await Bun.file(".github/workflows/ci.yml").text(); + expect(workflow).toContain("bun scripts/ci-dev-affected.ts --task=acp-lifecycle-smoke"); + // The workflow must not restate the override patterns itself; that would + // fork the contract away from BUN_TEST_IGNORE_OVERRIDES. + expect(workflow).not.toContain('--path-ignore-patterns="**/node_modules/**"'); + }); + + test("dedicated-only tests never run through fresh-process shard inventory", async () => { + const { enumerateTestFiles } = await import("./run-bun-test-files"); + const files = await enumerateTestFiles("packages/coding-agent"); + expect(files).not.toContain(ACP_LIFECYCLE); + for (const dedicated of DEDICATED_ONLY_TESTS) { + expect(files).not.toContain(dedicated); + expect(isDedicatedOnlyTest(dedicated)).toBe(true); + } + }); +}); diff --git a/scripts/ci-dev-affected.ts b/scripts/ci-dev-affected.ts index 5d5debd4c5..3df5b04aa9 100755 --- a/scripts/ci-dev-affected.ts +++ b/scripts/ci-dev-affected.ts @@ -59,6 +59,10 @@ const CODING_AGENT_SHARD_ONE_COVERAGE_PATHS = [ // Declared here (before the top-level `await main()`) so it is initialized for // every CLI mode despite top-level await halting later module statements. const NATIVE_BUILD_KEYS: ReadonlySet = new Set(["native-build", "native-linux-x64"]); +// Full Main CI tasks that run in named, fail-closed workflow jobs rather than +// the generic matrix. They remain in planFullTasks() so the named job resolves +// the exact canonical task through --task without duplicating its argv. +const FULL_PLAN_STANDALONE_TASK_KEYS: ReadonlySet = new Set(["acp-lifecycle-smoke"]); // Behavioral-owner tests cover entrypoint contracts whose names intentionally do // not follow the source-file basename convention. They supplement, rather than @@ -241,6 +245,9 @@ export function planFullTasks(packages: readonly WorkspacePackage[]): Task[] { addNativeBuild(tasks); addWorkspaceTestTasks(tasks, packages); add(tasks, "test:scripts/run-bun-test-files.test.ts", "Test fresh-process Bun harness", ["bun", "test", "scripts/run-bun-test-files.test.ts"]); + // Dedicated-only tests are pruned from package shards and default discovery, so + // the full plan must schedule them explicitly or Main CI never runs them. + add(tasks, "acp-lifecycle-smoke", "Test ACP session lifecycle smoke (dedicated)", dedicatedTestCommand("packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts"), undefined, { rust: false, nextest: false, nativeConsumer: true, nativeProducer: false }); add(tasks, "rust-check", "Rust check", ["bun", "run", "check:rs"]); addRustTestTasks(tasks); add(tasks, "cli-smoke", "GJC CLI smoke test", ["bun", "run", "ci:test:smoke"]); @@ -287,6 +294,43 @@ async function resolvePlannedTasks(paths: readonly string[]): Promise { // a changed source file to its directly-named test. node_modules is excluded so // the index is identical whether or not dependencies are installed (the planner // job skips install; shards install before running) — keeping plans stable. +// Tests that `bunfig.toml` prunes from default Bun discovery because they are too +// expensive or environment-heavy to run per ordinary suite, and that must therefore +// run only through an explicit dedicated task. The bunfig prune removes the file +// from *discovery*, so `bun test ` (a filter over already-discovered files) +// can never select it — the only way in is replacing the ignore list via +// `--path-ignore-patterns`, which is what dedicatedTestCommand() emits. +// Fresh-process shard inventory (run-bun-test-files.ts) mirrors this exclusion so +// package shards never schedule a file plain `bun test ./` cannot run. +export const DEDICATED_ONLY_TESTS: ReadonlySet = new Set([ + "packages/coding-agent/test/acp/acp-lifecycle-smoke.test.ts", +]); + +// The complete ignore list a dedicated test invocation must restate. Overriding +// `--path-ignore-patterns` replaces the bunfig list entirely, so dropping the +// repository's other canonical patterns here would silently widen discovery. +export const BUN_TEST_IGNORE_OVERRIDES: readonly string[] = [ + "**/node_modules/**", + ".wt/**", + ".worktrees/**", +]; + +export function isDedicatedOnlyTest(testFile: string): boolean { + return DEDICATED_ONLY_TESTS.has(testFile); +} + +// Argv for running exactly one test file. Dedicated-only files get the full +// bunfig ignore-list override, which is the sole mechanism that makes a pruned +// file runnable; ordinary files keep the plain invocation. +export function dedicatedTestCommand(testFile: string): readonly string[] { + return [ + "bun", + "test", + ...BUN_TEST_IGNORE_OVERRIDES.flatMap(pattern => ["--path-ignore-patterns", pattern]), + testFile, + ]; +} + async function gatherTestFiles(): Promise { const patterns = ["packages/**/*.test.ts", "packages/**/*.test.tsx", "scripts/**/*.test.ts"]; const found = new Set(); @@ -384,6 +428,10 @@ export function describeTasks(tasks: readonly Task[]): TaskMatrixEntry[] { })); } +export function isFullPlanMatrixTask(task: Task): boolean { + return !isNativeProducerTask(task) && task.phase !== "python" && !FULL_PLAN_STANDALONE_TASK_KEYS.has(task.key); +} + // `--matrix-json` prints the planned tasks as a JSON array on stdout (consumed // by tests and for debugging). Under GitHub Actions it also appends the dev-ci // planner outputs: `matrix`, `has_tasks`, `has_native`, and the canonical Darwin @@ -456,7 +504,7 @@ async function emitFullMatrix(): Promise { const githubOutput = process.env.GITHUB_OUTPUT; if (!githubOutput) return; const shards = tasks - .filter(task => !isNativeProducerTask(task) && task.phase !== "python") + .filter(isFullPlanMatrixTask) .map(task => { const entry = describeTasks([task])[0]!; return { key: entry.key, identity: entry.identity, description: entry.description, native: entry.native, rust: entry.rust, nextest: entry.nextest }; @@ -958,8 +1006,13 @@ export function planTargetedTasks(paths: readonly string[], packages: readonly W // Add a task that runs exactly one test file. Keyed as `test:` // so the matrix shard name stays small and directly traceable to the file. +// Dedicated-only files must run through the bunfig ignore-list override: plain +// `bun test ` filters over already-discovered files, and a bunfig-pruned +// file is never discovered, so the plain form deterministically fails with +// "filters did not match any test files" (exit 1). function addTestFileTask(tasks: Map, testFile: string): void { - add(tasks, `test:${testFile}`, `Test ${testFile}`, ["bun", "test", testFile]); + const command = isDedicatedOnlyTest(testFile) ? dedicatedTestCommand(testFile) : ["bun", "test", testFile]; + add(tasks, `test:${testFile}`, `Test ${testFile}`, command); } function addWorkspaceTestTasks(tasks: Map, packages: readonly WorkspacePackage[]): void { diff --git a/scripts/run-bun-test-files.ts b/scripts/run-bun-test-files.ts index 75d0d2062d..4cc2c18ec4 100644 --- a/scripts/run-bun-test-files.ts +++ b/scripts/run-bun-test-files.ts @@ -4,6 +4,7 @@ import * as fs from "node:fs/promises"; import * as fsSync from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import { isDedicatedOnlyTest } from "./ci-dev-affected"; export const DEFAULT_TEST_TIMEOUT_MS = 30_000; export const DEFAULT_FILE_TIMEOUT_MS = 5 * 60_000; @@ -78,6 +79,13 @@ function isCredentialEnvironmentName(name: string): boolean { // it, so package-wide runtime suites must not execute it implicitly. const SOURCE_BOUND_EVIDENCE_TESTS = new Set(["packages/ai/test/anthropic-cache-eval.integration.test.ts"]); +// Dedicated-only tests (ci-dev-affected.ts DEDICATED_ONLY_TESTS) are pruned from +// default Bun discovery by bunfig.toml because they are too expensive to run per +// ordinary suite. This harness spawns `bun test ./` per file, which obeys +// the same bunfig prune, so scheduling such a file here would fail every time +// with "filters did not match any test files". They run only through their +// explicit dedicated task with the full ignore-list override. + function usage(message?: string): never { if (message) process.stderr.write(`${message}\n`); process.stderr.write( @@ -134,6 +142,7 @@ export async function enumerateTestFiles(root: string, base: string = repoRoot): if (!TEST_FILE_PATTERN.test(normalized)) continue; const file = path.posix.join(relativeRoot.split(path.sep).join("/"), normalized); if (SOURCE_BOUND_EVIDENCE_TESTS.has(file)) continue; + if (isDedicatedOnlyTest(file)) continue; files.push(file); } return files.sort();