From 06cbe40e7d0f8ae7d56d299e3f32e4e6a5621178 Mon Sep 17 00:00:00 2001 From: JohnnyVicious Date: Sun, 12 Apr 2026 15:05:45 +0200 Subject: [PATCH 1/2] test: add opencode HTTP server integration tests + install opencode in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds protocol-level integration tests for the OpenCode HTTP wrapper our companion script is built on. These were previously untested — none of the existing 57 unit tests touched `lib/opencode-server.mjs` because running them requires a real `opencode serve` process. The CI workflow now installs `opencode-ai` so these tests can run. New file: `tests/opencode-server.test.mjs` Spawns `opencode serve` on a high test port (default 14096, override via `OPENCODE_TEST_PORT`) so it does not collide with a developer's default-port server. Tears the server down via SIGTERM in `after()`, even on test failure. Coverage: * `isServerRunning` detects the spawned server * `health()` returns `{ healthy: true, version: }` * `createSession` returns an id * `listSessions` includes a freshly-created session * `createSession` -> `getSession` -> `deleteSession` roundtrip, with the post-delete `getSession` rejected with a 4xx Intentionally does NOT exercise `sendPrompt` because that would call a configured AI provider and burn paid API credits in CI. End-to-end review tests will need a separate workflow with secrets. Local skipping: the entire suite is gated on `isOpencodeInstalled()` via `describe.skip`, so developers without `opencode` on their PATH can still run `npm test` without failures. CI changes (`.github/workflows/ci.yml`): * New "Install OpenCode CLI" step running `npm install -g opencode-ai`, modeled after codex-plugin-cc's `Install Codex CLI` step. * `timeout-minutes: 5 -> 10` to absorb the cli install + opencode server startup overhead. * Updated header comment to explain we install opencode (not codex). Test count: 57 -> 62. All passing locally with `OPENCODE_TEST_PORT=14096`. --- .github/workflows/ci.yml | 14 +++-- tests/opencode-server.test.mjs | 110 +++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 tests/opencode-server.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 747d7e2..80541e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,10 +1,11 @@ name: CI # Adapted from openai/codex-plugin-cc's pull-request-ci.yml. -# Modified by JohnnyVicious (2026): drops the Codex CLI install step (this -# fork wraps OpenCode, not Codex) and runs only on a single Node version -# matching the package.json `engines.node` floor. (Apache License 2.0 -# §4(b) modification notice — see NOTICE.) +# Modified by JohnnyVicious (2026): wraps OpenCode (not Codex), so installs +# `opencode-ai` instead of `@openai/codex` for the integration tests, and +# runs only on a single Node version matching the package.json +# `engines.node` floor. (Apache License 2.0 §4(b) modification notice — +# see NOTICE.) on: pull_request: @@ -19,7 +20,7 @@ jobs: test: name: Test runs-on: ubuntu-latest - timeout-minutes: 5 + timeout-minutes: 10 steps: - name: Check out repository @@ -34,6 +35,9 @@ jobs: - name: Install dependencies run: npm ci + - name: Install OpenCode CLI + run: npm install -g opencode-ai + - name: Syntax-check companion scripts run: | node --check plugins/opencode/scripts/opencode-companion.mjs diff --git a/tests/opencode-server.test.mjs b/tests/opencode-server.test.mjs new file mode 100644 index 0000000..88d261f --- /dev/null +++ b/tests/opencode-server.test.mjs @@ -0,0 +1,110 @@ +// Integration tests for the OpenCode HTTP server wrapper. +// +// These tests start a real `opencode serve` process on a high test port +// (so they don't collide with a user's default-port server) and exercise +// the protocol surfaces our companion script depends on: +// +// - server lifecycle (`isServerRunning`, `ensureServer`) +// - health endpoint +// - session create / get / list / delete +// +// They intentionally do NOT call `sendPrompt` because that requires a +// configured AI provider and would burn paid API credits in CI. +// +// Locally, this suite is skipped if the `opencode` binary is not on PATH, +// so developers without OpenCode installed can still run `npm test`. + +import { describe, it, before, after } from "node:test"; +import assert from "node:assert/strict"; +import { + isOpencodeInstalled, +} from "../plugins/opencode/scripts/lib/process.mjs"; +import { + isServerRunning, + ensureServer, + createClient, +} from "../plugins/opencode/scripts/lib/opencode-server.mjs"; + +const TEST_HOST = "127.0.0.1"; +const TEST_PORT = Number(process.env.OPENCODE_TEST_PORT ?? 14096); + +const opencodeAvailable = await isOpencodeInstalled(); +const describeOrSkip = opencodeAvailable ? describe : describe.skip; + +describeOrSkip("opencode HTTP server (integration)", () => { + let serverInfo; + let client; + + before(async () => { + // If something is already squatting our test port, fail fast with a + // useful message instead of silently sharing state with a foreign + // server. + if (await isServerRunning(TEST_HOST, TEST_PORT)) { + throw new Error( + `Test port ${TEST_PORT} is already in use. Set OPENCODE_TEST_PORT to a free port.` + ); + } + + serverInfo = await ensureServer({ host: TEST_HOST, port: TEST_PORT }); + client = createClient(serverInfo.url); + }); + + after(() => { + // Tear down the server we spawned. `ensureServer` only sets `pid` + // when it actually started a new process, so this is a no-op when + // the server was already running (which our `before` rejects, but + // be defensive). + if (serverInfo?.pid && !serverInfo.alreadyRunning) { + try { + process.kill(serverInfo.pid, "SIGTERM"); + } catch { + // already exited + } + } + }); + + it("isServerRunning detects the spawned server", async () => { + assert.equal(await isServerRunning(TEST_HOST, TEST_PORT), true); + }); + + it("health() reports healthy with a version string", async () => { + const h = await client.health(); + assert.equal(h.healthy, true); + assert.ok(typeof h.version === "string" && h.version.length > 0, + `expected version string, got ${JSON.stringify(h.version)}`); + }); + + it("createSession returns an id", async () => { + const session = await client.createSession({ title: "test session" }); + assert.ok(typeof session.id === "string" && session.id.length > 0); + await client.deleteSession(session.id); + }); + + it("listSessions returns an array containing a freshly-created session", async () => { + const created = await client.createSession({ title: "list test" }); + try { + const sessions = await client.listSessions(); + assert.ok(Array.isArray(sessions)); + assert.ok( + sessions.some((s) => s.id === created.id), + "freshly-created session was not present in listSessions output" + ); + } finally { + await client.deleteSession(created.id); + } + }); + + it("createSession -> getSession -> deleteSession roundtrip", async () => { + const created = await client.createSession({ title: "roundtrip" }); + const fetched = await client.getSession(created.id); + assert.equal(fetched.id, created.id); + + await client.deleteSession(created.id); + + // After deletion, getSession should reject with a non-2xx status. + await assert.rejects( + () => client.getSession(created.id), + /OpenCode API GET .* returned 4\d\d/ + ); + }); +}); From 3f3cf73e2ca6a09ba7e54f9ce29944d5fd45b799 Mon Sep 17 00:00:00 2001 From: JohnnyVicious Date: Sun, 12 Apr 2026 15:19:03 +0200 Subject: [PATCH 2/2] fix(test,server): stop integration tests from hanging CI for 10 minutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run 24307439945 reported the integration tests passing in 0.3s, then node:test sat idle for the full 10-minute job timeout before the runner was forced to terminate an orphan opencode process. Two bugs: 1. Latent upstream bug in `lib/opencode-server.mjs#ensureServer`. It spawned `opencode serve` with `stdio: ["ignore", "pipe", "pipe"]` but never read the piped stdout/stderr. Those pipe FDs were ref'd on the parent's event loop, so any long-lived parent (notably node:test) would deadlock once opencode's log output filled the pipe buffer. In normal CLI usage the bug was masked because the companion script exited fast enough that the OS reaped the pipes. Switching to `stdio: "ignore"` redirects opencode's output to /dev/null, removing the parent-side handles entirely. This is a real upstream fix and has no production-side downside. 2. The test cleanup in `tests/opencode-server.test.mjs#after` killed only the spawned PID, not its process group, and had no SIGKILL fallback. Since opencode is spawned with `detached: true` it lives in its own group; SIGTERM-ing the leader doesn't guarantee the whole tree exits. Now we kill `-pid` (the group), poll for graceful exit for 3s via `kill(pid, 0)`, and SIGKILL the group as a last resort. Verified locally with `time OPENCODE_TEST_PORT=14096 npm test`: ℹ tests 62 ℹ pass 62 real 0m2.034s Port 14096 is empty after the run. --- .../opencode/scripts/lib/opencode-server.mjs | 17 ++++++++-- tests/opencode-server.test.mjs | 31 ++++++++++++++++--- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/plugins/opencode/scripts/lib/opencode-server.mjs b/plugins/opencode/scripts/lib/opencode-server.mjs index f4192a8..14614bd 100644 --- a/plugins/opencode/scripts/lib/opencode-server.mjs +++ b/plugins/opencode/scripts/lib/opencode-server.mjs @@ -1,6 +1,15 @@ // OpenCode HTTP API client. // Unlike codex-plugin-cc which uses JSON-RPC over stdin/stdout, // OpenCode exposes a REST API + SSE. This module wraps that API. +// +// Modified by JohnnyVicious (2026): `ensureServer` now spawns opencode +// with `stdio: "ignore"` instead of piping stdout/stderr that nothing +// reads. The piped streams were ref'd handles on the parent event loop, +// which deadlocked any long-lived parent (e.g. `node:test`) once +// opencode wrote enough log output to fill the pipe buffer. In normal +// CLI usage the deadlock was masked because the companion script exited +// before the buffer filled. (Apache License 2.0 §4(b) modification +// notice — see NOTICE.) import { spawn } from "node:child_process"; @@ -42,9 +51,13 @@ export async function ensureServer(opts = {}) { return { url, alreadyRunning: true }; } - // Start the server + // Start the server. + // `stdio: "ignore"` is critical: piping stdout/stderr without draining + // them creates ref'd file descriptors on the parent that prevent any + // long-lived parent (notably `node:test`) from exiting cleanly once + // opencode writes enough output to fill the pipe buffer. const proc = spawn("opencode", ["serve", "--port", String(port)], { - stdio: ["ignore", "pipe", "pipe"], + stdio: "ignore", detached: true, cwd: opts.cwd, }); diff --git a/tests/opencode-server.test.mjs b/tests/opencode-server.test.mjs index 88d261f..ea1eccf 100644 --- a/tests/opencode-server.test.mjs +++ b/tests/opencode-server.test.mjs @@ -49,17 +49,40 @@ describeOrSkip("opencode HTTP server (integration)", () => { client = createClient(serverInfo.url); }); - after(() => { + after(async () => { // Tear down the server we spawned. `ensureServer` only sets `pid` // when it actually started a new process, so this is a no-op when // the server was already running (which our `before` rejects, but // be defensive). - if (serverInfo?.pid && !serverInfo.alreadyRunning) { + if (!serverInfo?.pid || serverInfo.alreadyRunning) return; + + // opencode is spawned with `detached: true`, which puts it in its + // own process group. SIGTERM the negative pid to take down the + // whole group (any children opencode forked included). + const pgid = -serverInfo.pid; + try { + process.kill(pgid, "SIGTERM"); + } catch { + // group may already be gone — that's fine + } + + // Wait briefly for graceful shutdown, then SIGKILL the group if + // anything is still alive. We poll instead of sleeping a fixed + // interval so a fast exit doesn't waste time. + const deadline = Date.now() + 3_000; + while (Date.now() < deadline) { try { - process.kill(serverInfo.pid, "SIGTERM"); + process.kill(pgid, 0); // signal 0 = existence check } catch { - // already exited + return; // group is gone } + await new Promise((r) => setTimeout(r, 100)); + } + + try { + process.kill(pgid, "SIGKILL"); + } catch { + // already exited between checks } });