From 2e5f4f45b516bac26f82700165e5b8d0c8adfcee Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Sun, 9 Aug 2026 23:33:33 -0700 Subject: [PATCH] =?UTF-8?q?fix(collaboration):=20stop=20reporting=20unknow?= =?UTF-8?q?n=20catalog=20state=20as=20a=20catalog=20change=20v2=20guidance?= =?UTF-8?q?=20mapped=20`unknown`=20to=20the=20same=20text=20as=20`stale`:?= =?UTF-8?q?=20=20=20=20=20The=20model=20catalog=20changed=20after=20Codex?= =?UTF-8?q?=20started;=20do=20not=20set=20model=20or=20=20=20=20=20reasoni?= =?UTF-8?q?ng=5Feffort=20overrides=20until=20Codex=20restarts.=20`unknown`?= =?UTF-8?q?=20does=20not=20mean=20the=20catalog=20changed.=20It=20means=20?= =?UTF-8?q?the=20comparison=20could=20not=20be=20made=20at=20all=20?= =?UTF-8?q?=E2=80=94=20`collectCodexAppServerCatalogState`=20returns=20`un?= =?UTF-8?q?known`=20when=20the=20catalog=20mtime=20is=20unreadable,=20when?= =?UTF-8?q?=20process=20enumeration=20fails,=20or=20when=20any=20app-serve?= =?UTF-8?q?r's=20start=20time=20is=20unreadable.=20A=20transient=20code-mo?= =?UTF-8?q?de-host=20process=20that=20exits=20between=20enumeration=20and?= =?UTF-8?q?=20the=20start-time=20read=20lands=20here,=20which=20is=20why?= =?UTF-8?q?=20the=20message=20appears=20intermittently=20and=20alternates?= =?UTF-8?q?=20with=20normal=20guidance=20turn=20to=20turn.=20So=20the=20te?= =?UTF-8?q?xt=20asserts=20a=20cause=20that=20was=20never=20established,=20?= =?UTF-8?q?and=20prescribes=20a=20remedy=20that=20cannot=20work:=20restart?= =?UTF-8?q?ing=20Codex=20does=20not=20clear=20`unknown`,=20because=20the?= =?UTF-8?q?=20state=20is=20a=20failed=20measurement=20rather=20than=20a=20?= =?UTF-8?q?stale=20process.=20Suppressing=20overrides=20while=20the=20stat?= =?UTF-8?q?e=20is=20unknown=20is=20still=20correct=20and=20is=20unchanged.?= =?UTF-8?q?=20Only=20the=20claim=20changes:=20`unknown`=20now=20gets=20its?= =?UTF-8?q?=20own=20message=20mirroring=20the=20honest=20phrasing=20`ocx?= =?UTF-8?q?=20doctor`=20already=20uses=20for=20the=20same=20state=20(src/c?= =?UTF-8?q?li/doctor.ts),=20so=20an=20operator=20can=20tell=20"the=20catal?= =?UTF-8?q?og=20changed"=20from=20"we=20could=20not=20check".=20Verified:?= =?UTF-8?q?=20=20=20bun=20test=20tests/multi-agent-compat.test.ts=20=20=20?= =?UTF-8?q?->=2042=20pass,=200=20fail=20=20=20bun=20x=20tsc=20--noEmit=20?= =?UTF-8?q?=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20?= =?UTF-8?q?=20=20=20=20->=20clean=20=20=20coverage=20src/server/responses/?= =?UTF-8?q?collaboration.ts=20->=2084.00%=20lines=20Fixes=20#1354?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/server/responses/collaboration.ts | 14 ++++++++- tests/multi-agent-compat.test.ts | 41 +++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/server/responses/collaboration.ts b/src/server/responses/collaboration.ts index 0b28267ce8..97620bfcbe 100644 --- a/src/server/responses/collaboration.ts +++ b/src/server/responses/collaboration.ts @@ -249,10 +249,22 @@ export async function multiAgentGuidanceText( // Codex cannot actually spawn makes spawn_agent reject the override, so // suppress positive model claims while the state is stale or unknown. const catalogState = await (deps.collectCatalogState ?? defaultCollectCatalogState)(); - if (catalogState.state === "stale" || catalogState.state === "unknown") { + if (catalogState.state === "stale") { return "The model catalog changed after Codex started; do not set " + "model or reasoning_effort overrides until Codex restarts."; } + // #1354: `unknown` is not `stale`. It means the comparison could not be made + // — the app-server start time or the catalog mtime was unreadable (a + // transient code-mode-host process exiting between enumeration and the + // start-time read is one way to land here). Suppressing overrides is still + // correct, but the stale text asserts a cause that was never established and + // prescribes a restart that cannot clear `unknown`. Mirror the honest + // phrasing `ocx doctor` already uses for this state. + if (catalogState.state === "unknown") { + return "Could not verify whether Codex's model catalog is current " + + "(app-server start time or catalog timestamp unreadable); do not set model or " + + "reasoning_effort overrides for this turn."; + } // codex-rs supplies the Proactive text on v2; the proxy only adds model-designation // guidance, and only when there is something concrete to designate: a configured // injectionModel and/or a roster entry that resolves in the injected catalog. diff --git a/tests/multi-agent-compat.test.ts b/tests/multi-agent-compat.test.ts index 8e9740cb1f..bfdfd7465a 100644 --- a/tests/multi-agent-compat.test.ts +++ b/tests/multi-agent-compat.test.ts @@ -143,6 +143,47 @@ describe("multiAgentGuidanceText", () => { } }); + test("v2 guidance does not assert a catalog change or a restart remedy when the state is unknown (#1354)", async () => { + const dir = codexHomeFixture(V2_ON); + catalogFixture(dir, [{ + slug: "anthropic/claude-sonnet-5", + efforts: ["low", "medium", "high", "xhigh"], + }]); + const parsed = parsedFixture({ reasoning: "medium", tools: [{ name: "spawn_agent" }] }); + const options = { injectionModel: "anthropic/claude-sonnet-5" }; + + const stale = await multiAgentGuidanceText(parsed, options, { + collectCatalogState: () => ({ state: "stale" }), + }); + const unknown = await multiAgentGuidanceText(parsed, options, { + collectCatalogState: () => ({ state: "unknown" }), + }); + + // "stale" is an established fact: the catalog really did change after the + // app-server started, and restarting Codex really does clear it. + expect(stale).toContain("The model catalog changed after Codex started"); + expect(stale).toContain("until Codex restarts"); + + // "unknown" means the comparison could not be made at all — the process + // start time or the catalog mtime was unreadable. Neither the cause nor the + // remedy is established, and a restart cannot clear it, so the guidance must + // not assert either. + expect(unknown).not.toContain("The model catalog changed after Codex started"); + expect(unknown).not.toContain("until Codex restarts"); + expect(unknown).toContain("Could not verify"); + + // The suppression itself is correct in both states and must not regress: + // withhold the override instruction and any positive model claims. + for (const text of [stale, unknown]) { + expect(text).toContain("do not set"); + expect(text).not.toContain("Preferred sub-agent"); + expect(text).not.toContain("Available models"); + } + + // The two states must stay distinguishable to an operator reading the turn. + expect(unknown).not.toBe(stale); + }); + test("v2 built-in guidance is schema-agnostic and keeps fork rules", async () => { const dir = codexHomeFixture(V2_ON); catalogFixture(dir, [{