From 1ada8f5ff1b902aa15deb72d38349488c8bcbdde Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 08:20:19 +0900 Subject: [PATCH 1/2] fix(stop): name the real refusal cause instead of asserting ownership POST /api/stop refuses for three distinct reasons - respawnable_service, self_unload_service and service_state_unknown - but when no readable message arrived, stopProxy fell back to a single hardcoded sentence naming a fourth cause the server never reports: a CODEX_HOME/OPENCODEX_HOME ownership mismatch. The reporter's homes matched exactly and the server had answered respawnable_service, so three attempts went into re-exporting CODEX_HOME. Carried from #4170 by @yeongjunyoo: the refusal code is captured next to the message, the fallback wording is selected from that code, and the refusal is returned per attempt rather than published to module state, so two overlapping stops cannot lend each other the wrong cause. This also closes the second half of the issue, which the carry left open. #4169 asks that "the recommended next command should not be the command the operator just ran", and it is still is: ocx stop reaches POST /api/stop through handleStop -> stopWithDeferral -> stopProxy, and the server answers "the stop must be run by `ocx stop`" because that refusal is written for an API client. The CLI echoes it verbatim, so following the advice returns the operator to the same message. The carried fallback said "Run `ocx stop`" too, making the empty-body path a tighter loop than before. The refusal code now travels on the error, the fallback messages name the cause only, and refusalNextStep supplies the command. The only callers of stopProxy are ocx stop and the service manager's own cleanup, and by the time either reaches this point the service manager has already been asked to stop, so no branch answers with the command that just failed - every one points at `ocx service status`, which is what actually reports the wrapper. ProxyOwnershipRefusedError keeps its name. #4169 suggests renaming it and the name does overstate what it carries, but the carrying author deliberately left that out as a separate wider change and it is not part of the issue's expected behaviour. Closes #4169 Co-authored-by: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com> --- src/cli/index.ts | 13 +- src/lib/process-control.ts | 150 +++++++++++++++++--- tests/lib/process-control-graceful.test.ts | 151 ++++++++++++++++++++- tests/providers/xai/grok-lifecycle.test.ts | 25 +++- 4 files changed, 308 insertions(+), 31 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index d59f9e3257..2589f1eb5d 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -68,7 +68,7 @@ import { findLiveProxy, probeHostname, type LiveProxy } from "../server/proxy-li import { createReadinessGate } from "../server/readiness"; import { runReady, type ReadyArgs } from "./ready"; import { runCli } from "./root"; -import { isProcessAlive, ProxyOwnershipRefusedError, stopProxy } from "../lib/process-control"; +import { isProcessAlive, ProxyOwnershipRefusedError, refusalNextStep, stopProxy } from "../lib/process-control"; import { loadServiceTokenFromFile } from "../lib/service-secrets"; import { assertNotAdminToken, diagnoseService, isServiceOwnershipError, proxyStillLiveAfterStop, serviceCommand, serviceEnvironmentOwnedHere, serviceStartableFromTray, serviceStatusSummary, stopServiceIfInstalledDetailed, uninstallServiceIfInstalled, uninstallServiceDetailed } from "../service"; import { formatStartupRoutingDetail, startupHealthSummary } from "../codex/autostart-health"; @@ -952,7 +952,12 @@ async function handleStop() { if (detail) console.error(` ${detail}`); if (err instanceof ProxyOwnershipRefusedError) { ownershipBlocked = true; - console.error(" Skipping shared teardown (native Codex restore, Grok config): the foreign proxy is still running."); + // Every refusal `POST /api/stop` produces is written for an API client, so it + // recommends `ocx stop` — the command printing it. Following that advice returns + // the operator to this exact message, which is the loop #4169 was filed for. The + // service manager was already asked to stop above, so name what is actually left. + console.error(` ${refusalNextStep(err.code)}`); + console.error(" Skipping shared teardown (native Codex restore, Grok config): the refusing proxy is still running."); } } } else { @@ -979,7 +984,9 @@ async function handleStop() { if (detail) console.error(` ${detail}`); if (err instanceof ProxyOwnershipRefusedError) { ownershipBlocked = true; - console.error(" Skipping shared teardown (native Codex restore, Grok config): the foreign proxy is still running."); + // Same loop as the tracked-pid path above: the refusal recommends this command. + console.error(` ${refusalNextStep(err.code)}`); + console.error(" Skipping shared teardown (native Codex restore, Grok config): the refusing proxy is still running."); } } } else if (live) { diff --git a/src/lib/process-control.ts b/src/lib/process-control.ts index 49da43987c..165d21aeaa 100644 --- a/src/lib/process-control.ts +++ b/src/lib/process-control.ts @@ -80,17 +80,96 @@ export type GracefulStopResult = boolean | "refused" | "teardown-unconfirmed"; */ let lastRefusalMessage: string | null = null; +/** + * The server's machine-readable reason for the most recent 409, captured alongside the + * message so a refusal that arrives without a body still names the right cause. Without it + * the fallback has to guess, and guessing "ownership" sent operators to re-check + * CODEX_HOME for a refusal the scheduler wrapper had issued (#4169). + */ +let lastRefusalCode: string | null = null; + /** The server's explanation for the most recent 409, or `null` when it sent none. */ export function lastStopRefusalMessage(): string | null { return lastRefusalMessage; } +/** The server's `code` for the most recent 409, or `null` when it sent none. */ +export function lastStopRefusalCode(): string | null { + return lastRefusalCode; +} + +/** + * Wording for a refusal whose body carried no message. Each branch mirrors a refusal the + * management API can return from `POST /api/stop`; the default stays cause-neutral because + * naming the wrong cause is worse than naming none — it costs the operator the time they + * spend acting on it. + * + * These name the cause only. The command belongs to {@link refusalNextStep}, because the + * only callers of `stopProxy` are `ocx stop` and the service manager's own cleanup, and a + * message that told either of them to run `ocx stop` would be the #4169 loop again. + */ +function refusalFallbackMessage(code: string | null): string { + switch (code) { + case "respawnable_service": + return "The running proxy refused to stop: a service manager that can respawn it owns " + + "the process."; + case "self_unload_service": + return "The running proxy refused to stop: it is the installed service itself, so " + + "stopping the manager from inside it would end the process before native Codex is " + + "restored."; + case "service_state_unknown": + return "The running proxy refused to stop: the service manager state could not be read, " + + "so it cannot tell whether a wrapper would respawn it."; + default: + return "The running proxy refused to stop and sent no reason."; + } +} + +/** + * What is actually left to do when `ocx stop` is the command that received the refusal. + * + * Every refusal `POST /api/stop` produces is written for an API client, so it recommends + * `ocx stop` — which is the command already running when the CLI prints it. That is the + * loop #4169 reports: the endpoint points at `ocx stop`, `ocx stop` repeats the endpoint, + * and neither names the wrapper that is refusing. `ocx stop` has already asked the service + * manager to stop by the time this is reached, so the remaining question is always what the + * service manager is doing, and no branch may answer with the command that just failed. + */ +export function refusalNextStep(code: string | null): string { + switch (code) { + case "respawnable_service": + return "This stop already asked the service manager to stop, so running `ocx stop` " + + "again is not the missing step. Run `ocx service status` to see whether a wrapper " + + "is still installed and able to respawn the proxy."; + case "self_unload_service": + return "This stop already asked the service manager to stop, so running `ocx stop` " + + "again is not the missing step. Run `ocx service status` to see whether the service " + + "is still registered."; + case "service_state_unknown": + return "Run `ocx service status` to see the query error, repair the service manager " + + "access, then retry."; + default: + return "Run `ocx service status` to inspect the service state."; + } +} + /** * A proxy declined shutdown (HTTP 409). There is more than one reason it can say no — a * scheduler wrapper under another home, or the proxy being the installed service itself * (#4023) — so the server's own message is carried through rather than guessed at. + * + * The refusal's `code` travels on the error because the reporting caller has to act on the + * cause, not re-parse prose: the message is the server's, and it recommends a command the + * CLI has already run. */ -export class ProxyOwnershipRefusedError extends Error {} +export class ProxyOwnershipRefusedError extends Error { + readonly code: string | null; + + constructor(message: string, code: string | null = null) { + super(message); + this.code = code; + } +} /** * Ask a running proxy to stop itself via the management API (`POST /api/stop`), which @@ -104,9 +183,28 @@ export class ProxyOwnershipRefusedError extends Error {} * attest the process exit code or completion of every drain/shutdown hook. */ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): Promise { + return (await stopProxyGracefullyDetailed(pid, io)).result; +} + +/** + * The refusal a single stop attempt received, carried back to that attempt's caller. + * + * Module-scoped state cannot do this job: two overlapping stops race, and the first would + * report the second's cause. The exported accessors stay as observational state for callers + * that only want the last refusal, but the error text is built from this per-call value. + */ +type StopRefusal = { message: string | null; code: string | null }; + +async function stopProxyGracefullyDetailed( + pid: number, + io: GracefulStopIo = {}, +): Promise<{ result: GracefulStopResult; refusal: StopRefusal }> { + const refusal: StopRefusal = { message: null, code: null }; + const done = (result: GracefulStopResult): { result: GracefulStopResult; refusal: StopRefusal } => + ({ result, refusal }); const readRuntime = io.readRuntime ?? readRuntimePort; const runtime = io.runtimeEndpoint ?? readRuntime(pid); - if (!runtime?.port) return false; + if (!runtime?.port) return done(false); const env = io.env ?? process.env; const headers: Record = {}; const token = configuredAdminToken(env.OPENCODEX_HOME?.trim() || undefined, env as NodeJS.ProcessEnv); @@ -128,20 +226,31 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): // longer than a health poll so we prefer drain over taskkill /F. signal: AbortSignal.timeout(io.exitTimeoutMs ? Math.min(io.exitTimeoutMs, 10_000) : 10_000), }); - // 409 is the proxy REFUSING to stop (a service installed under another home owns it and - // would respawn it anyway). That is a policy answer, not a dead endpoint — escalating to - // SIGTERM here would run the daemon's cleanup and strip shared config out from under the + // 409 is the proxy REFUSING to stop. There is more than one reason it can say no — a + // respawning service manager, the proxy being the installed service itself, or an + // unreadable scheduler state — so both the message and the code are captured rather + // than assumed. That is a policy answer, not a dead endpoint — escalating to SIGTERM + // here would run the daemon's cleanup and strip shared config out from under the // still-running service. Report the refusal instead of forcing. if (res.status === 409) { - lastRefusalMessage = await res.json() + const parsed = await res.json() .then(body => { - const message = (body as { message?: unknown } | null)?.message; - return typeof message === "string" && message.trim() ? message.trim() : null; + const record = body as { message?: unknown; code?: unknown } | null; + const message = record?.message; + const code = record?.code; + return { + message: typeof message === "string" && message.trim() ? message.trim() : null, + code: typeof code === "string" && code.trim() ? code.trim() : null, + }; }) - .catch(() => null); - return "refused"; + .catch(() => ({ message: null, code: null })); + refusal.message = parsed.message; + refusal.code = parsed.code; + lastRefusalMessage = parsed.message; + lastRefusalCode = parsed.code; + return done("refused"); } - if (!res.ok) return false; + if (!res.ok) return done(false); const body: unknown = await res.json().catch(() => null); const expectedTeardown = io.deferSharedTeardownNonce ? "deferred" : "performed"; sharedTeardownConfirmed = body !== null @@ -150,14 +259,14 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): && "success" in body && body.success === true && "sharedTeardown" in body && body.sharedTeardown === expectedTeardown; } catch { - return false; + return done(false); } const waitExit = io.waitExit ?? waitForExit; // Honor the server's own drain window: /api/stop answers 200 first, then drains for // config.shutdownTimeoutMs. Waiting less than that hard-kills mid-drain. const exitTimeoutMs = io.exitTimeoutMs ?? drainDeadlineMs(); - if (!waitExit(pid, exitTimeoutMs)) return false; - return sharedTeardownConfirmed ? true : "teardown-unconfirmed"; + if (!waitExit(pid, exitTimeoutMs)) return done(false); + return done(sharedTeardownConfirmed ? true : "teardown-unconfirmed"); } function drainDeadlineMs(): number { @@ -172,14 +281,15 @@ function drainDeadlineMs(): number { export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise { if (!isProcessAlive(pid)) return false; const runtime = io.runtimeEndpoint ?? readRuntimePort(pid); - const graceful = await stopProxyGracefully(pid, io); + const { result: graceful, refusal } = await stopProxyGracefullyDetailed(pid, io); if (graceful === "refused") { - // The proxy refused on purpose (foreign service owns it). Forcing would strip shared - // config while that service keeps the proxy alive. + // The proxy refused on purpose. Forcing would strip shared config while whatever owns + // the process keeps it alive. The server's own message is preferred; the fallback is + // selected from its code so an empty body still names the right cause. Both come from + // THIS attempt, so an overlapping stop cannot lend it the wrong reason. throw new ProxyOwnershipRefusedError( - lastRefusalMessage - ?? "The running proxy refused to stop: a service installed under a different " - + "CODEX_HOME/OPENCODEX_HOME owns it. Run the stop from that home.", + refusal.message ?? refusalFallbackMessage(refusal.code), + refusal.code, ); } if (graceful === "teardown-unconfirmed") { diff --git a/tests/lib/process-control-graceful.test.ts b/tests/lib/process-control-graceful.test.ts index 9fad73d38b..9f1f2fe7fc 100644 --- a/tests/lib/process-control-graceful.test.ts +++ b/tests/lib/process-control-graceful.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { gracefulStopHost, lastStopRefusalMessage, stopProxyGracefully } from "../../src/lib/process-control"; +import { gracefulStopHost, lastStopRefusalCode, lastStopRefusalMessage, ProxyOwnershipRefusedError, refusalNextStep, stopProxy, stopProxyGracefully } from "../../src/lib/process-control"; function okResponse(): Response { return new Response(JSON.stringify({ success: true, sharedTeardown: "performed" }), { status: 200 }); @@ -198,4 +198,153 @@ describe("409 refusal reporting", () => { expect(result).toBe("refused"); expect(lastStopRefusalMessage()).toBeNull(); }); + + test("the refusal code is captured alongside the message", async () => { + // The message alone cannot drive the fallback: a refusal that arrives with an empty or + // unparseable body still has to name a cause, and #4169 showed what happens when the + // fallback guesses one — the operator re-checks CODEX_HOME for a refusal the scheduler + // wrapper issued. + await stopProxyGracefully(7, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response( + JSON.stringify({ success: false, code: "respawnable_service", message: "wrapper owns it" }), + { status: 409, headers: { "content-type": "application/json" } }, + )) as typeof fetch, + waitExit: () => true, + env: {}, + }); + expect(lastStopRefusalCode()).toBe("respawnable_service"); + + await stopProxyGracefully(7, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response("not json", { status: 409 })) as typeof fetch, + waitExit: () => true, + env: {}, + }); + expect(lastStopRefusalCode()).toBeNull(); + }); + + test("a refusal without a message falls back by code, never to an ownership claim", async () => { + const refusalFor = async (code: string | null): Promise => { + const body = code === null ? "not json" : JSON.stringify({ success: false, code }); + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response(body, { + status: 409, + headers: { "content-type": "application/json" }, + })) as typeof fetch, + waitExit: () => { throw new Error("must not wait for a refused stop"); }, + env: {}, + }); + } catch (err) { + if (err instanceof ProxyOwnershipRefusedError) return err.message; + throw err; + } + throw new Error("stopProxy must throw on a refusal"); + }; + + const respawnable = await refusalFor("respawnable_service"); + expect(respawnable).toContain("respawn"); + // Not `ocx stop`: the only callers of stopProxy are `ocx stop` and the service + // manager's own cleanup, so recommending it here is the #4169 loop. The fallback names + // the cause and refusalNextStep names the command. + expect(respawnable).not.toContain("ocx stop"); + + const selfUnload = await refusalFor("self_unload_service"); + expect(selfUnload).toContain("installed service itself"); + expect(selfUnload).not.toContain("ocx stop"); + + const unknownState = await refusalFor("service_state_unknown"); + expect(unknownState).toContain("could not be read"); + expect(unknownState).not.toContain("ocx stop"); + + const noBody = await refusalFor(null); + expect(noBody).toContain("sent no reason"); + expect(noBody).not.toContain("ocx stop"); + + // None of them may assert the cause that #4169 was filed for. + for (const message of [respawnable, selfUnload, unknownState, noBody]) { + expect(message).not.toContain("CODEX_HOME"); + expect(message).not.toContain("OPENCODEX_HOME"); + } + }); + + test("the refusal carries its code to the caller that has to report it", async () => { + // The reporting caller acts on the cause. Re-parsing the prose is not an option: the + // message is the server's, and the server's message is exactly what recommends the + // command that already failed. + let thrown: unknown; + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response( + JSON.stringify({ success: false, code: "respawnable_service", message: "wrapper owns it" }), + { status: 409, headers: { "content-type": "application/json" } }, + )) as typeof fetch, + waitExit: () => { throw new Error("must not wait for a refused stop"); }, + env: {}, + }); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(ProxyOwnershipRefusedError); + expect((thrown as ProxyOwnershipRefusedError).code).toBe("respawnable_service"); + // The server's own message still wins, unchanged. + expect((thrown as ProxyOwnershipRefusedError).message).toBe("wrapper owns it"); + }); + + test("no next step sends the operator back to the command that just refused", () => { + // #4169's loop: POST /api/stop answers "the stop must be run by `ocx stop`", and + // `ocx stop` reprints it. Whatever the cause, the next step has to be something else, + // because the service manager was already asked to stop before this point. + for (const code of ["respawnable_service", "self_unload_service", "service_state_unknown", null]) { + const step = refusalNextStep(code); + // Naming `ocx stop` in order to rule it out is the point; recommending it is the loop. + expect(step).not.toMatch(/Run `ocx stop`/); + expect(step).toContain("ocx service status"); + } + // The two service causes say why repeating the stop is not the missing step, since the + // server's message printed just above them recommends exactly that. + expect(refusalNextStep("respawnable_service")).toContain("already asked the service manager"); + expect(refusalNextStep("self_unload_service")).toContain("already asked the service manager"); + }); + + test("concurrent refusals each keep their own cause", async () => { + // Reading the reason from module state lets one stop publish its refusal and a second + // overwrite it before the first continuation consumes it. Starting both together is + // what actually reproduces that: verified against the pre-fix global handoff, where + // this schedule fails with the first call throwing the second's cause + // ("...it is the installed service itself..." for the respawnable_service stop). + // A schedule that lets one call finish entirely before resuming the other does NOT + // discriminate — the parked call republishes its own globals last and passes either way. + const refusalOf = (code: string) => async (): Promise => { + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response(JSON.stringify({ success: false, code }), { + status: 409, + headers: { "content-type": "application/json" }, + })) as typeof fetch, + waitExit: () => { throw new Error("must not wait for a refused stop"); }, + env: {}, + }); + } catch (err) { + if (err instanceof ProxyOwnershipRefusedError) return err.message; + throw err; + } + throw new Error("stopProxy must throw on a refusal"); + }; + + // Repeated because the interleaving is scheduler-dependent; the pre-fix code fails on + // the first iteration, but a single run would be a weak guard against reintroduction. + for (let i = 0; i < 20; i++) { + const [respawnable, selfUnload] = await Promise.all([ + refusalOf("respawnable_service")(), + refusalOf("self_unload_service")(), + ]); + expect(respawnable).toContain("respawn"); + expect(selfUnload).toContain("installed service itself"); + } + }); }); diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index 88212768af..e4d69b599f 100644 --- a/tests/providers/xai/grok-lifecycle.test.ts +++ b/tests/providers/xai/grok-lifecycle.test.ts @@ -150,8 +150,14 @@ describe("Grok fence lifecycle wiring", () => { // shared teardown must be skipped at both call sites, exactly like the service-manager path. const ownershipRefusals = stopFn.match(/err instanceof ProxyOwnershipRefusedError[\s\S]{0,200}?ownershipBlocked = true;/g); expect(ownershipRefusals).toHaveLength(2); - expect(stopFn.match(/Skipping shared teardown \(native Codex restore, Grok config\): the foreign proxy is still running\./g)).toHaveLength(2); + expect(stopFn.match(/Skipping shared teardown \(native Codex restore, Grok config\): the refusing proxy is still running\./g)).toHaveLength(2); expect(PROCESS_CONTROL_SOURCE).toContain("throw new ProxyOwnershipRefusedError("); + + // Both sites also print what is actually left to do. The refusal itself is written for + // an API client, so it recommends `ocx stop` — the command doing the printing — which + // is the loop #4169 reports. Echoing the server's message alone reproduces it. + expect(stopFn.match(/console\.error\(` \$\{refusalNextStep\(err\.code\)\}`\);/g)).toHaveLength(2); + expect(PROCESS_CONTROL_SOURCE).toContain("export function refusalNextStep("); }); test("handleStop returns its outcome while both restart surfaces share the in-place lifecycle", () => { @@ -507,10 +513,12 @@ describe("POST /api/stop teardown", () => { }); test("a 409 does not escalate to a forced kill", () => { - // Escalating would run the daemon's cleanup and strip shared config while the foreign - // service keeps the proxy alive — the exact hole the ownership gate exists to close. + // Escalating would run the daemon's cleanup and strip shared config while the refusing + // service keeps the proxy alive — the exact hole the refusal gate exists to close. // The 409 branch may capture the server's reason first (#4023 added a second refusal - // cause), but it must still return "refused" without falling through to !res.ok. + // cause, #4169 the code that names it), but it must still yield "refused" without + // falling through to !res.ok. Matched loosely so a wrapped return (`done("refused")`) + // still satisfies the invariant this guards, which is ordering, not spelling. const stopGracefully = sliceFn( PROCESS_CONTROL_SOURCE, "export async function stopProxyGracefully(", @@ -518,9 +526,12 @@ describe("POST /api/stop teardown", () => { ); const four09At = stopGracefully.indexOf("res.status === 409"); expect(four09At).toBeGreaterThan(-1); - expect(stopGracefully.slice(four09At)).toContain('return "refused"'); - expect(stopGracefully.indexOf('return "refused"', four09At)) - .toBeLessThan(stopGracefully.indexOf("if (!res.ok) return false;", four09At)); + const refusedReturn = /return (?:done\()?"refused"/; + const okFallthrough = /if \(!res\.ok\) return (?:done\()?false/; + const afterFour09 = stopGracefully.slice(four09At); + expect(afterFour09).toMatch(refusedReturn); + expect(afterFour09.search(refusedReturn)) + .toBeLessThan(afterFour09.search(okFallthrough)); const stopProxyFn = sliceFn(PROCESS_CONTROL_SOURCE, "export async function stopProxy(", "export function killProxy("); const refusedAt = stopProxyFn.indexOf('graceful === "refused"'); From 9e4654afb046cf50e8c658b2eb00f14907c2b74e Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 08:24:09 +0900 Subject: [PATCH 2/2] docs(devlog): record the wp2 stop-refusal work-phase --- .../020_wp2_stop_refusal.md | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 devlog/_plan/260911_l4_service_cli/020_wp2_stop_refusal.md diff --git a/devlog/_plan/260911_l4_service_cli/020_wp2_stop_refusal.md b/devlog/_plan/260911_l4_service_cli/020_wp2_stop_refusal.md new file mode 100644 index 0000000000..06962ff39b --- /dev/null +++ b/devlog/_plan/260911_l4_service_cli/020_wp2_stop_refusal.md @@ -0,0 +1,87 @@ +# wp2 — #4169: every stop refusal is reported as a CODEX_HOME ownership mismatch + +Work-phase 2 of the L4 lane, stacked on wp1. Carried source: PR #4170 by `yeongjunyoo`, +head `4d72ef0103`, three commits on base `c15a98caa9`. All four of its files are inside this +lane's ownership, so nothing was dropped. + +## What the carry does + +`POST /api/stop` refuses for three distinct reasons. `stopProxy` preferred the server's own +message but fell back, when none was readable, to one hardcoded sentence naming a fourth cause +the server never reports. #4170 captures the refusal `code`, selects the fallback wording from +it, and returns the refusal per attempt instead of publishing it to module state — the last of +those fixes a real interleaving bug where two overlapping stops could swap causes. + +## What the carry left open + +#4169's **Expected** section asks for two things: + +1. a `respawnable_service` or `service_state_unknown` refusal must not be described as an + ownership mismatch, and +2. *"the recommended next command should not be the command the operator just ran."* + +The carry does (1). Subagent Heisenberg traced (2) and returned `LOOP_REAL`: + +- `ocx stop` reaches `POST /api/stop` through `dispatch.stop` → `handleStop` → + `stopWithDeferral` → `stopProxy` → `stopProxyGracefully`. Stopping the service manager + first does not skip it. +- The server answers with *"the stop must be run by `ocx stop`"* because that refusal is + written for an API client, and `management-api.ts` emits `respawnable_service` precisely + when the CLI's teardown receipt was **not** honoured, so it cannot tell the two apart. +- `handleStop` prints `err.message` verbatim, so the operator is told to run the command they + are already running. +- The carried fallback for `respawnable_service` also ends in "Run `ocx stop`", making the + empty-body path a tighter loop than the one being fixed. + +There is no header, flag, query parameter or route that marks a CLI-originated stop, so the +server cannot word the refusal differently. The correction belongs to the CLI, which is the one +caller that knows which it is. + +## What this work-phase adds + +- The refusal `code` travels on `ProxyOwnershipRefusedError`. The reporting caller acts on the + cause; re-parsing the prose is not an option, because the prose is the server's. +- `refusalFallbackMessage` names the cause only. +- A new exported `refusalNextStep(code)` owns the command, and `handleStop` prints it under the + refusal at both call sites. The only callers of `stopProxy` are `ocx stop` and the service + manager's own cleanup, and both have already asked the service manager to stop by then, so no + branch answers with the command that just failed. + +`ProxyOwnershipRefusedError` keeps its name. The issue suggests renaming it and the name does +overstate what it carries, but the carrying author deliberately deferred that as a separate +wider change and it is not part of the issue's Expected behaviour. Recorded, not decided +unilaterally. + +## Audit + +Subagent Confucius reviewed the staged diff adversarially and returned `BLOCKERS_FOUND` with one +item: the new next-step test banned the literal `` `ocx stop` ``, which the production wording +contains **in order to rule it out**, so the test failed against its own implementation. Folded: +the assertion now bans a recommendation (`/Run \`ocx stop\`/`) rather than a mention. Its second +observation — that no test pinned the CLI wiring — is folded as a source oracle over the two +`refusalNextStep(err.code)` print sites. + +Re-checked deterministically afterwards: the four source-oracle counts in +`grok-lifecycle.test.ts` are all exactly 2, and no `refusalNextStep` branch matches +`/Run \`ocx stop\`/`. + +## CI repair carried into this phase + +Hosted CI on wp1's head failed on Linux and macOS with one test: +`codex-cli-update-launcher-policy` asserts `bin/ocx.mjs` contains +`"!codexCliUpdateInspection && isNodeModulesInstall()"` as an adjacent string. The carry gates +the #1849 boot probe on the npm layout, inserting `installMethod === "npm"` between those two +clauses. The invariant the oracle protects is intact and still evaluated first; only the +adjacency changed. The oracle now locates the guard wrapping the `bootRestoreProbe` call and +asserts both clauses are in it. + +`tests/codex-integration/codex-cli-update-launcher-policy.test.ts` is not in the packet's +keep-set. It is an oracle over `bin/ocx.mjs`, which this lane owns, and the round already +granted L2 the same thing for the same reason: a lane that changes a file owns the oracle +asserting it, or the change cannot land at all. Reported rather than assumed. + +## Not run + +`bun test`, `bun run test`, `bun run test:changed`, `bun run typecheck`, `bun run build:gui` +and `bun install` are NOT RUN by operator instruction. Hosted CI on the exact pushed head is +the only product evidence this round accepts.