diff --git a/CHANGELOG.md b/CHANGELOG.md index de2549b..a1ffbdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- Never send a liveness signal to the host's own process when checking a persistence lease owner. On Windows, runtimes that emulate POSIX signals may treat `kill(pid, 0)` as a real termination, which matches reports of the OpenCode service dying silently with the plugin installed (#81). +- On OpenCode 2, tear down everything a partially failed `setup` already started (event subscription, timers, hook registrations, and the goal core with its persistence lease) instead of leaking it on every location boot. +- On Windows, treat `/` and `\` as the same separator when matching a location, so a reload with a differently spelled path retires the previous instance instead of running beside it. - Fix completion audits always failing on OpenCode 2.0.26. That host appends a `browser` deny rule to every agent, and the verifier ownership check compared permissions by exact JSON, so it rejected the plugin's own `goal-verify` agent and every completion was rejected. The check now accepts extra host-added deny rules, which only narrow access, and still rejects any changed, missing, reordered, allow or ask rule. Verified against live 2.0.16 and 2.0.26 hosts, including the foreign-verifier rejection mode. - When the auditor's prompt fails and no verdict reached the transcript, report `auditor prompt failed: ` instead of the misleading "no single clear final-line verdict". diff --git a/src/persistence-lease.js b/src/persistence-lease.js index 2cddc99..4cd414f 100644 --- a/src/persistence-lease.js +++ b/src/persistence-lease.js @@ -103,6 +103,10 @@ function persistenceLeaseHardLinkError() { function processIsAlive(pid) { if (!Number.isSafeInteger(pid) || pid <= 0) return null + // The host process is alive by definition. Never signal it: runtimes that + // emulate POSIX signals on Windows may map signal 0 to a real termination, + // which would kill the OpenCode service silently (issue #81). + if (pid === process.pid) return true try { process.kill(pid, 0) return true diff --git a/src/v2-bridge.js b/src/v2-bridge.js index c949719..633a1e6 100644 --- a/src/v2-bridge.js +++ b/src/v2-bridge.js @@ -100,7 +100,9 @@ function normalizeDirectory(value) { let end = directory.length while (end > 0 && (directory[end - 1] === "/" || directory[end - 1] === "\\")) end-- const trimmed = directory.slice(0, end) - return process.platform === "win32" ? trimmed.toLowerCase() : trimmed + // Windows accepts either separator, so one location can arrive spelled both + // ways. Unify them so reloads retire the previous same-location instance. + return process.platform === "win32" ? trimmed.replaceAll("/", "\\").toLowerCase() : trimmed } function sessionIDOf(input) { @@ -672,16 +674,58 @@ export function normalizeCommandName(value) { * @param {(context: {client: unknown, directory?: string}, options: object) => Promise} goalPluginFactory */ export function createV2Setup(goalPluginFactory) { + // Everything setup starts is tracked here so a setup that fails partway can + // still be torn down. Without that, a rejected setup leaked its event + // subscription, timers, hook registrations and persistence lease on every + // host location boot (issue #81). + const disposeInstance = (instance) => instance.disposePromise ||= (async () => { + const { host, timers, controller, registrations } = instance + host.disposed = true + for (const timer of timers) clearTimeout(timer) + timers.clear() + controller.abort() + for (const registration of [...registrations].reverse()) { + try { + await registration?.dispose?.() + } catch { + // Disposal is best-effort; registrations are also disposed on unload. + } + } + try { + await instance.hooks?.dispose?.() + } catch (error) { + logError("OpenCode 2 dispose hook failed", error?.stack || error?.message || error) + } + })() + const setupInstance = async function setup(ctx) { - const host = createHostState() + const instance = { + host: createHostState(), + registrations: [], + timers: new Set(), + controller: new AbortController(), + hooks: null, + disposePromise: null, + } + try { + await wireInstance(ctx, instance) + } catch (error) { + await disposeInstance(instance) + throw error + } + return () => disposeInstance(instance) + } + + async function wireInstance(ctx, instance) { + const { host, registrations, timers, controller } = instance const directory = ctx?.location?.directory || process.cwd() // V2's session API only speaks the flat argument shape; the compatibility // adapter probes the legacy generated-client shape otherwise. const options = { ...(isPlainObject(ctx?.options) ? ctx.options : {}), sdkShape: "flat" } const client = createV2Client(ctx, host, options) const hooks = await goalPluginFactory({ client, directory }, options) + instance.hooks = hooks - const registrations = [] const track = async (registration) => { registrations.push(registration) return registration @@ -958,7 +1002,6 @@ export function createV2Setup(goalPluginFactory) { /* ---------------- event stream ---------------- */ - const controller = new AbortController() const consumeEvents = async () => { try { for await (const event of ctx.event.subscribe({ signal: controller.signal })) { @@ -992,7 +1035,6 @@ export function createV2Setup(goalPluginFactory) { /* ---------------- deferred host-activation work ---------------- */ - const timers = new Set() const later = (fn, delay) => { const timer = setTimeout(() => { timers.delete(timer) @@ -1080,25 +1122,6 @@ export function createV2Setup(goalPluginFactory) { /* ---------------- cleanup ---------------- */ - let disposePromise - return () => disposePromise ||= (async () => { - host.disposed = true - for (const timer of timers) clearTimeout(timer) - timers.clear() - controller.abort() - for (const registration of [...registrations].reverse()) { - try { - await registration.dispose() - } catch { - // Disposal is best-effort; registrations are also disposed on unload. - } - } - try { - await hooks.dispose() - } catch (error) { - hookError("dispose", error) - } - })() } return async function setup(ctx) { // Host reloads can omit cleanup. Retire the old same-location instance diff --git a/test/persistence-lease.test.js b/test/persistence-lease.test.js index caef8b7..1e2dc97 100644 --- a/test/persistence-lease.test.js +++ b/test/persistence-lease.test.js @@ -135,6 +135,20 @@ async function acquireVersion1Lease( throw new Error("version-1 lease exhausted") } +test("the host's own pid is alive without being signalled", () => { + const original = process.kill + const signalled = [] + process.kill = (pid, signal) => { signalled.push([pid, signal]); return true } + try { + assert.equal(persistenceLeaseInternals.processIsAlive(process.pid), true) + assert.deepEqual(signalled, [], "signal 0 may terminate the caller on Windows runtimes (#81)") + persistenceLeaseInternals.processIsAlive(process.pid + 1) + assert.equal(signalled.length, 1, "other pids still get the liveness probe") + } finally { + process.kill = original + } +}) + test("persistence lease rejects a concurrent owner and releases by token", async () => { const dir = await mkdtemp(join(tmpdir(), "goal-lease-")) const state = join(dir, "state.json") diff --git a/test/v2-bridge.test.js b/test/v2-bridge.test.js index e9f18fa..561afb1 100644 --- a/test/v2-bridge.test.js +++ b/test/v2-bridge.test.js @@ -646,6 +646,35 @@ test("same-location setup retires the previous instance instead of stealing a li assert.equal(disposals, 2) }) +test("a setup that fails partway disposes everything it already started", async () => { + let disposals = 0 + const factory = async () => ({ config: async () => {}, dispose: async () => { disposals++ }, "command.execute.before": async () => {} }) + const { ctx, registrations } = createFakeCtx({ pluginOptions: { registerAgents: false } }) + ctx.session.hook = async () => ({ dispose: async () => { registrations.push("session:prompt") } }) + ctx.command.transform = async () => { throw new Error("host rejected command") } + await assert.rejects(createV2Setup(factory)(ctx), /host rejected command/) + assert.equal(disposals, 1, "the core (and its persistence lease) is released") + assert.ok(registrations.includes("session:prompt"), "hook registrations are released") + + // The failed attempt must not block the next boot of the same location. + const retry = createFakeCtx({ pluginOptions: { registerAgents: false } }).ctx + const cleanup = await createV2Setup(factory)(retry) + assert.equal(disposals, 1) + await cleanup() + assert.equal(disposals, 2) +}) + +test("on Windows, either separator spelling names the same location", () => { + const original = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { ...original, value: "win32" }) + try { + const event = { type: "session.execution.succeeded", data: { sessionID: "s1" }, location: { directory: "C:/Proj/App/" } } + assert.equal(eventBelongsToLocation(event, "c:\\proj\\app"), true) + } finally { + Object.defineProperty(process, "platform", original) + } +}) + test("setup registers every V2 surface the goal workflow needs", async () => { const { ctx, registered } = createFakeCtx() const cleanup = await createV2Setup(GoalPlugin)(ctx)