diff --git a/plugins/opencode/scripts/lib/process.mjs b/plugins/opencode/scripts/lib/process.mjs index f04a1b5..32773e9 100644 --- a/plugins/opencode/scripts/lib/process.mjs +++ b/plugins/opencode/scripts/lib/process.mjs @@ -174,6 +174,46 @@ export function commandLineLooksLikeTaskWorker(commandLine, { jobId } = {}) { ); } +function tokensContainPort(tokens, port) { + const expected = String(port ?? ""); + if (!expected) { + return false; + } + + for (let index = 0; index < tokens.length; index += 1) { + if (tokens[index] === "--port" && tokens[index + 1] === expected) { + return true; + } + if (tokens[index] === `--port=${expected}`) { + return true; + } + } + return false; +} + +export function commandLineLooksLikeOpencodeServe(commandLine, { port } = {}) { + // On win32 the server is spawned through a shell, so `ps`/PowerShell can + // report the whole invocation as one quoted blob (`cmd.exe /c "opencode + // serve --port N"`). Re-split compound tokens so the matcher sees the real + // arguments on every platform. + const tokens = commandLineTokens(commandLine).flatMap((token) => + /\s/.test(token) ? token.split(/\s+/).filter(Boolean) : [token] + ); + // Require an opencode-ish executable token in addition to `serve --port N`. + // Matches the real binary (/opt/homebrew/bin/opencode), test fixtures + // (/opencode), and Windows shims (opencode.cmd). + if (!tokens.some((token) => path.basename(token).toLowerCase().startsWith("opencode"))) { + return false; + } + if (!tokens.includes("serve")) { + return false; + } + if (!tokensContainPort(tokens, port)) { + return false; + } + return true; +} + export function readProcessCommandLine(pid, options = {}) { if (!Number.isFinite(pid)) { return null; diff --git a/plugins/opencode/scripts/lib/server-lifecycle.mjs b/plugins/opencode/scripts/lib/server-lifecycle.mjs index e01992b..50be8f5 100644 --- a/plugins/opencode/scripts/lib/server-lifecycle.mjs +++ b/plugins/opencode/scripts/lib/server-lifecycle.mjs @@ -7,6 +7,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { OpencodeServerClient } from "./opencode-server.mjs"; +import { commandLineLooksLikeOpencodeServe, readProcessCommandLine } from "./process.mjs"; import { atomicWriteFile, resolveStateDir } from "./state.mjs"; export const SERVER_URL_ENV = "OPENCODE_COMPANION_SERVER_URL"; @@ -432,7 +433,7 @@ export async function ensureServer(cwd, options = {}) { const staleExisting = loadServerSession(cwd); if (staleExisting) { - const { url, pidFile, logFile, sessionDir, pid, external, password, username } = staleExisting; + const { url, pidFile, logFile, sessionDir, pid, external, password, username, port } = staleExisting; // The server lock is already held here; intentionally omit cwd so teardown // uses the unlocked path even if the persisted session schema grows. await teardownServerSession({ @@ -444,7 +445,9 @@ export async function ensureServer(cwd, options = {}) { external, password, username, - killProcess: options.killProcess ?? null + port, + killProcess: options.killProcess ?? null, + readProcessCommandLineImpl: options.readProcessCommandLineImpl ?? null }); clearServerSession(cwd); } @@ -467,6 +470,10 @@ export async function ensureServer(cwd, options = {}) { env: options.env ?? process.env, password }); + // Recorded for forensics (compare against the live command line when an + // identity-mismatch teardown skip is investigated); verification itself + // matches the LIVE command line against `opencode serve --port `. + const pidCommandLine = readProcessCommandLine(child.pid, options); const ready = await waitForServerHealth(url, options.timeoutMs ?? 10000, { password, @@ -481,7 +488,9 @@ export async function ensureServer(cwd, options = {}) { pid: child.pid ?? null, password, username: OWNED_SERVER_USERNAME, - killProcess: options.killProcess ?? null + port, + killProcess: options.killProcess ?? null, + readProcessCommandLineImpl: options.readProcessCommandLineImpl ?? null }); return null; } @@ -494,7 +503,9 @@ export async function ensureServer(cwd, options = {}) { sessionDir, external: false, password, - username: OWNED_SERVER_USERNAME + username: OWNED_SERVER_USERNAME, + port, + pidCommandLine }; const leasedSession = addServerLease(session, options); saveServerSession(cwd, leasedSession); @@ -504,6 +515,37 @@ export async function ensureServer(cwd, options = {}) { } } +function parseServerUrlPort(url) { + try { + const port = Number(new URL(String(url ?? "")).port); + return Number.isInteger(port) && port > 0 ? port : null; + } catch { + return null; + } +} + +// Fail-closed PID identity check (issue #31): only signal a PID when its live +// command line still looks like the plugin-owned `opencode serve` instance for +// this session's port. The port always exists for owned sessions — it is part +// of the persisted URL — so records written before the explicit identity +// fields existed remain verifiable. Anything unverifiable is left untouched; +// only the stale metadata is cleared. +function verifyServerPidIdentity(pid, { url = null, port = null, readProcessCommandLineImpl = null } = {}) { + const expectedPort = Number.isFinite(port) ? Number(port) : parseServerUrlPort(url); + if (!Number.isFinite(expectedPort)) { + return { verified: false, reason: "identity-unverified" }; + } + const readCommandLine = readProcessCommandLineImpl ?? readProcessCommandLine; + const commandLine = readCommandLine(pid); + if (!commandLine) { + return { verified: false, reason: "identity-unverified" }; + } + if (!commandLineLooksLikeOpencodeServe(commandLine, { port: expectedPort })) { + return { verified: false, reason: "identity-mismatch" }; + } + return { verified: true, reason: null }; +} + async function teardownServerSessionUnlocked({ url = null, pidFile = null, @@ -513,7 +555,9 @@ async function teardownServerSessionUnlocked({ external = false, password = null, username = null, - killProcess = null + killProcess = null, + port = null, + readProcessCommandLineImpl = null } = {}) { if (url && !external) { try { @@ -526,11 +570,17 @@ async function teardownServerSessionUnlocked({ } } + let killSkippedReason = null; if (!external && Number.isFinite(pid)) { - try { - killServerPid(pid, killProcess); - } catch { - // Ignore teardown failures during Claude session shutdown. + const identity = verifyServerPidIdentity(pid, { url, port, readProcessCommandLineImpl }); + if (identity.verified) { + try { + killServerPid(pid, killProcess); + } catch { + // Ignore teardown failures during Claude session shutdown. + } + } else { + killSkippedReason = identity.reason; } } @@ -550,6 +600,18 @@ async function teardownServerSessionUnlocked({ } } + if (killSkippedReason) { + // Metadata is cleared (so callers still clear the session record), but the + // process was deliberately left untouched. `skipped` keeps its existing + // meaning of "teardown did not run at all" (leases / lock timeouts). + return { + skipped: false, + killSkipped: true, + reason: killSkippedReason, + diagnostic: `Left PID ${pid} untouched (${killSkippedReason}); cleared stale OpenCode server metadata only.` + }; + } + return { skipped: false }; } @@ -568,7 +630,9 @@ export async function teardownServerSession({ external = false, password = null, username = null, - killProcess = null + killProcess = null, + port = null, + readProcessCommandLineImpl = null } = {}) { if (!cwd) { return teardownServerSessionUnlocked({ @@ -580,7 +644,9 @@ export async function teardownServerSession({ external, password, username, - killProcess + killProcess, + port, + readProcessCommandLineImpl }); } @@ -615,7 +681,9 @@ export async function teardownServerSession({ external: Boolean(session?.external ?? external), password: session?.password ?? password, username: session?.username ?? username, - killProcess + killProcess, + port: session?.port ?? port, + readProcessCommandLineImpl }; const result = await teardownServerSessionUnlocked(teardownTarget); if (session) { diff --git a/plugins/opencode/scripts/opencode-companion.mjs b/plugins/opencode/scripts/opencode-companion.mjs index fb4be68..4a09b2c 100644 --- a/plugins/opencode/scripts/opencode-companion.mjs +++ b/plugins/opencode/scripts/opencode-companion.mjs @@ -1122,11 +1122,14 @@ async function handleCancel(argv) { ? serverExternal === false : interrupt.serverExternal !== true; if (ownedServerTeardown) { - await teardownServerSession({ + const teardownResult = await teardownServerSession({ cwd: workspaceRoot, ignoreCurrentProcessLease: Boolean(interrupt.serverUrl && !serverUrl), ...(teardownServerUrl ? { url: teardownServerUrl } : {}) }); + if (teardownResult?.killSkipped) { + appendLogLine(cancelLogFile, `Skipped OpenCode server kill: ${teardownResult.reason}.`); + } } } catch (error) { appendLogLine( diff --git a/tests/server-lifecycle.test.mjs b/tests/server-lifecycle.test.mjs index 1d77341..0e85419 100644 --- a/tests/server-lifecycle.test.mjs +++ b/tests/server-lifecycle.test.mjs @@ -9,6 +9,7 @@ import assert from "node:assert/strict"; import { makeTempDir } from "./helpers.mjs"; import { ensureServer, isServerHealthy, loadServerSession, saveServerSession, teardownServerSession } from "../plugins/opencode/scripts/lib/server-lifecycle.mjs"; import { resolveStateDir } from "../plugins/opencode/scripts/lib/state.mjs"; +import { commandLineLooksLikeOpencodeServe } from "../plugins/opencode/scripts/lib/process.mjs"; async function canListenLocalhost() { return new Promise((resolve) => { @@ -233,7 +234,8 @@ test("teardownServerSession can ignore only this process lease", async () => { ignoreCurrentProcessLease: true, killProcess: (pid) => { killedPid = pid; - } + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 1" }); assert.equal(result.skipped, false); @@ -443,7 +445,8 @@ test("teardownServerSession expires a lease whose pid reports EPERM", async () = pid: session.pid, killProcess: (pid) => { killedPid = pid; - } + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 1" }); assert.equal(result.skipped, false); @@ -459,6 +462,222 @@ test("teardownServerSession expires a lease whose pid reports EPERM", async () = } }); +test("teardownServerSession signals the process when the persisted identity matches the live command line", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 8080" + }); + + assert.equal(result.skipped, false); + assert.equal(killedPid, session.pid); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession does NOT signal and clears the record when the live command line differs (PID reused)", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "node /opt/unrelated/server.js --port 3000" + }); + + // The teardown itself ran (metadata cleared, record removed); only the + // kill was withheld — `skipped` keeps meaning "teardown did not run". + assert.equal(result.skipped, false); + assert.equal(result.killSkipped, true); + assert.equal(result.reason, "identity-mismatch"); + assert.match(result.diagnostic, /Left PID 123456 untouched/); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession does NOT signal when the command line cannot be read", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => null + }); + + assert.equal(result.skipped, false); + assert.equal(result.killSkipped, true); + assert.equal(result.reason, "identity-unverified"); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession verifies legacy records via the port derived from the persisted url", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + // A record written before the identity fields existed: no `port`, no + // `pidCommandLine`. Verification must still work (the URL carries the port) + // instead of falling open and killing blind. + const session = { + url: "http://127.0.0.1:43117", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + leases: [] + }; + saveServerSession(workspace, session); + + try { + let killedPid = null; + const matched = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "/usr/local/bin/opencode serve --hostname 127.0.0.1 --port 43117" + }); + assert.equal(matched.skipped, false); + assert.equal(matched.killSkipped, undefined); + assert.equal(killedPid, session.pid); + assert.equal(loadServerSession(workspace), null); + + // Same legacy record, but the PID now belongs to something else: no kill. + saveServerSession(workspace, session); + killedPid = null; + const reused = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "postgres: writer process" + }); + assert.equal(reused.killSkipped, true); + assert.equal(reused.reason, "identity-mismatch"); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("commandLineLooksLikeOpencodeServe matches shell-wrapped Windows command lines", () => { + assert.equal( + commandLineLooksLikeOpencodeServe('C:\\Windows\\system32\\cmd.exe /c "opencode serve --hostname 127.0.0.1 --port 5150"', { + port: 5150 + }), + true + ); + assert.equal( + commandLineLooksLikeOpencodeServe("/opt/homebrew/bin/opencode serve --hostname 127.0.0.1 --port 5150", { port: 5150 }), + true + ); + // Same port, different program: must not match. + assert.equal( + commandLineLooksLikeOpencodeServe("node /srv/other-tool serve --port 5150", { port: 5150 }), + false + ); + // Right program, wrong port: must not match. + assert.equal( + commandLineLooksLikeOpencodeServe("/opt/homebrew/bin/opencode serve --hostname 127.0.0.1 --port 5151", { port: 5150 }), + false + ); +}); + test("saveServerSession preserves the existing session when its atomic rename fails", () => { const workspace = makeTempDir(); const pluginDataDir = makeTempDir();