diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index ed6bdc0fd6..32e57be954 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -379,6 +379,8 @@ zeroize = "1.8.2" rust = {} [workspace.lints.clippy] +await_holding_invalid_type = "deny" +await_holding_lock = "deny" expect_used = "deny" identity_op = "deny" manual_clamp = "deny" diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index ac9f480a8c..d195d57b6c 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1437,19 +1437,27 @@ }, "PluginInstallParams": { "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local install flow.", - "type": "boolean" - }, "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "type": "object" @@ -1465,10 +1473,6 @@ "array", "null" ] - }, - "forceRemoteSync": { - "description": "When true, reconcile the official curated marketplace against the remote plugin state before listing marketplaces.", - "type": "boolean" } }, "type": "object" @@ -1476,24 +1480,32 @@ "PluginReadParams": { "properties": { "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "type": "object" }, "PluginUninstallParams": { "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local uninstall flow.", - "type": "boolean" - }, "pluginId": { "type": "string" } diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index c9d38c01b9..1ab8b7adfb 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -10428,19 +10428,27 @@ "PluginInstallParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local install flow.", - "type": "boolean" - }, "marketplacePath": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/v2/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginInstallParams", @@ -10502,6 +10510,14 @@ { "type": "null" } + ], + "description": "Local composer icon path, resolved from the installed plugin package." + }, + "composerIconUrl": { + "description": "Remote composer icon URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "defaultPrompt": { @@ -10534,6 +10550,14 @@ { "type": "null" } + ], + "description": "Local logo path, resolved from the installed plugin package." + }, + "logoUrl": { + "description": "Remote logo URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "longDescription": { @@ -10548,7 +10572,15 @@ "null" ] }, + "screenshotUrls": { + "description": "Remote screenshot URLs from the plugin catalog.", + "items": { + "type": "string" + }, + "type": "array" + }, "screenshots": { + "description": "Local screenshot paths, resolved from the installed plugin package.", "items": { "$ref": "#/definitions/v2/AbsolutePathBuf" }, @@ -10575,6 +10607,7 @@ }, "required": [ "capabilities", + "screenshotUrls", "screenshots" ], "type": "object" @@ -10591,10 +10624,6 @@ "array", "null" ] - }, - "forceRemoteSync": { - "description": "When true, reconcile the official curated marketplace against the remote plugin state before listing marketplaces.", - "type": "boolean" } }, "title": "PluginListParams", @@ -10622,12 +10651,6 @@ "$ref": "#/definitions/v2/PluginMarketplaceEntry" }, "type": "array" - }, - "remoteSyncError": { - "type": [ - "string", - "null" - ] } }, "required": [ @@ -10652,7 +10675,15 @@ "type": "string" }, "path": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/v2/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Local marketplace file path when the marketplace is backed by a local file. Remote-only catalog marketplaces do not have a local path." }, "plugins": { "items": { @@ -10663,7 +10694,6 @@ }, "required": [ "name", - "path", "plugins" ], "type": "object" @@ -10672,14 +10702,26 @@ "$schema": "http://json-schema.org/draft-07/schema#", "properties": { "marketplacePath": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/v2/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginReadParams", @@ -10757,6 +10799,23 @@ ], "title": "GitPluginSource", "type": "object" + }, + { + "description": "The plugin is available in the remote catalog. Download metadata is kept server-side and is not exposed through the app-server API.", + "properties": { + "type": { + "enum": [ + "remote" + ], + "title": "RemotePluginSourceType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "RemotePluginSource", + "type": "object" } ] }, @@ -10808,10 +10867,6 @@ "PluginUninstallParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local uninstall flow.", - "type": "boolean" - }, "pluginId": { "type": "string" } diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 458367e205..da0381d8b1 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -6960,19 +6960,27 @@ "PluginInstallParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local install flow.", - "type": "boolean" - }, "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginInstallParams", @@ -7034,6 +7042,14 @@ { "type": "null" } + ], + "description": "Local composer icon path, resolved from the installed plugin package." + }, + "composerIconUrl": { + "description": "Remote composer icon URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "defaultPrompt": { @@ -7066,6 +7082,14 @@ { "type": "null" } + ], + "description": "Local logo path, resolved from the installed plugin package." + }, + "logoUrl": { + "description": "Remote logo URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "longDescription": { @@ -7080,7 +7104,15 @@ "null" ] }, + "screenshotUrls": { + "description": "Remote screenshot URLs from the plugin catalog.", + "items": { + "type": "string" + }, + "type": "array" + }, "screenshots": { + "description": "Local screenshot paths, resolved from the installed plugin package.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -7107,6 +7139,7 @@ }, "required": [ "capabilities", + "screenshotUrls", "screenshots" ], "type": "object" @@ -7123,10 +7156,6 @@ "array", "null" ] - }, - "forceRemoteSync": { - "description": "When true, reconcile the official curated marketplace against the remote plugin state before listing marketplaces.", - "type": "boolean" } }, "title": "PluginListParams", @@ -7154,12 +7183,6 @@ "$ref": "#/definitions/PluginMarketplaceEntry" }, "type": "array" - }, - "remoteSyncError": { - "type": [ - "string", - "null" - ] } }, "required": [ @@ -7184,7 +7207,15 @@ "type": "string" }, "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Local marketplace file path when the marketplace is backed by a local file. Remote-only catalog marketplaces do not have a local path." }, "plugins": { "items": { @@ -7195,7 +7226,6 @@ }, "required": [ "name", - "path", "plugins" ], "type": "object" @@ -7204,14 +7234,26 @@ "$schema": "http://json-schema.org/draft-07/schema#", "properties": { "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginReadParams", @@ -7289,6 +7331,23 @@ ], "title": "GitPluginSource", "type": "object" + }, + { + "description": "The plugin is available in the remote catalog. Download metadata is kept server-side and is not exposed through the app-server API.", + "properties": { + "type": { + "enum": [ + "remote" + ], + "title": "RemotePluginSourceType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "RemotePluginSource", + "type": "object" } ] }, @@ -7340,10 +7399,6 @@ "PluginUninstallParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local uninstall flow.", - "type": "boolean" - }, "pluginId": { "type": "string" } diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginInstallParams.json b/codex-rs/app-server-protocol/schema/json/v2/PluginInstallParams.json index 6890705311..ad3c0c1079 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginInstallParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginInstallParams.json @@ -7,19 +7,27 @@ } }, "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local install flow.", - "type": "boolean" - }, "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginInstallParams", diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginListParams.json b/codex-rs/app-server-protocol/schema/json/v2/PluginListParams.json index 669ff92b9e..27ea8c4df3 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginListParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginListParams.json @@ -16,10 +16,6 @@ "array", "null" ] - }, - "forceRemoteSync": { - "description": "When true, reconcile the official curated marketplace against the remote plugin state before listing marketplaces.", - "type": "boolean" } }, "title": "PluginListParams", diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json index ee039060e6..72c941c45f 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json @@ -74,6 +74,14 @@ { "type": "null" } + ], + "description": "Local composer icon path, resolved from the installed plugin package." + }, + "composerIconUrl": { + "description": "Remote composer icon URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "defaultPrompt": { @@ -106,6 +114,14 @@ { "type": "null" } + ], + "description": "Local logo path, resolved from the installed plugin package." + }, + "logoUrl": { + "description": "Remote logo URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "longDescription": { @@ -120,7 +136,15 @@ "null" ] }, + "screenshotUrls": { + "description": "Remote screenshot URLs from the plugin catalog.", + "items": { + "type": "string" + }, + "type": "array" + }, "screenshots": { + "description": "Local screenshot paths, resolved from the installed plugin package.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -147,6 +171,7 @@ }, "required": [ "capabilities", + "screenshotUrls", "screenshots" ], "type": "object" @@ -167,7 +192,15 @@ "type": "string" }, "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Local marketplace file path when the marketplace is backed by a local file. Remote-only catalog marketplaces do not have a local path." }, "plugins": { "items": { @@ -178,7 +211,6 @@ }, "required": [ "name", - "path", "plugins" ], "type": "object" @@ -242,6 +274,23 @@ ], "title": "GitPluginSource", "type": "object" + }, + { + "description": "The plugin is available in the remote catalog. Download metadata is kept server-side and is not exposed through the app-server API.", + "properties": { + "type": { + "enum": [ + "remote" + ], + "title": "RemotePluginSourceType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "RemotePluginSource", + "type": "object" } ] }, @@ -311,12 +360,6 @@ "$ref": "#/definitions/PluginMarketplaceEntry" }, "type": "array" - }, - "remoteSyncError": { - "type": [ - "string", - "null" - ] } }, "required": [ diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginReadParams.json b/codex-rs/app-server-protocol/schema/json/v2/PluginReadParams.json index a720ae3b59..5cc3e5cab5 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginReadParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginReadParams.json @@ -8,14 +8,26 @@ }, "properties": { "marketplacePath": { - "$ref": "#/definitions/AbsolutePathBuf" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ] }, "pluginName": { "type": "string" + }, + "remoteMarketplaceName": { + "type": [ + "string", + "null" + ] } }, "required": [ - "marketplacePath", "pluginName" ], "title": "PluginReadParams", diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json index 43628e28f0..5ec07f00f1 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json @@ -126,6 +126,14 @@ { "type": "null" } + ], + "description": "Local composer icon path, resolved from the installed plugin package." + }, + "composerIconUrl": { + "description": "Remote composer icon URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "defaultPrompt": { @@ -158,6 +166,14 @@ { "type": "null" } + ], + "description": "Local logo path, resolved from the installed plugin package." + }, + "logoUrl": { + "description": "Remote logo URL from the plugin catalog.", + "type": [ + "string", + "null" ] }, "longDescription": { @@ -172,7 +188,15 @@ "null" ] }, + "screenshotUrls": { + "description": "Remote screenshot URLs from the plugin catalog.", + "items": { + "type": "string" + }, + "type": "array" + }, "screenshots": { + "description": "Local screenshot paths, resolved from the installed plugin package.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -199,6 +223,7 @@ }, "required": [ "capabilities", + "screenshotUrls", "screenshots" ], "type": "object" @@ -262,6 +287,23 @@ ], "title": "GitPluginSource", "type": "object" + }, + { + "description": "The plugin is available in the remote catalog. Download metadata is kept server-side and is not exposed through the app-server API.", + "properties": { + "type": { + "enum": [ + "remote" + ], + "title": "RemotePluginSourceType", + "type": "string" + } + }, + "required": [ + "type" + ], + "title": "RemotePluginSource", + "type": "object" } ] }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginUninstallParams.json b/codex-rs/app-server-protocol/schema/json/v2/PluginUninstallParams.json index a6d7ec78bb..5b7e0a592f 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginUninstallParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginUninstallParams.json @@ -1,10 +1,6 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { - "forceRemoteSync": { - "description": "When true, apply the remote plugin change before the local uninstall flow.", - "type": "boolean" - }, "pluginId": { "type": "string" } diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginInstallParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginInstallParams.ts index 9ac1c50c1e..257dc47a1e 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginInstallParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginInstallParams.ts @@ -3,8 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AbsolutePathBuf } from "../AbsolutePathBuf"; -export type PluginInstallParams = { marketplacePath: AbsolutePathBuf, pluginName: string, -/** - * When true, apply the remote plugin change before the local install flow. - */ -forceRemoteSync?: boolean, }; +export type PluginInstallParams = { marketplacePath?: AbsolutePathBuf | null, remoteMarketplaceName?: string | null, pluginName: string, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginInterface.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginInterface.ts index 7e0a48aaec..4e97ee66f3 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginInterface.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginInterface.ts @@ -8,4 +8,28 @@ export type PluginInterface = { displayName: string | null, shortDescription: st * Starter prompts for the plugin. Capped at 3 entries with a maximum of * 128 characters per entry. */ -defaultPrompt: Array | null, brandColor: string | null, composerIcon: AbsolutePathBuf | null, logo: AbsolutePathBuf | null, screenshots: Array, }; +defaultPrompt: Array | null, brandColor: string | null, +/** + * Local composer icon path, resolved from the installed plugin package. + */ +composerIcon: AbsolutePathBuf | null, +/** + * Remote composer icon URL from the plugin catalog. + */ +composerIconUrl: string | null, +/** + * Local logo path, resolved from the installed plugin package. + */ +logo: AbsolutePathBuf | null, +/** + * Remote logo URL from the plugin catalog. + */ +logoUrl: string | null, +/** + * Local screenshot paths, resolved from the installed plugin package. + */ +screenshots: Array, +/** + * Remote screenshot URLs from the plugin catalog. + */ +screenshotUrls: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginListParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginListParams.ts index cd2a0cde1e..dcf23796db 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginListParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginListParams.ts @@ -8,9 +8,4 @@ export type PluginListParams = { * Optional working directories used to discover repo marketplaces. When omitted, * only home-scoped marketplaces and the official curated marketplace are considered. */ -cwds?: Array | null, -/** - * When true, reconcile the official curated marketplace against the remote plugin state - * before listing marketplaces. - */ -forceRemoteSync?: boolean, }; +cwds?: Array | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginListResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginListResponse.ts index 7ae5f8e505..d50200c905 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginListResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginListResponse.ts @@ -4,4 +4,4 @@ import type { MarketplaceLoadErrorInfo } from "./MarketplaceLoadErrorInfo"; import type { PluginMarketplaceEntry } from "./PluginMarketplaceEntry"; -export type PluginListResponse = { marketplaces: Array, marketplaceLoadErrors: Array, remoteSyncError: string | null, featuredPluginIds: Array, }; +export type PluginListResponse = { marketplaces: Array, marketplaceLoadErrors: Array, featuredPluginIds: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginMarketplaceEntry.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginMarketplaceEntry.ts index c0ab75b8f9..f9dcee27df 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginMarketplaceEntry.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginMarketplaceEntry.ts @@ -5,4 +5,9 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; import type { MarketplaceInterface } from "./MarketplaceInterface"; import type { PluginSummary } from "./PluginSummary"; -export type PluginMarketplaceEntry = { name: string, path: AbsolutePathBuf, interface: MarketplaceInterface | null, plugins: Array, }; +export type PluginMarketplaceEntry = { name: string, +/** + * Local marketplace file path when the marketplace is backed by a local file. + * Remote-only catalog marketplaces do not have a local path. + */ +path: AbsolutePathBuf | null, interface: MarketplaceInterface | null, plugins: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginReadParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginReadParams.ts index cd6696873d..8c4394f0da 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginReadParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginReadParams.ts @@ -3,4 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AbsolutePathBuf } from "../AbsolutePathBuf"; -export type PluginReadParams = { marketplacePath: AbsolutePathBuf, pluginName: string, }; +export type PluginReadParams = { marketplacePath?: AbsolutePathBuf | null, remoteMarketplaceName?: string | null, pluginName: string, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginSource.ts index 5c8771aa0b..f6e867195d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginSource.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginSource.ts @@ -3,4 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AbsolutePathBuf } from "../AbsolutePathBuf"; -export type PluginSource = { "type": "local", path: AbsolutePathBuf, } | { "type": "git", url: string, path: string | null, refName: string | null, sha: string | null, }; +export type PluginSource = { "type": "local", path: AbsolutePathBuf, } | { "type": "git", url: string, path: string | null, refName: string | null, sha: string | null, } | { "type": "remote" }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginUninstallParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginUninstallParams.ts index aa1d1bfef9..e7f52c0eb3 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginUninstallParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginUninstallParams.ts @@ -2,8 +2,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type PluginUninstallParams = { pluginId: string, -/** - * When true, apply the remote plugin change before the local uninstall flow. - */ -forceRemoteSync?: boolean, }; +export type PluginUninstallParams = { pluginId: string, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index aaa3d9257a..463cb4c57f 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -3602,10 +3602,6 @@ pub struct PluginListParams { /// only home-scoped marketplaces and the official curated marketplace are considered. #[ts(optional = nullable)] pub cwds: Option>, - /// When true, reconcile the official curated marketplace against the remote plugin state - /// before listing marketplaces. - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub force_remote_sync: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3615,7 +3611,6 @@ pub struct PluginListResponse { pub marketplaces: Vec, #[serde(default)] pub marketplace_load_errors: Vec, - pub remote_sync_error: Option, #[serde(default)] pub featured_plugin_ids: Vec, } @@ -3632,7 +3627,10 @@ pub struct MarketplaceLoadErrorInfo { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct PluginReadParams { - pub marketplace_path: AbsolutePathBuf, + #[ts(optional = nullable)] + pub marketplace_path: Option, + #[ts(optional = nullable)] + pub remote_marketplace_name: Option, pub plugin_name: String, } @@ -3744,7 +3742,9 @@ pub struct SkillsListEntry { #[ts(export_to = "v2/")] pub struct PluginMarketplaceEntry { pub name: String, - pub path: AbsolutePathBuf, + /// Local marketplace file path when the marketplace is backed by a local file. + /// Remote-only catalog marketplaces do not have a local path. + pub path: Option, pub interface: Option, pub plugins: Vec, } @@ -3837,9 +3837,18 @@ pub struct PluginInterface { /// 128 characters per entry. pub default_prompt: Option>, pub brand_color: Option, + /// Local composer icon path, resolved from the installed plugin package. pub composer_icon: Option, + /// Remote composer icon URL from the plugin catalog. + pub composer_icon_url: Option, + /// Local logo path, resolved from the installed plugin package. pub logo: Option, + /// Remote logo URL from the plugin catalog. + pub logo_url: Option, + /// Local screenshot paths, resolved from the installed plugin package. pub screenshots: Vec, + /// Remote screenshot URLs from the plugin catalog. + pub screenshot_urls: Vec, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3858,6 +3867,9 @@ pub enum PluginSource { ref_name: Option, sha: Option, }, + /// The plugin is available in the remote catalog. Download metadata is + /// kept server-side and is not exposed through the app-server API. + Remote, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3884,11 +3896,11 @@ pub struct SkillsConfigWriteResponse { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct PluginInstallParams { - pub marketplace_path: AbsolutePathBuf, + #[ts(optional = nullable)] + pub marketplace_path: Option, + #[ts(optional = nullable)] + pub remote_marketplace_name: Option, pub plugin_name: String, - /// When true, apply the remote plugin change before the local install flow. - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub force_remote_sync: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3904,9 +3916,6 @@ pub struct PluginInstallResponse { #[ts(export_to = "v2/")] pub struct PluginUninstallParams { pub plugin_id: String, - /// When true, apply the remote plugin change before the local uninstall flow. - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub force_remote_sync: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -8748,27 +8757,44 @@ mod tests { } #[test] - fn plugin_list_params_serialization_uses_force_remote_sync() { + fn plugin_source_serializes_local_git_and_remote_variants() { + let local_path = if cfg!(windows) { + r"C:\plugins\linear" + } else { + "/plugins/linear" + }; + let local_path = AbsolutePathBuf::try_from(PathBuf::from(local_path)).unwrap(); + let local_path_json = local_path.as_path().display().to_string(); + assert_eq!( - serde_json::to_value(PluginListParams { - cwds: None, - force_remote_sync: false, - }) - .unwrap(), + serde_json::to_value(PluginSource::Local { path: local_path }).unwrap(), json!({ - "cwds": null, + "type": "local", + "path": local_path_json, }), ); assert_eq!( - serde_json::to_value(PluginListParams { - cwds: None, - force_remote_sync: true, + serde_json::to_value(PluginSource::Git { + url: "https://github.com/openai/example.git".to_string(), + path: Some("plugins/example".to_string()), + ref_name: Some("main".to_string()), + sha: Some("abc123".to_string()), }) .unwrap(), json!({ - "cwds": null, - "forceRemoteSync": true, + "type": "git", + "url": "https://github.com/openai/example.git", + "path": "plugins/example", + "refName": "main", + "sha": "abc123", + }), + ); + + assert_eq!( + serde_json::to_value(PluginSource::Remote).unwrap(), + json!({ + "type": "remote", }), ); } @@ -8805,7 +8831,143 @@ mod tests { } #[test] - fn plugin_install_params_serialization_uses_force_remote_sync() { + fn plugin_marketplace_entry_serializes_remote_only_path_as_null() { + assert_eq!( + serde_json::to_value(PluginMarketplaceEntry { + name: "openai-curated".to_string(), + path: None, + interface: None, + plugins: Vec::new(), + }) + .unwrap(), + json!({ + "name": "openai-curated", + "path": null, + "interface": null, + "plugins": [], + }), + ); + } + + #[test] + fn plugin_interface_serializes_local_paths_and_remote_urls_separately() { + let composer_icon = if cfg!(windows) { + r"C:\plugins\linear\icon.png" + } else { + "/plugins/linear/icon.png" + }; + let composer_icon = AbsolutePathBuf::try_from(PathBuf::from(composer_icon)).unwrap(); + let composer_icon_json = composer_icon.as_path().display().to_string(); + + let interface = PluginInterface { + display_name: Some("Linear".to_string()), + short_description: None, + long_description: None, + developer_name: None, + category: Some("Productivity".to_string()), + capabilities: Vec::new(), + website_url: None, + privacy_policy_url: None, + terms_of_service_url: None, + default_prompt: None, + brand_color: None, + composer_icon: Some(composer_icon), + composer_icon_url: Some("https://example.com/linear/icon.png".to_string()), + logo: None, + logo_url: Some("https://example.com/linear/logo.png".to_string()), + screenshots: Vec::new(), + screenshot_urls: vec!["https://example.com/linear/screenshot.png".to_string()], + }; + + assert_eq!( + serde_json::to_value(interface).unwrap(), + json!({ + "displayName": "Linear", + "shortDescription": null, + "longDescription": null, + "developerName": null, + "category": "Productivity", + "capabilities": [], + "websiteUrl": null, + "privacyPolicyUrl": null, + "termsOfServiceUrl": null, + "defaultPrompt": null, + "brandColor": null, + "composerIcon": composer_icon_json, + "composerIconUrl": "https://example.com/linear/icon.png", + "logo": null, + "logoUrl": "https://example.com/linear/logo.png", + "screenshots": [], + "screenshotUrls": ["https://example.com/linear/screenshot.png"], + }), + ); + } + + #[test] + fn plugin_list_params_ignore_removed_force_remote_sync_field() { + assert_eq!( + serde_json::from_value::(json!({ + "cwds": null, + "forceRemoteSync": true, + })) + .unwrap(), + PluginListParams { cwds: None }, + ); + } + + #[test] + fn plugin_read_params_serialization_uses_install_source_fields() { + let marketplace_path = if cfg!(windows) { + r"C:\plugins\marketplace.json" + } else { + "/plugins/marketplace.json" + }; + let marketplace_path = AbsolutePathBuf::try_from(PathBuf::from(marketplace_path)).unwrap(); + let marketplace_path_json = marketplace_path.as_path().display().to_string(); + assert_eq!( + serde_json::to_value(PluginReadParams { + marketplace_path: Some(marketplace_path.clone()), + remote_marketplace_name: None, + plugin_name: "gmail".to_string(), + }) + .unwrap(), + json!({ + "marketplacePath": marketplace_path_json, + "remoteMarketplaceName": null, + "pluginName": "gmail", + }), + ); + + assert_eq!( + serde_json::from_value::(json!({ + "marketplacePath": marketplace_path_json, + "pluginName": "gmail", + "forceRemoteSync": true, + })) + .unwrap(), + PluginReadParams { + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, + plugin_name: "gmail".to_string(), + }, + ); + + assert_eq!( + serde_json::from_value::(json!({ + "remoteMarketplaceName": "openai-curated", + "pluginName": "gmail", + })) + .unwrap(), + PluginReadParams { + marketplace_path: None, + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "gmail".to_string(), + }, + ); + } + + #[test] + fn plugin_install_params_serialization_omits_force_remote_sync() { let marketplace_path = if cfg!(windows) { r"C:\plugins\marketplace.json" } else { @@ -8815,38 +8977,52 @@ mod tests { let marketplace_path_json = marketplace_path.as_path().display().to_string(); assert_eq!( serde_json::to_value(PluginInstallParams { - marketplace_path: marketplace_path.clone(), + marketplace_path: Some(marketplace_path.clone()), + remote_marketplace_name: None, plugin_name: "gmail".to_string(), - force_remote_sync: false, }) .unwrap(), json!({ "marketplacePath": marketplace_path_json, + "remoteMarketplaceName": null, "pluginName": "gmail", }), ); assert_eq!( - serde_json::to_value(PluginInstallParams { - marketplace_path, - plugin_name: "gmail".to_string(), - force_remote_sync: true, - }) - .unwrap(), - json!({ + serde_json::from_value::(json!({ "marketplacePath": marketplace_path_json, "pluginName": "gmail", "forceRemoteSync": true, - }), + })) + .unwrap(), + PluginInstallParams { + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, + plugin_name: "gmail".to_string(), + }, + ); + + assert_eq!( + serde_json::from_value::(json!({ + "remoteMarketplaceName": "openai-curated", + "pluginName": "gmail", + "forceRemoteSync": true, + })) + .unwrap(), + PluginInstallParams { + marketplace_path: None, + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "gmail".to_string(), + }, ); } #[test] - fn plugin_uninstall_params_serialization_uses_force_remote_sync() { + fn plugin_uninstall_params_serialization_omits_force_remote_sync() { assert_eq!( serde_json::to_value(PluginUninstallParams { plugin_id: "gmail@openai-curated".to_string(), - force_remote_sync: false, }) .unwrap(), json!({ @@ -8855,15 +9031,14 @@ mod tests { ); assert_eq!( - serde_json::to_value(PluginUninstallParams { - plugin_id: "gmail@openai-curated".to_string(), - force_remote_sync: true, - }) - .unwrap(), - json!({ + serde_json::from_value::(json!({ "pluginId": "gmail@openai-curated", "forceRemoteSync": true, - }), + })) + .unwrap(), + PluginUninstallParams { + plugin_id: "gmail@openai-curated".to_string(), + }, ); } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 88156f5532..fcc19ce42d 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -185,7 +185,7 @@ Example with notification opt-out: - `collaborationMode/list` — list available collaboration mode presets (experimental, no pagination). This response omits built-in developer instructions; clients should either pass `settings.developer_instructions: null` when setting a mode to use Codex's built-in instructions, or provide their own instructions explicitly. - `skills/list` — list skills for one or more `cwd` values (optional `forceReload`). - `marketplace/add` — add a remote plugin marketplace from an HTTP(S) Git URL, SSH Git URL, or GitHub `owner/repo` shorthand, then persist it into the user marketplace config. Returns the installed root path plus whether the marketplace was already present. -- `plugin/list` — list discovered plugin marketplaces and plugin state, including effective marketplace install/auth policy metadata, fail-open `marketplaceLoadErrors` entries for marketplace files that could not be parsed or loaded, and best-effort `featuredPluginIds` for the official curated marketplace. `interface.category` uses the marketplace category when present; otherwise it falls back to the plugin manifest category. Pass `forceRemoteSync: true` to refresh curated plugin state before listing (**under development; do not call from production clients yet**). +- `plugin/list` — list discovered plugin marketplaces and plugin state, including effective marketplace install/auth policy metadata, fail-open `marketplaceLoadErrors` entries for marketplace files that could not be parsed or loaded, and best-effort `featuredPluginIds` for the official curated marketplace. `interface.category` uses the marketplace category when present; otherwise it falls back to the plugin manifest category (**under development; do not call from production clients yet**). - `plugin/read` — read one plugin by `marketplacePath` plus `pluginName`, returning marketplace info, a list-style `summary`, manifest descriptions/interface metadata, and bundled skills/apps/MCP server names. Returned plugin skills include their current `enabled` state after local config filtering. Plugin app summaries also include `needsAuth` when the server can determine connector accessibility (**under development; do not call from production clients yet**). - `skills/changed` — notification emitted when watched local skill files change. - `app/list` — list available apps. diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index e3f6322d40..7a82a24800 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -6428,55 +6428,19 @@ impl CodexMessageProcessor { async fn plugin_list(&self, request_id: ConnectionRequestId, params: PluginListParams) { let plugins_manager = self.thread_manager.plugins_manager(); - let PluginListParams { - cwds, - force_remote_sync, - } = params; + let PluginListParams { cwds } = params; let roots = cwds.unwrap_or_default(); plugins_manager.maybe_start_non_curated_plugin_cache_refresh(&roots); - let mut config = match self.load_latest_config(/*fallback_cwd*/ None).await { + let config = match self.load_latest_config(/*fallback_cwd*/ None).await { Ok(config) => config, Err(err) => { self.outgoing.send_error(request_id, err).await; return; } }; - let mut remote_sync_error = None; let auth = self.auth_manager.auth().await; - if force_remote_sync { - match plugins_manager - .sync_plugins_from_remote(&config, auth.as_ref(), /*additive_only*/ false) - .await - { - Ok(sync_result) => { - info!( - installed_plugin_ids = ?sync_result.installed_plugin_ids, - enabled_plugin_ids = ?sync_result.enabled_plugin_ids, - disabled_plugin_ids = ?sync_result.disabled_plugin_ids, - uninstalled_plugin_ids = ?sync_result.uninstalled_plugin_ids, - "completed plugin/list remote sync" - ); - } - Err(err) => { - warn!( - error = %err, - "plugin/list remote sync failed; returning local marketplace state" - ); - remote_sync_error = Some(err.to_string()); - } - } - - config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; - } - let config_for_marketplace_listing = config.clone(); let plugins_manager_for_marketplace_listing = plugins_manager.clone(); let (data, marketplace_load_errors) = match tokio::task::spawn_blocking(move || { @@ -6494,7 +6458,7 @@ impl CodexMessageProcessor { .into_iter() .map(|marketplace| PluginMarketplaceEntry { name: marketplace.name, - path: marketplace.path, + path: Some(marketplace.path), interface: marketplace.interface.map(|interface| MarketplaceInterface { display_name: interface.display_name, }), @@ -6509,7 +6473,7 @@ impl CodexMessageProcessor { source: marketplace_plugin_source_to_info(plugin.source), install_policy: plugin.policy.installation.into(), auth_policy: plugin.policy.authentication.into(), - interface: plugin.interface.map(plugin_interface_to_info), + interface: plugin.interface.map(local_plugin_interface_to_info), }) .collect(), }) @@ -6569,7 +6533,6 @@ impl CodexMessageProcessor { PluginListResponse { marketplaces: data, marketplace_load_errors, - remote_sync_error, featured_plugin_ids, }, ) @@ -6613,8 +6576,40 @@ impl CodexMessageProcessor { let plugins_manager = self.thread_manager.plugins_manager(); let PluginReadParams { marketplace_path, + remote_marketplace_name, plugin_name, } = params; + let marketplace_path = match (marketplace_path, remote_marketplace_name) { + (Some(marketplace_path), None) => marketplace_path, + (None, Some(remote_marketplace_name)) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!( + "remote plugin read is not supported yet for marketplace {remote_marketplace_name}" + ), + data: None, + }, + ) + .await; + return; + } + (Some(_), Some(_)) | (None, None) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: "plugin/read requires exactly one of marketplacePath or remoteMarketplaceName".to_string(), + data: None, + }, + ) + .await; + return; + } + }; let config_cwd = marketplace_path.as_path().parent().map(Path::to_path_buf); let config = match self.load_latest_config(config_cwd).await { @@ -6664,7 +6659,7 @@ impl CodexMessageProcessor { enabled: outcome.plugin.enabled, install_policy: outcome.plugin.policy.installation.into(), auth_policy: outcome.plugin.policy.authentication.into(), - interface: outcome.plugin.interface.map(plugin_interface_to_info), + interface: outcome.plugin.interface.map(local_plugin_interface_to_info), }, description: outcome.plugin.description, skills: plugin_skills_to_info(&visible_skills, &outcome.plugin.disabled_skill_paths), @@ -6738,9 +6733,40 @@ impl CodexMessageProcessor { async fn plugin_install(&self, request_id: ConnectionRequestId, params: PluginInstallParams) { let PluginInstallParams { marketplace_path, + remote_marketplace_name, plugin_name, - force_remote_sync, } = params; + let marketplace_path = match (marketplace_path, remote_marketplace_name) { + (Some(marketplace_path), None) => marketplace_path, + (None, Some(remote_marketplace_name)) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!( + "remote plugin install is not supported yet for marketplace {remote_marketplace_name}" + ), + data: None, + }, + ) + .await; + return; + } + (Some(_), Some(_)) | (None, None) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: "plugin/install requires exactly one of marketplacePath or remoteMarketplaceName".to_string(), + data: None, + }, + ) + .await; + return; + } + }; let config_cwd = marketplace_path.as_path().parent().map(Path::to_path_buf); let plugins_manager = self.thread_manager.plugins_manager(); @@ -6749,21 +6775,7 @@ impl CodexMessageProcessor { marketplace_path, }; - let install_result = if force_remote_sync { - let config = match self.load_latest_config(config_cwd.clone()).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; - let auth = self.auth_manager.auth().await; - plugins_manager - .install_plugin_with_remote_sync(&config, auth.as_ref(), request) - .await - } else { - plugins_manager.install_plugin(request).await - }; + let install_result = plugins_manager.install_plugin(request).await; match install_result { Ok(result) => { @@ -6915,27 +6927,10 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: PluginUninstallParams, ) { - let PluginUninstallParams { - plugin_id, - force_remote_sync, - } = params; + let PluginUninstallParams { plugin_id } = params; let plugins_manager = self.thread_manager.plugins_manager(); - let uninstall_result = if force_remote_sync { - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; - let auth = self.auth_manager.auth().await; - plugins_manager - .uninstall_plugin_with_remote_sync(&config, auth.as_ref(), plugin_id) - .await - } else { - plugins_manager.uninstall_plugin(plugin_id).await - }; + let uninstall_result = plugins_manager.uninstall_plugin(plugin_id).await; match uninstall_result { Ok(()) => { @@ -7866,6 +7861,10 @@ impl CodexMessageProcessor { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "listener subscription must be serialized against pending thread unloads" + )] async fn ensure_conversation_listener_task( listener_task_context: ListenerTaskContext, conversation_id: ThreadId, @@ -8673,6 +8672,10 @@ async fn handle_thread_listener_command( } #[allow(clippy::too_many_arguments)] +#[expect( + clippy::await_holding_invalid_type, + reason = "running-thread resume subscription must be serialized against pending unloads" +)] async fn handle_pending_thread_resume_request( conversation_id: ThreadId, conversation: &Arc, @@ -9105,7 +9108,7 @@ fn plugin_skills_to_info( .collect() } -fn plugin_interface_to_info(interface: PluginManifestInterface) -> PluginInterface { +fn local_plugin_interface_to_info(interface: PluginManifestInterface) -> PluginInterface { PluginInterface { display_name: interface.display_name, short_description: interface.short_description, @@ -9119,8 +9122,11 @@ fn plugin_interface_to_info(interface: PluginManifestInterface) -> PluginInterfa default_prompt: interface.default_prompt, brand_color: interface.brand_color, composer_icon: interface.composer_icon, + composer_icon_url: None, logo: interface.logo, + logo_url: None, screenshots: interface.screenshots, + screenshot_urls: Vec::new(), } } diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index c1bb995bab..ac6c48df06 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -504,6 +504,10 @@ where } #[tokio::test(flavor = "current_thread")] +#[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to global tracing state for its full duration" +)] async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> Result<()> { let _guard = tracing_test_guard().lock().await; let mut harness = TracingHarness::new().await?; @@ -583,6 +587,10 @@ async fn thread_start_jsonrpc_span_exports_server_span_and_parents_children() -> } #[tokio::test(flavor = "current_thread")] +#[expect( + clippy::await_holding_invalid_type, + reason = "test serializes access to global tracing state for its full duration" +)] async fn turn_start_jsonrpc_span_parents_core_turn_spans() -> Result<()> { let _guard = tracing_test_guard().lock().await; let mut harness = TracingHarness::new().await?; diff --git a/codex-rs/app-server/src/transport/remote_control/websocket.rs b/codex-rs/app-server/src/transport/remote_control/websocket.rs index 58df3035aa..523c93f3be 100644 --- a/codex-rs/app-server/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server/src/transport/remote_control/websocket.rs @@ -172,6 +172,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control client shutdown must serialize tracker state" + )] pub(crate) async fn run( mut self, app_server_client_name_rx: Option>, @@ -381,6 +385,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control server event receiver is shared across reconnects" + )] async fn run_server_writer_inner( state: Arc>, server_event_rx: Arc>>, @@ -522,6 +530,10 @@ impl RemoteControlWebsocket { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "remote-control client tracking must stay serialized while processing inbound events" + )] async fn run_websocket_reader_inner( client_tracker: Arc>, state: Arc>, diff --git a/codex-rs/app-server/tests/suite/v2/external_agent_config.rs b/codex-rs/app-server/tests/suite/v2/external_agent_config.rs index 21acf24906..049256b602 100644 --- a/codex-rs/app-server/tests/suite/v2/external_agent_config.rs +++ b/codex-rs/app-server/tests/suite/v2/external_agent_config.rs @@ -98,10 +98,7 @@ async fn external_agent_config_import_sends_completion_notification_for_local_pl assert_eq!(notification.method, "externalAgentConfig/import/completed"); let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: false, - }) + .send_plugin_list_request(PluginListParams { cwds: None }) .await?; let response: JSONRPCResponse = timeout( DEFAULT_TIMEOUT, diff --git a/codex-rs/app-server/tests/suite/v2/plugin_install.rs b/codex-rs/app-server/tests/suite/v2/plugin_install.rs index e51fac725f..3555dd745b 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_install.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_install.rs @@ -44,12 +44,6 @@ use tempfile::TempDir; use tokio::net::TcpListener; use tokio::task::JoinHandle; use tokio::time::timeout; -use wiremock::Mock; -use wiremock::MockServer; -use wiremock::ResponseTemplate; -use wiremock::matchers::header; -use wiremock::matchers::method; -use wiremock::matchers::path; // Plugin install tests wait on connector discovery after the install response path // starts, which is noticeably slower on Windows CI. @@ -82,6 +76,97 @@ async fn plugin_install_rejects_relative_marketplace_paths() -> Result<()> { Ok(()) } +#[tokio::test] +async fn plugin_install_rejects_missing_install_source() -> Result<()> { + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_install_request(PluginInstallParams { + marketplace_path: None, + remote_marketplace_name: None, + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("requires exactly one of marketplacePath or remoteMarketplaceName") + ); + Ok(()) +} + +#[tokio::test] +async fn plugin_install_rejects_multiple_install_sources() -> Result<()> { + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_install_request(PluginInstallParams { + marketplace_path: Some(AbsolutePathBuf::try_from( + codex_home.path().join("marketplace.json"), + )?), + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("requires exactly one of marketplacePath or remoteMarketplaceName") + ); + Ok(()) +} + +#[tokio::test] +async fn plugin_install_rejects_remote_marketplace_until_remote_install_is_supported() -> Result<()> +{ + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_install_request(PluginInstallParams { + marketplace_path: None, + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("remote plugin install is not supported yet") + ); + assert!(err.error.message.contains("openai-curated")); + Ok(()) +} + #[tokio::test] async fn plugin_install_returns_invalid_request_for_missing_marketplace_file() -> Result<()> { let codex_home = TempDir::new()?; @@ -90,11 +175,11 @@ async fn plugin_install_returns_invalid_request_for_missing_marketplace_file() - let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path: AbsolutePathBuf::try_from( + marketplace_path: Some(AbsolutePathBuf::try_from( codex_home.path().join("missing-marketplace.json"), - )?, + )?), + remote_marketplace_name: None, plugin_name: "missing-plugin".to_string(), - force_remote_sync: false, }) .await?; @@ -131,9 +216,9 @@ async fn plugin_install_returns_invalid_request_for_not_available_plugin() -> Re let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; @@ -181,9 +266,9 @@ async fn plugin_install_returns_invalid_request_for_disallowed_product_plugin() let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; @@ -198,76 +283,6 @@ async fn plugin_install_returns_invalid_request_for_disallowed_product_plugin() Ok(()) } -#[tokio::test] -async fn plugin_install_force_remote_sync_enables_remote_plugin_before_local_install() -> Result<()> -{ - let server = MockServer::start().await; - let codex_home = TempDir::new()?; - write_plugin_remote_sync_config(codex_home.path(), &format!("{}/backend-api/", server.uri()))?; - write_chatgpt_auth( - codex_home.path(), - ChatGptAuthFixture::new("chatgpt-token") - .account_id("account-123") - .chatgpt_user_id("user-123") - .chatgpt_account_id("account-123"), - AuthCredentialsStoreMode::File, - )?; - - let repo_root = TempDir::new()?; - write_plugin_marketplace( - repo_root.path(), - "debug", - "sample-plugin", - "./sample-plugin", - /*install_policy*/ None, - /*auth_policy*/ None, - )?; - write_plugin_source(repo_root.path(), "sample-plugin", &[])?; - let marketplace_path = - AbsolutePathBuf::try_from(repo_root.path().join(".agents/plugins/marketplace.json"))?; - - Mock::given(method("POST")) - .and(path("/backend-api/plugins/sample-plugin@debug/enable")) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with( - ResponseTemplate::new(200) - .set_body_string(r#"{"id":"sample-plugin@debug","enabled":true}"#), - ) - .expect(1) - .mount(&server) - .await; - - let mut mcp = McpProcess::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_plugin_install_request(PluginInstallParams { - marketplace_path, - plugin_name: "sample-plugin".to_string(), - force_remote_sync: true, - }) - .await?; - let response: JSONRPCResponse = timeout( - DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let response: PluginInstallResponse = to_response(response)?; - assert_eq!(response.apps_needing_auth, Vec::::new()); - - assert!( - codex_home - .path() - .join("plugins/cache/debug/sample-plugin/local/.codex-plugin/plugin.json") - .is_file() - ); - let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?; - assert!(config.contains(r#"[plugins."sample-plugin@debug"]"#)); - assert!(config.contains("enabled = true")); - Ok(()) -} - #[tokio::test] async fn plugin_install_tracks_analytics_event() -> Result<()> { let analytics_server = start_analytics_events_server().await?; @@ -300,9 +315,9 @@ async fn plugin_install_tracks_analytics_event() -> Result<()> { let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; let response: JSONRPCResponse = timeout( @@ -415,9 +430,9 @@ async fn plugin_install_returns_apps_needing_auth() -> Result<()> { let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; @@ -499,9 +514,9 @@ async fn plugin_install_filters_disallowed_apps_needing_auth() -> Result<()> { let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; @@ -566,9 +581,9 @@ async fn plugin_install_makes_bundled_mcp_servers_available_to_followup_requests let request_id = mcp .send_plugin_install_request(PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), - force_remote_sync: false, }) .await?; let response: JSONRPCResponse = timeout( @@ -758,23 +773,6 @@ fn write_analytics_config(codex_home: &std::path::Path, base_url: &str) -> std:: ) } -fn write_plugin_remote_sync_config( - codex_home: &std::path::Path, - base_url: &str, -) -> std::io::Result<()> { - std::fs::write( - codex_home.join("config.toml"), - format!( - r#" -chatgpt_base_url = "{base_url}" - -[features] -plugins = true -"# - ), - ) -} - fn write_plugin_marketplace( repo_root: &std::path::Path, marketplace_name: &str, diff --git a/codex-rs/app-server/tests/suite/v2/plugin_list.rs b/codex-rs/app-server/tests/suite/v2/plugin_list.rs index 28056e9ec7..bf69df3c47 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_list.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_list.rs @@ -71,7 +71,6 @@ async fn plugin_list_skips_invalid_marketplace_file_and_reports_error() -> Resul let request_id = mcp .send_plugin_list_request(PluginListParams { cwds: Some(vec![AbsolutePathBuf::try_from(repo_root.path())?]), - force_remote_sync: false, }) .await?; @@ -86,7 +85,7 @@ async fn plugin_list_skips_invalid_marketplace_file_and_reports_error() -> Resul response .marketplaces .iter() - .all(|marketplace| { marketplace.path != marketplace_path }), + .all(|marketplace| { marketplace.path.as_ref() != Some(&marketplace_path) }), "invalid marketplace should be skipped" ); assert_eq!(response.marketplace_load_errors.len(), 1); @@ -200,7 +199,6 @@ async fn plugin_list_keeps_valid_marketplaces_when_another_marketplace_fails_to_ AbsolutePathBuf::try_from(valid_repo_root.path())?, AbsolutePathBuf::try_from(invalid_repo_root.path())?, ]), - force_remote_sync: false, }) .await?; @@ -215,7 +213,7 @@ async fn plugin_list_keeps_valid_marketplaces_when_another_marketplace_fails_to_ response.marketplaces, vec![PluginMarketplaceEntry { name: "valid-marketplace".to_string(), - path: valid_marketplace_path, + path: Some(valid_marketplace_path), interface: None, plugins: vec![PluginSummary { id: "valid-plugin@valid-marketplace".to_string(), @@ -243,7 +241,6 @@ async fn plugin_list_keeps_valid_marketplaces_when_another_marketplace_fails_to_ "unexpected error: {:?}", response.marketplace_load_errors ); - assert_eq!(response.remote_sync_error, None); assert!(response.featured_plugin_ids.is_empty()); Ok(()) } @@ -314,7 +311,6 @@ async fn plugin_list_uses_alternate_discoverable_manifest_and_keeps_undiscoverab let request_id = mcp .send_plugin_list_request(PluginListParams { cwds: Some(vec![AbsolutePathBuf::try_from(repo_root.path())?]), - force_remote_sync: false, }) .await?; @@ -329,7 +325,7 @@ async fn plugin_list_uses_alternate_discoverable_manifest_and_keeps_undiscoverab response.marketplaces, vec![PluginMarketplaceEntry { name: "alternate-marketplace".to_string(), - path: marketplace_path, + path: Some(marketplace_path), interface: None, plugins: vec![ PluginSummary { @@ -355,8 +351,11 @@ async fn plugin_list_uses_alternate_discoverable_manifest_and_keeps_undiscoverab default_prompt: None, brand_color: None, composer_icon: None, + composer_icon_url: None, logo: None, + logo_url: None, screenshots: Vec::new(), + screenshot_urls: Vec::new(), }), }, PluginSummary { @@ -412,10 +411,7 @@ async fn plugin_list_accepts_omitted_cwds() -> Result<()> { timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: false, - }) + .send_plugin_list_request(PluginListParams { cwds: None }) .await?; let response: JSONRPCResponse = timeout( @@ -486,7 +482,6 @@ enabled = false let request_id = mcp .send_plugin_list_request(PluginListParams { cwds: Some(vec![AbsolutePathBuf::try_from(repo_root.path())?]), - force_remote_sync: false, }) .await?; @@ -501,11 +496,13 @@ enabled = false .marketplaces .into_iter() .find(|marketplace| { - marketplace.path - == AbsolutePathBuf::try_from( - repo_root.path().join(".agents/plugins/marketplace.json"), + marketplace.path.as_ref() + == Some( + &AbsolutePathBuf::try_from( + repo_root.path().join(".agents/plugins/marketplace.json"), + ) + .expect("absolute marketplace path"), ) - .expect("absolute marketplace path") }) .expect("expected repo marketplace entry"); @@ -641,7 +638,6 @@ enabled = false AbsolutePathBuf::try_from(workspace_enabled.path())?, AbsolutePathBuf::try_from(workspace_default.path())?, ]), - force_remote_sync: false, }) .await?; @@ -725,7 +721,6 @@ async fn plugin_list_returns_plugin_interface_with_absolute_asset_paths() -> Res let request_id = mcp .send_plugin_list_request(PluginListParams { cwds: Some(vec![AbsolutePathBuf::try_from(repo_root.path())?]), - force_remote_sync: false, }) .await?; @@ -838,7 +833,6 @@ async fn plugin_list_accepts_legacy_string_default_prompt() -> Result<()> { let request_id = mcp .send_plugin_list_request(PluginListParams { cwds: Some(vec![AbsolutePathBuf::try_from(repo_root.path())?]), - force_remote_sync: false, }) .await?; @@ -865,163 +859,6 @@ async fn plugin_list_accepts_legacy_string_default_prompt() -> Result<()> { Ok(()) } -#[tokio::test] -async fn plugin_list_force_remote_sync_returns_remote_sync_error_on_fail_open() -> Result<()> { - let codex_home = TempDir::new()?; - write_plugin_sync_config(codex_home.path(), "https://chatgpt.com/backend-api/")?; - write_openai_curated_marketplace(codex_home.path(), &["linear"])?; - write_installed_plugin(&codex_home, "openai-curated", "linear")?; - - let mut mcp = McpProcess::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: true, - }) - .await?; - - let response: JSONRPCResponse = timeout( - DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let response: PluginListResponse = to_response(response)?; - - assert!( - response - .remote_sync_error - .as_deref() - .is_some_and(|message| message.contains("chatgpt authentication required")) - ); - let curated_marketplace = response - .marketplaces - .into_iter() - .find(|marketplace| marketplace.name == "openai-curated") - .expect("expected openai-curated marketplace entry"); - assert_eq!( - curated_marketplace - .plugins - .into_iter() - .map(|plugin| (plugin.id, plugin.installed, plugin.enabled)) - .collect::>(), - vec![("linear@openai-curated".to_string(), true, false)] - ); - Ok(()) -} - -#[tokio::test] -async fn plugin_list_force_remote_sync_reconciles_curated_plugin_state() -> Result<()> { - let codex_home = TempDir::new()?; - let server = MockServer::start().await; - write_plugin_sync_config(codex_home.path(), &format!("{}/backend-api/", server.uri()))?; - write_chatgpt_auth( - codex_home.path(), - ChatGptAuthFixture::new("chatgpt-token") - .account_id("account-123") - .chatgpt_user_id("user-123") - .chatgpt_account_id("account-123"), - AuthCredentialsStoreMode::File, - )?; - write_openai_curated_marketplace(codex_home.path(), &["linear", "gmail", "calendar"])?; - write_installed_plugin(&codex_home, "openai-curated", "linear")?; - write_installed_plugin(&codex_home, "openai-curated", "gmail")?; - write_installed_plugin(&codex_home, "openai-curated", "calendar")?; - - Mock::given(method("GET")) - .and(path("/backend-api/plugins/list")) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with(ResponseTemplate::new(200).set_body_string( - r#"[ - {"id":"1","name":"linear","marketplace_name":"openai-curated","version":"1.0.0","enabled":true}, - {"id":"2","name":"gmail","marketplace_name":"openai-curated","version":"1.0.0","enabled":false} -]"#, - )) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/backend-api/plugins/featured")) - .and(query_param("platform", "codex")) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with( - ResponseTemplate::new(200) - .set_body_string(r#"["linear@openai-curated","calendar@openai-curated"]"#), - ) - .mount(&server) - .await; - - let mut mcp = McpProcess::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: true, - }) - .await?; - - let response: JSONRPCResponse = timeout( - DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let response: PluginListResponse = to_response(response)?; - assert_eq!(response.remote_sync_error, None); - assert_eq!( - response.featured_plugin_ids, - vec![ - "linear@openai-curated".to_string(), - "calendar@openai-curated".to_string(), - ] - ); - - let curated_marketplace = response - .marketplaces - .into_iter() - .find(|marketplace| marketplace.name == "openai-curated") - .expect("expected openai-curated marketplace entry"); - assert_eq!( - curated_marketplace - .plugins - .into_iter() - .map(|plugin| (plugin.id, plugin.installed, plugin.enabled)) - .collect::>(), - vec![ - ("linear@openai-curated".to_string(), true, true), - ("gmail@openai-curated".to_string(), false, false), - ("calendar@openai-curated".to_string(), false, false), - ] - ); - - let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?; - assert!(config.contains(r#"[plugins."linear@openai-curated"]"#)); - assert!(!config.contains(r#"[plugins."gmail@openai-curated"]"#)); - assert!(!config.contains(r#"[plugins."calendar@openai-curated"]"#)); - - assert!( - codex_home - .path() - .join("plugins/cache/openai-curated/linear/local") - .is_dir() - ); - assert!( - !codex_home - .path() - .join("plugins/cache/openai-curated/gmail") - .exists() - ); - assert!( - !codex_home - .path() - .join("plugins/cache/openai-curated/calendar") - .exists() - ); - Ok(()) -} - #[tokio::test] async fn app_server_startup_remote_plugin_sync_runs_once() -> Result<()> { let codex_home = TempDir::new()?; @@ -1069,10 +906,7 @@ async fn app_server_startup_remote_plugin_sync_runs_once() -> Result<()> { wait_for_remote_plugin_request_count(&server, "/plugins/list", /*expected_count*/ 1) .await?; let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: false, - }) + .send_plugin_list_request(PluginListParams { cwds: None }) .await?; let response: JSONRPCResponse = timeout( DEFAULT_TIMEOUT, @@ -1128,10 +962,7 @@ async fn plugin_list_fetches_featured_plugin_ids_without_chatgpt_auth() -> Resul timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: false, - }) + .send_plugin_list_request(PluginListParams { cwds: None }) .await?; let response: JSONRPCResponse = timeout( @@ -1145,7 +976,6 @@ async fn plugin_list_fetches_featured_plugin_ids_without_chatgpt_auth() -> Resul response.featured_plugin_ids, vec!["linear@openai-curated".to_string()] ); - assert_eq!(response.remote_sync_error, None); Ok(()) } @@ -1169,10 +999,7 @@ async fn plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request() -> wait_for_featured_plugin_request_count(&server, /*expected_count*/ 1).await?; let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - force_remote_sync: false, - }) + .send_plugin_list_request(PluginListParams { cwds: None }) .await?; let response: JSONRPCResponse = timeout( @@ -1186,7 +1013,6 @@ async fn plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request() -> response.featured_plugin_ids, vec!["linear@openai-curated".to_string()] ); - assert_eq!(response.remote_sync_error, None); Ok(()) } diff --git a/codex-rs/app-server/tests/suite/v2/plugin_read.rs b/codex-rs/app-server/tests/suite/v2/plugin_read.rs index 20114e79ba..f5681e9d4d 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_read.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_read.rs @@ -45,6 +45,96 @@ use tokio::time::timeout; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); +#[tokio::test] +async fn plugin_read_rejects_missing_read_source() -> Result<()> { + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_read_request(PluginReadParams { + marketplace_path: None, + remote_marketplace_name: None, + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("requires exactly one of marketplacePath or remoteMarketplaceName") + ); + Ok(()) +} + +#[tokio::test] +async fn plugin_read_rejects_multiple_read_sources() -> Result<()> { + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_read_request(PluginReadParams { + marketplace_path: Some(AbsolutePathBuf::try_from( + codex_home.path().join("marketplace.json"), + )?), + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("requires exactly one of marketplacePath or remoteMarketplaceName") + ); + Ok(()) +} + +#[tokio::test] +async fn plugin_read_rejects_remote_marketplace_until_remote_read_is_supported() -> Result<()> { + let codex_home = TempDir::new()?; + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_plugin_read_request(PluginReadParams { + marketplace_path: None, + remote_marketplace_name: Some("openai-curated".to_string()), + plugin_name: "sample-plugin".to_string(), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(err.error.code, -32600); + assert!( + err.error + .message + .contains("remote plugin read is not supported yet") + ); + assert!(err.error.message.contains("openai-curated")); + Ok(()) +} + #[tokio::test] async fn plugin_read_returns_plugin_details_with_bundle_contents() -> Result<()> { let codex_home = TempDir::new()?; @@ -179,7 +269,8 @@ enabled = true AbsolutePathBuf::try_from(repo_root.path().join(".agents/plugins/marketplace.json"))?; let request_id = mcp .send_plugin_read_request(PluginReadParams { - marketplace_path: marketplace_path.clone(), + marketplace_path: Some(marketplace_path.clone()), + remote_marketplace_name: None, plugin_name: "demo-plugin".to_string(), }) .await?; @@ -326,7 +417,8 @@ async fn plugin_read_returns_app_needs_auth() -> Result<()> { let request_id = mcp .send_plugin_read_request(PluginReadParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name: "sample-plugin".to_string(), }) .await?; @@ -392,9 +484,10 @@ async fn plugin_read_accepts_legacy_string_default_prompt() -> Result<()> { let request_id = mcp .send_plugin_read_request(PluginReadParams { - marketplace_path: AbsolutePathBuf::try_from( + marketplace_path: Some(AbsolutePathBuf::try_from( repo_root.path().join(".agents/plugins/marketplace.json"), - )?, + )?), + remote_marketplace_name: None, plugin_name: "demo-plugin".to_string(), }) .await?; @@ -446,9 +539,10 @@ async fn plugin_read_returns_invalid_request_when_plugin_is_missing() -> Result< let request_id = mcp .send_plugin_read_request(PluginReadParams { - marketplace_path: AbsolutePathBuf::try_from( + marketplace_path: Some(AbsolutePathBuf::try_from( repo_root.path().join(".agents/plugins/marketplace.json"), - )?, + )?), + remote_marketplace_name: None, plugin_name: "missing-plugin".to_string(), }) .await?; @@ -498,9 +592,10 @@ async fn plugin_read_returns_invalid_request_when_plugin_manifest_is_missing() - let request_id = mcp .send_plugin_read_request(PluginReadParams { - marketplace_path: AbsolutePathBuf::try_from( + marketplace_path: Some(AbsolutePathBuf::try_from( repo_root.path().join(".agents/plugins/marketplace.json"), - )?, + )?), + remote_marketplace_name: None, plugin_name: "demo-plugin".to_string(), }) .await?; diff --git a/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs b/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs index 00fabe4832..512cce3994 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs @@ -16,12 +16,6 @@ use pretty_assertions::assert_eq; use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; -use wiremock::Mock; -use wiremock::MockServer; -use wiremock::ResponseTemplate; -use wiremock::matchers::header; -use wiremock::matchers::method; -use wiremock::matchers::path; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); @@ -44,7 +38,6 @@ enabled = true let params = PluginUninstallParams { plugin_id: "sample-plugin@debug".to_string(), - force_remote_sync: false, }; let request_id = mcp.send_plugin_uninstall_request(params.clone()).await?; @@ -77,74 +70,6 @@ enabled = true Ok(()) } -#[tokio::test] -async fn plugin_uninstall_force_remote_sync_calls_remote_uninstall_first() -> Result<()> { - let server = MockServer::start().await; - let codex_home = TempDir::new()?; - write_installed_plugin(&codex_home, "debug", "sample-plugin")?; - std::fs::write( - codex_home.path().join("config.toml"), - format!( - r#"chatgpt_base_url = "{}/backend-api/" - -[features] -plugins = true - -[plugins."sample-plugin@debug"] -enabled = true -"#, - server.uri() - ), - )?; - write_chatgpt_auth( - codex_home.path(), - ChatGptAuthFixture::new("chatgpt-token") - .account_id("account-123") - .chatgpt_user_id("user-123") - .chatgpt_account_id("account-123"), - AuthCredentialsStoreMode::File, - )?; - - Mock::given(method("POST")) - .and(path("/backend-api/plugins/sample-plugin@debug/uninstall")) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with( - ResponseTemplate::new(200) - .set_body_string(r#"{"id":"sample-plugin@debug","enabled":false}"#), - ) - .expect(1) - .mount(&server) - .await; - - let mut mcp = McpProcess::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_plugin_uninstall_request(PluginUninstallParams { - plugin_id: "sample-plugin@debug".to_string(), - force_remote_sync: true, - }) - .await?; - let response: JSONRPCResponse = timeout( - DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let response: PluginUninstallResponse = to_response(response)?; - assert_eq!(response, PluginUninstallResponse {}); - - assert!( - !codex_home - .path() - .join("plugins/cache/debug/sample-plugin") - .exists() - ); - let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?; - assert!(!config.contains(r#"[plugins."sample-plugin@debug"]"#)); - Ok(()) -} - #[tokio::test] async fn plugin_uninstall_tracks_analytics_event() -> Result<()> { let analytics_server = start_analytics_events_server().await?; @@ -172,7 +97,6 @@ async fn plugin_uninstall_tracks_analytics_event() -> Result<()> { let request_id = mcp .send_plugin_uninstall_request(PluginUninstallParams { plugin_id: "sample-plugin@debug".to_string(), - force_remote_sync: false, }) .await?; let response: JSONRPCResponse = timeout( diff --git a/codex-rs/clippy.toml b/codex-rs/clippy.toml index a2aae6591b..2feed8a487 100644 --- a/codex-rs/clippy.toml +++ b/codex-rs/clippy.toml @@ -1,5 +1,10 @@ allow-expect-in-tests = true allow-unwrap-in-tests = true +await-holding-invalid-types = [ + "tokio::sync::MutexGuard", + "tokio::sync::RwLockReadGuard", + "tokio::sync::RwLockWriteGuard", +] disallowed-methods = [ { path = "ratatui::style::Color::Rgb", reason = "Use ANSI colors, which work better in various terminal themes." }, { path = "ratatui::style::Color::Indexed", reason = "Use ANSI colors, which work better in various terminal themes." }, diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index bc72aee411..aea9b917fb 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -229,6 +229,10 @@ impl ResponsesWebsocketConnection { let current_span = Span::current(); tokio::spawn( + #[expect( + clippy::await_holding_invalid_type, + reason = "the guard serializes exclusive use of the websocket stream for the lifetime of the response stream" + )] async move { if let Some(model) = server_model { let _ = tx_event.send(Ok(ResponseEvent::ServerModel(model))).await; diff --git a/codex-rs/core/src/agent_identity.rs b/codex-rs/core/src/agent_identity.rs index cfcab507bb..5684c55ce0 100644 --- a/codex-rs/core/src/agent_identity.rs +++ b/codex-rs/core/src/agent_identity.rs @@ -22,7 +22,7 @@ use rand::TryRngCore; use rand::rngs::OsRng; use serde::Deserialize; use serde::Serialize; -use tokio::sync::Mutex; +use tokio::sync::Semaphore; use tracing::debug; use tracing::info; use tracing::warn; @@ -42,7 +42,7 @@ pub(crate) struct AgentIdentityManager { chatgpt_base_url: String, feature_enabled: bool, abom: AgentBillOfMaterials, - ensure_lock: Arc>, + ensure_lock: Arc, } impl std::fmt::Debug for AgentIdentityManager { @@ -110,7 +110,7 @@ impl AgentIdentityManager { chatgpt_base_url: config.chatgpt_base_url.clone(), feature_enabled: config.features.enabled(Feature::UseAgentIdentity), abom: build_abom(session_source), - ensure_lock: Arc::new(Mutex::new(())), + ensure_lock: Arc::new(Semaphore::new(1)), } } @@ -137,7 +137,11 @@ impl AgentIdentityManager { auth: &CodexAuth, binding: &AgentIdentityBinding, ) -> Result { - let _guard = self.ensure_lock.lock().await; + let _guard = self + .ensure_lock + .acquire() + .await + .map_err(|_| anyhow::anyhow!("agent identity ensure semaphore closed"))?; if let Some(stored_identity) = self.load_stored_identity(auth, binding)? { info!( @@ -339,7 +343,7 @@ impl AgentIdentityManager { chatgpt_base_url, feature_enabled, abom: build_abom(session_source), - ensure_lock: Arc::new(Mutex::new(())), + ensure_lock: Arc::new(Semaphore::new(1)), } } } diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 798bacfbf3..e1af955a16 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -27,6 +27,7 @@ use codex_shell_command::is_dangerous_command::command_might_be_dangerous; use codex_shell_command::is_safe_command::is_known_safe_command; use thiserror::Error; use tokio::fs; +use tokio::sync::Semaphore; use tokio::task::spawn_blocking; use tracing::instrument; @@ -190,7 +191,7 @@ pub enum ExecPolicyUpdateError { pub(crate) struct ExecPolicyManager { policy: ArcSwap, - update_lock: tokio::sync::Mutex<()>, + update_lock: Semaphore, } pub(crate) struct ExecApprovalRequest<'a> { @@ -206,7 +207,7 @@ impl ExecPolicyManager { pub(crate) fn new(policy: Arc) -> Self { Self { policy: ArcSwap::from(policy), - update_lock: tokio::sync::Mutex::new(()), + update_lock: Semaphore::new(1), } } @@ -324,7 +325,15 @@ impl ExecPolicyManager { codex_home: &Path, amendment: &ExecPolicyAmendment, ) -> Result<(), ExecPolicyUpdateError> { - let _update_guard = self.update_lock.lock().await; + let _update_guard = + self.update_lock + .acquire() + .await + .map_err(|_| ExecPolicyUpdateError::AddRule { + source: ExecPolicyRuleError::InvalidRule( + "exec policy update semaphore closed".to_string(), + ), + })?; let policy_path = default_policy_path(codex_home); spawn_blocking({ let policy_path = policy_path.clone(); @@ -369,7 +378,15 @@ impl ExecPolicyManager { decision: Decision, justification: Option, ) -> Result<(), ExecPolicyUpdateError> { - let _update_guard = self.update_lock.lock().await; + let _update_guard = + self.update_lock + .acquire() + .await + .map_err(|_| ExecPolicyUpdateError::AddRule { + source: ExecPolicyRuleError::InvalidRule( + "exec policy update semaphore closed".to_string(), + ), + })?; let policy_path = default_policy_path(codex_home); let host = host.to_string(); spawn_blocking({ diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 3721dc8dfe..996c80c4f2 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -19,6 +19,7 @@ use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SubAgentSource; use serde_json::Value; use tokio::sync::Mutex; +use tokio::sync::Semaphore; use tokio_util::sync::CancellationToken; use tracing::warn; @@ -91,7 +92,7 @@ struct GuardianReviewSession { codex: Codex, cancel_token: CancellationToken, reuse_key: GuardianReviewSessionReuseKey, - review_lock: Mutex<()>, + review_lock: Semaphore, state: Mutex, } @@ -264,6 +265,10 @@ impl GuardianReviewSessionManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "review session selection and trunk spawning must stay serialized" + )] pub(crate) async fn run_review( &self, params: GuardianReviewSessionParams, @@ -281,7 +286,7 @@ impl GuardianReviewSessionManager { Ok(mut state) => { if let Some(trunk) = state.trunk.as_ref() && trunk.reuse_key != next_reuse_key - && trunk.review_lock.try_lock().is_ok() + && trunk.review_lock.try_acquire().is_ok() { stale_trunk_to_shutdown = state.trunk.take(); } @@ -336,7 +341,7 @@ impl GuardianReviewSessionManager { .await; } - let trunk_guard = match trunk.review_lock.try_lock() { + let trunk_guard = match trunk.review_lock.try_acquire() { Ok(trunk_guard) => trunk_guard, Err(_) => { return Box::pin(self.run_ephemeral_review( @@ -375,7 +380,7 @@ impl GuardianReviewSessionManager { reuse_key, codex, cancel_token: CancellationToken::new(), - review_lock: Mutex::new(()), + review_lock: Semaphore::new(1), state: Mutex::new(GuardianReviewState { prior_review_count: 0, last_reviewed_transcript_cursor: None, @@ -397,7 +402,7 @@ impl GuardianReviewSessionManager { reuse_key, codex, cancel_token: CancellationToken::new(), - review_lock: Mutex::new(()), + review_lock: Semaphore::new(1), state: Mutex::new(GuardianReviewState { prior_review_count: 0, last_reviewed_transcript_cursor: None, @@ -531,7 +536,7 @@ async fn spawn_guardian_review_session( codex, cancel_token, reuse_key, - review_lock: Mutex::new(()), + review_lock: Semaphore::new(1), state: Mutex::new(GuardianReviewState { prior_review_count, last_reviewed_transcript_cursor: initial_transcript_cursor, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 32c7cf879e..c5d4068d27 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -494,6 +494,10 @@ async fn execute_mcp_tool_call( ) } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP sandbox metadata reads through the session-owned manager guard" +)] async fn augment_mcp_tool_request_meta_with_sandbox_state( sess: &Session, turn_context: &TurnContext, @@ -1067,6 +1071,10 @@ fn mcp_tool_approval_callsite_mode( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP approval metadata reads through the session-owned manager guard" +)] pub(crate) async fn lookup_mcp_tool_metadata( sess: &Session, turn_context: &TurnContext, @@ -1149,6 +1157,10 @@ fn get_mcp_app_resource_uri( }) } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP app metadata reads through the session-owned manager guard" +)] async fn lookup_mcp_app_usage_metadata( sess: &Session, server: &str, diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index d3afbf8978..5d49cf739d 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -70,7 +70,7 @@ use std::sync::RwLock; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use std::time::Instant; -use tokio::sync::Mutex; +use tokio::sync::Semaphore; use toml_edit::value; use tracing::info; use tracing::warn; @@ -349,7 +349,7 @@ pub struct PluginsManager { configured_marketplace_upgrade_state: RwLock, non_curated_cache_refresh_state: RwLock, cached_enabled_outcome: RwLock>, - remote_sync_lock: Mutex<()>, + remote_sync_lock: Semaphore, restriction_product: Option, analytics_events_client: RwLock>, } @@ -379,7 +379,7 @@ impl PluginsManager { ), non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), cached_enabled_outcome: RwLock::new(None), - remote_sync_lock: Mutex::new(()), + remote_sync_lock: Semaphore::new(1), restriction_product, analytics_events_client: RwLock::new(None), } @@ -565,9 +565,7 @@ impl PluginsManager { self.restriction_product, )?; let plugin_id = resolved.plugin_id.as_key(); - // This only forwards the backend mutation before the local install flow. We rely on - // `plugin/list(forceRemoteSync=true)` to sync local state rather than doing an extra - // reconcile pass here. + // This only forwards the backend mutation before the local install flow. codex_core_plugins::remote::enable_remote_plugin( &remote_plugin_service_config(config), auth, @@ -655,11 +653,11 @@ impl PluginsManager { auth: Option<&CodexAuth>, plugin_id: String, ) -> Result<(), PluginUninstallError> { + // TODO: Remove this legacy remote-sync path once remote plugins have + // their own manager and installed-state API. let plugin_id = PluginId::parse(&plugin_id)?; let plugin_key = plugin_id.as_key(); - // This only forwards the backend mutation before the local uninstall flow. We rely on - // `plugin/list(forceRemoteSync=true)` to sync local state rather than doing an extra - // reconcile pass here. + // This only forwards the backend mutation before the local uninstall flow. codex_core_plugins::remote::uninstall_remote_plugin( &remote_plugin_service_config(config), auth, @@ -708,7 +706,9 @@ impl PluginsManager { auth: Option<&CodexAuth>, additive_only: bool, ) -> Result { - let _remote_sync_guard = self.remote_sync_lock.lock().await; + let _remote_sync_guard = self.remote_sync_lock.acquire().await.map_err(|_| { + PluginRemoteSyncError::Config(anyhow::anyhow!("remote plugin sync semaphore closed")) + })?; if !config.features.enabled(Feature::Plugins) { return Ok(RemotePluginSyncResult::default()); diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 72cfc57822..23090c6cab 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -468,6 +468,10 @@ pub async fn reload_user_config(sess: &Arc) { sess.reload_user_config_layer().await; } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool listing reads through the session-owned manager guard" +)] pub async fn list_mcp_tools(sess: &Session, config: &Arc, sub_id: String) { let mcp_connection_manager = sess.services.mcp_connection_manager.read().await; let auth = sess.services.auth_manager.auth().await; diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 56628f0543..7465e51f3f 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -1,6 +1,10 @@ use super::*; impl Session { + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_mcp_server_elicitation( &self, turn_context: &TurnContext, @@ -80,6 +84,10 @@ impl Session { rx_response.await.ok() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and manager fallback must stay serialized" + )] pub async fn resolve_elicitation( &self, server_name: String, @@ -111,6 +119,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn list_resources( &self, server: &str, @@ -124,6 +136,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn list_resource_templates( &self, server: &str, @@ -137,6 +153,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource calls are serialized through the session-owned manager guard" + )] pub async fn read_resource( &self, server: &str, @@ -150,6 +170,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool calls are serialized through the session-owned manager guard" + )] pub async fn call_tool( &self, server: &str, @@ -165,6 +189,10 @@ impl Session { .await } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP tool metadata reads through the session-owned manager guard" + )] pub(crate) async fn resolve_mcp_tool_info(&self, tool_name: &ToolName) -> Option { self.services .mcp_connection_manager diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 766bd04e69..5348a59d6f 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -892,7 +892,10 @@ impl Session { let Some(started_proxy) = self.services.network_proxy.as_ref() else { return; }; - let _refresh_guard = self.managed_network_proxy_refresh_lock.lock().await; + let Ok(_refresh_guard) = self.managed_network_proxy_refresh_lock.acquire().await else { + error!("managed network proxy refresh semaphore closed"); + return; + }; let session_configuration = { let state = self.state.lock().await; state.session_configuration.clone() @@ -1750,7 +1753,11 @@ impl Session { amendment: &NetworkPolicyAmendment, network_approval_context: &NetworkApprovalContext, ) -> anyhow::Result<()> { - let _refresh_guard = self.managed_network_proxy_refresh_lock.lock().await; + let _refresh_guard = self + .managed_network_proxy_refresh_lock + .acquire() + .await + .map_err(|_| anyhow::anyhow!("managed network proxy refresh semaphore closed"))?; let host = Self::validated_network_policy_amendment_host(amendment, network_approval_context)?; let codex_home = self @@ -1854,6 +1861,10 @@ impl Session { /// be used to derive the available decisions via /// [ExecApprovalRequestEvent::default_available_decisions]. #[allow(clippy::too_many_arguments)] + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_command_approval( &self, turn_context: &TurnContext, @@ -1925,6 +1936,10 @@ impl Session { rx_approve.await.unwrap_or(ReviewDecision::Abort) } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_patch_approval( &self, turn_context: &TurnContext, @@ -1961,6 +1976,10 @@ impl Session { rx_approve } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_permissions( &self, turn_context: &TurnContext, @@ -2016,6 +2035,10 @@ impl Session { rx_response.await.ok() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn request_user_input( &self, turn_context: &TurnContext, @@ -2048,6 +2071,10 @@ impl Session { rx_response.await.ok() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_user_input_response( &self, sub_id: &str, @@ -2073,6 +2100,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_request_permissions_response( &self, call_id: &str, @@ -2114,6 +2145,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn reads must stay consistent with the matching turn state" + )] pub(crate) async fn granted_turn_permissions(&self) -> Option { let active = self.active_turn.lock().await; let active = active.as_ref()?; @@ -2126,6 +2161,10 @@ impl Session { state.granted_permissions() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_dynamic_tool_response(&self, call_id: &str, response: DynamicToolResponse) { let entry = { let mut active = self.active_turn.lock().await; @@ -2147,6 +2186,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn notify_approval(&self, approval_id: &str, decision: ReviewDecision) { let entry = { let mut active = self.active_turn.lock().await; @@ -2307,6 +2350,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP app context rendering reads through the session-owned manager guard" + )] pub(crate) async fn build_initial_context( &self, turn_context: &TurnContext, @@ -2751,6 +2798,10 @@ impl Session { /// Inject additional user input into the currently active turn. /// /// Returns the active turn id when accepted. + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn steer_input( &self, input: Vec, @@ -2810,6 +2861,10 @@ impl Session { } /// Returns the input if there was no task running to inject into. + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn inject_response_items( &self, input: Vec, @@ -2875,6 +2930,10 @@ impl Session { self.mailbox_rx.lock().await.has_pending_trigger_turn() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn prepend_pending_input(&self, input: Vec) -> Result<(), ()> { let mut active = self.active_turn.lock().await; match active.as_mut() { @@ -2887,6 +2946,10 @@ impl Session { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] pub async fn get_pending_input(&self) -> Vec { let (pending_input, accepts_mailbox_delivery) = { let mut active = self.active_turn.lock().await; @@ -2942,6 +3005,10 @@ impl Session { !self.idle_pending_input.lock().await.is_empty() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state reads must remain atomic" + )] pub async fn has_pending_input(&self) -> bool { let (has_turn_pending_input, accepts_mailbox_delivery) = { let active = self.active_turn.lock().await; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 766ac79ec1..98ea80a86e 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1,4 +1,5 @@ use super::*; +use tokio::sync::Semaphore; /// Context for an initialized model agent /// @@ -11,7 +12,7 @@ pub(crate) struct Session { pub(super) state: Mutex, /// Serializes rebuild/apply cycles for the running proxy; each cycle /// rebuilds from the current SessionState while holding this lock. - pub(super) managed_network_proxy_refresh_lock: Mutex<()>, + pub(super) managed_network_proxy_refresh_lock: Semaphore, /// The set of enabled features should be invariant for the lifetime of the /// session. pub(super) features: ManagedFeatures, @@ -207,6 +208,10 @@ pub(crate) struct AppServerClientMetadata { impl Session { #[instrument(name = "session_init", level = "info", skip_all)] #[allow(clippy::too_many_arguments)] + #[expect( + clippy::await_holding_invalid_type, + reason = "session initialization must serialize access through session-owned manager guards" + )] pub(crate) async fn new( mut session_configuration: SessionConfiguration, config: Arc, @@ -695,7 +700,7 @@ impl Session { agent_status, out_of_band_elicitation_paused, state: Mutex::new(state), - managed_network_proxy_refresh_lock: Mutex::new(()), + managed_network_proxy_refresh_lock: Semaphore::new(1), features: config.features.clone(), pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index abe96c5490..a82b41ddc1 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -108,6 +108,7 @@ use opentelemetry::trace::TraceContextExt; use opentelemetry::trace::TraceId; use std::path::Path; use std::time::Duration; +use tokio::sync::Semaphore; use tokio::time::sleep; use tokio::time::timeout; use tracing_opentelemetry::OpenTelemetrySpanExt; @@ -3021,7 +3022,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { agent_status: agent_status_tx, out_of_band_elicitation_paused: watch::channel(false).0, state: Mutex::new(state), - managed_network_proxy_refresh_lock: Mutex::new(()), + managed_network_proxy_refresh_lock: Semaphore::new(1), features: config.features.clone(), pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), @@ -3984,7 +3985,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( agent_status: agent_status_tx, out_of_band_elicitation_paused: watch::channel(false).0, state: Mutex::new(state), - managed_network_proxy_refresh_lock: Mutex::new(()), + managed_network_proxy_refresh_lock: Semaphore::new(1), features: config.features.clone(), pending_mcp_server_refresh_config: Mutex::new(None), conversation: Arc::new(RealtimeConversationManager::new()), @@ -5659,6 +5660,10 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn fatal_tool_error_stops_turn_and_reports_error() { let (session, turn_context, _rx) = make_session_and_context_with_rx().await; let tools = { diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 9ccd6eb4e0..97543105b6 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -298,6 +298,10 @@ async fn process_compacted_history_preserves_separate_guardian_developer_message #[tokio::test] #[cfg(unix)] +#[expect( + clippy::await_holding_invalid_type, + reason = "test mutates active turn state directly to seed granted permissions" +)] async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_permissions_feature() { let (mut session, turn_context_raw) = make_session_and_context().await; session diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index ae7088c603..aaad680aa0 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -126,6 +126,10 @@ use tracing::warn; /// - If the model sends only an assistant message, we record it in the /// conversation history and consider the turn complete. /// +#[expect( + clippy::await_holding_invalid_type, + reason = "turn execution must keep active-turn state transitions atomic" +)] pub(crate) async fn run_turn( sess: Arc, turn_context: Arc, @@ -1126,6 +1130,10 @@ async fn run_sampling_request( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "tool router construction reads through the session-owned manager guard" +)] pub(crate) async fn built_tools( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index 5049836393..575d3f9a89 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -259,6 +259,10 @@ pub(super) async fn build_enabled_tools( collect_code_mode_tool_definitions(&specs) } +#[expect( + clippy::await_holding_invalid_type, + reason = "nested tool router construction reads through the session-owned manager guard" +)] async fn build_nested_router(exec: &ExecContext) -> ToolRouter { let nested_tools_config = exec.turn.tools_config.for_code_mode_nested_tools(); let listed_mcp_tools = exec diff --git a/codex-rs/core/src/tools/handlers/dynamic.rs b/codex-rs/core/src/tools/handlers/dynamic.rs index 140d586be9..c989d70f19 100644 --- a/codex-rs/core/src/tools/handlers/dynamic.rs +++ b/codex-rs/core/src/tools/handlers/dynamic.rs @@ -71,6 +71,10 @@ impl ToolHandler for DynamicToolHandler { } } +#[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and dynamic tool response registration must remain atomic" +)] async fn request_dynamic_tool( session: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/tools/handlers/mcp_resource.rs b/codex-rs/core/src/tools/handlers/mcp_resource.rs index 67f44cb699..fa4a066741 100644 --- a/codex-rs/core/src/tools/handlers/mcp_resource.rs +++ b/codex-rs/core/src/tools/handlers/mcp_resource.rs @@ -240,6 +240,10 @@ impl ToolHandler for McpResourceHandler { } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource listing reads through the session-owned manager guard" +)] async fn handle_list_resources( session: Arc, turn: Arc, @@ -344,6 +348,10 @@ async fn handle_list_resources( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "MCP resource template listing reads through the session-owned manager guard" +)] async fn handle_list_resource_templates( session: Arc, turn: Arc, diff --git a/codex-rs/core/src/tools/handlers/tool_suggest.rs b/codex-rs/core/src/tools/handlers/tool_suggest.rs index cc433601bc..5c51c90d66 100644 --- a/codex-rs/core/src/tools/handlers/tool_suggest.rs +++ b/codex-rs/core/src/tools/handlers/tool_suggest.rs @@ -34,6 +34,10 @@ impl ToolHandler for ToolSuggestHandler { ToolKind::Function } + #[expect( + clippy::await_holding_invalid_type, + reason = "tool suggestion discovery reads through the session-owned manager guard" + )] async fn handle(&self, invocation: ToolInvocation) -> Result { let ToolInvocation { payload, @@ -193,6 +197,10 @@ async fn verify_tool_suggestion_completed( } } +#[expect( + clippy::await_holding_invalid_type, + reason = "connector cache refresh reads through the session-owned manager guard" +)] async fn refresh_missing_suggested_connectors( session: &crate::session::session::Session, turn: &crate::session::turn_context::TurnContext, diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index deaab9c2f3..075b9d7f95 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -835,6 +835,10 @@ impl JsReplManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl kernel initialization must be serialized with kernel state" + )] pub async fn execute( &self, session: Arc, @@ -1161,6 +1165,10 @@ impl JsReplManager { Ok(kernel_path) } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl stdin writes must be serialized per kernel" + )] async fn write_message( stdin: &Arc>, msg: &HostToKernel, @@ -1208,6 +1216,10 @@ impl JsReplManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "js_repl child shutdown must serialize process inspection and termination" + )] async fn kill_kernel_child(child: &Arc>, reason: &'static str) { let mut guard = child.lock().await; let pid = guard.id(); @@ -1531,6 +1543,10 @@ impl JsReplManager { } } + #[expect( + clippy::await_holding_invalid_type, + reason = "nested js_repl tool routing reads through the session-owned manager guard" + )] async fn run_tool_request(exec: ExecContext, req: RunToolRequest) -> RunToolResult { if is_js_repl_internal_tool(&req.tool_name) { let error = "js_repl cannot invoke itself".to_string(); diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 0e0b7b36dd..65202f624f 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -234,6 +234,10 @@ impl ToolRegistry { // } // } + #[expect( + clippy::await_holding_invalid_type, + reason = "tool dispatch must keep active-turn accounting atomic" + )] pub(crate) async fn dispatch_any( &self, invocation: ToolInvocation, diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index 1a59f1e1ba..14536e3985 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -14,6 +14,10 @@ use super::ToolRouter; use super::ToolRouterParams; #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { let (session, mut turn) = make_session_and_context().await; turn.tools_config.js_repl_tools_only = true; @@ -68,6 +72,10 @@ async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> { let (session, mut turn) = make_session_and_context().await; turn.tools_config.js_repl_tools_only = true; @@ -171,6 +179,10 @@ async fn js_repl_tools_only_blocks_namespaced_js_repl_tool() -> anyhow::Result<( } #[tokio::test] +#[expect( + clippy::await_holding_invalid_type, + reason = "test builds a router from session-owned MCP manager state" +)] async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow::Result<()> { let (session, turn) = make_session_and_context().await; let mcp_tools = session diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index f9d6d9402c..47254ab177 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -1,5 +1,6 @@ #![cfg(not(target_os = "windows"))] +use anyhow::Context; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use codex_exec_server::CreateDirectoryOptions; @@ -127,10 +128,10 @@ async fn write_workspace_png( write_workspace_file(test, rel_path, png_bytes(width, height, rgba)?).await } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { - skip_if_no_network!(Ok(())); - +async fn assert_user_turn_local_image_resizes_to( + original_dimensions: (u32, u32), + expected_dimensions: (u32, u32), +) -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); @@ -142,8 +143,7 @@ async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { .. } = &test; - let original_width = 2304; - let original_height = 864; + let (original_width, original_height) = original_dimensions; let local_image_dir = tempfile::tempdir()?; let abs_path = local_image_dir.path().join("example.png"); let image = ImageBuffer::from_pixel(original_width, original_height, Rgba([20u8, 40, 60, 255])); @@ -187,7 +187,7 @@ async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { let body = mock.single_request().body_json(); let image_message = - find_image_message(&body).expect("pending input image message not included in request"); + find_image_message(&body).context("pending input image message not included in request")?; let image_url = image_message .get("content") .and_then(Value::as_array) @@ -200,26 +200,37 @@ async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { } }) }) - .expect("image_url present"); + .context("image_url present")?; let (prefix, encoded) = image_url .split_once(',') - .expect("image url contains data prefix"); + .context("image url contains data prefix")?; assert_eq!(prefix, "data:image/png;base64"); let decoded = BASE64_STANDARD .decode(encoded) - .expect("image data decodes from base64 for request"); - let resized = load_from_memory(&decoded).expect("load resized image"); + .context("image data decodes from base64 for request")?; + let resized = load_from_memory(&decoded).context("load resized image")?; let (width, height) = resized.dimensions(); - assert!(width <= 2048); - assert!(height <= 768); - assert!(width < original_width); - assert!(height < original_height); + assert_eq!((width, height), expected_dimensions); Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + assert_user_turn_local_image_resizes_to((2304, 864), (2048, 768)).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_turn_with_vertical_local_image_resizes_to_square_bounds() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + assert_user_turn_local_image_resizes_to((1024, 4096), (512, 2048)).await +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -347,10 +358,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { .expect("image data decodes from base64 for request"); let resized = load_from_memory(&decoded).expect("load resized image"); let (resized_width, resized_height) = resized.dimensions(); - assert!(resized_width <= 2048); - assert!(resized_height <= 768); - assert!(resized_width < original_width); - assert!(resized_height < original_height); + assert_eq!((resized_width, resized_height), (2048, 768)); Ok(()) } @@ -637,10 +645,7 @@ async fn view_image_tool_treats_null_detail_as_omitted() -> anyhow::Result<()> { .expect("image data decodes from base64 for request"); let resized = load_from_memory(&decoded).expect("load resized image"); let (width, height) = resized.dimensions(); - assert!(width <= 2048); - assert!(height <= 768); - assert!(width < original_width); - assert!(height < original_height); + assert_eq!((width, height), (2048, 768)); Ok(()) } @@ -740,10 +745,7 @@ async fn view_image_tool_resizes_when_model_lacks_original_detail_support() -> a .expect("image data decodes from base64 for request"); let resized = load_from_memory(&decoded).expect("load resized image"); let (resized_width, resized_height) = resized.dimensions(); - assert!(resized_width <= 2048); - assert!(resized_height <= 768); - assert!(resized_width < original_width); - assert!(resized_height < original_height); + assert_eq!((resized_width, resized_height), (2048, 768)); Ok(()) } @@ -841,10 +843,7 @@ async fn view_image_tool_does_not_force_original_resolution_with_capability_only .expect("image data decodes from base64 for request"); let resized = load_from_memory(&decoded).expect("load resized image"); let (resized_width, resized_height) = resized.dimensions(); - assert!(resized_width <= 2048); - assert!(resized_height <= 768); - assert!(resized_width < original_width); - assert!(resized_height < original_height); + assert_eq!((resized_width, resized_height), (2048, 768)); Ok(()) } diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index c67d73fedf..7823a71eaf 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -12,7 +12,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::sync::RwLock; -use tokio::sync::Mutex as AsyncMutex; +use tokio::sync::Semaphore; use tokio::sync::watch; use codex_app_server_protocol::AuthMode; @@ -1178,7 +1178,7 @@ pub struct AuthManager { enable_codex_api_key_env: bool, auth_credentials_store_mode: AuthCredentialsStoreMode, forced_chatgpt_workspace_id: RwLock>, - refresh_lock: AsyncMutex<()>, + refresh_lock: Semaphore, external_auth: RwLock>>, auth_state_tx: watch::Sender<()>, } @@ -1246,7 +1246,7 @@ impl AuthManager { enable_codex_api_key_env, auth_credentials_store_mode, forced_chatgpt_workspace_id: RwLock::new(None), - refresh_lock: AsyncMutex::new(()), + refresh_lock: Semaphore::new(1), external_auth: RwLock::new(None), auth_state_tx, } @@ -1266,7 +1266,7 @@ impl AuthManager { enable_codex_api_key_env: false, auth_credentials_store_mode: AuthCredentialsStoreMode::File, forced_chatgpt_workspace_id: RwLock::new(None), - refresh_lock: AsyncMutex::new(()), + refresh_lock: Semaphore::new(1), external_auth: RwLock::new(None), auth_state_tx, }) @@ -1285,7 +1285,7 @@ impl AuthManager { enable_codex_api_key_env: false, auth_credentials_store_mode: AuthCredentialsStoreMode::File, forced_chatgpt_workspace_id: RwLock::new(None), - refresh_lock: AsyncMutex::new(()), + refresh_lock: Semaphore::new(1), external_auth: RwLock::new(None), auth_state_tx, }) @@ -1302,7 +1302,7 @@ impl AuthManager { enable_codex_api_key_env: false, auth_credentials_store_mode: AuthCredentialsStoreMode::File, forced_chatgpt_workspace_id: RwLock::new(None), - refresh_lock: AsyncMutex::new(()), + refresh_lock: Semaphore::new(1), external_auth: RwLock::new(Some( Arc::new(BearerTokenRefresher::new(config)) as Arc )), @@ -1572,7 +1572,12 @@ impl AuthManager { /// can assume that some other instance already refreshed it. If the persisted /// token is the same as the cached, then ask the token authority to refresh. pub async fn refresh_token(&self) -> Result<(), RefreshTokenError> { - let _refresh_guard = self.refresh_lock.lock().await; + let _refresh_guard = self.refresh_lock.acquire().await.map_err(|_| { + RefreshTokenError::Permanent(RefreshTokenFailedError::new( + RefreshTokenFailedReason::Other, + REFRESH_TOKEN_UNKNOWN_MESSAGE.to_string(), + )) + })?; let auth_before_reload = self.auth_cached(); if auth_before_reload .as_ref() @@ -1604,7 +1609,12 @@ impl AuthManager { /// observe refreshed token. If the token refresh fails, returns the error to /// the caller. pub async fn refresh_token_from_authority(&self) -> Result<(), RefreshTokenError> { - let _refresh_guard = self.refresh_lock.lock().await; + let _refresh_guard = self.refresh_lock.acquire().await.map_err(|_| { + RefreshTokenError::Permanent(RefreshTokenFailedError::new( + RefreshTokenFailedReason::Other, + REFRESH_TOKEN_UNKNOWN_MESSAGE.to_string(), + )) + })?; self.refresh_token_from_authority_impl().await } diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 979a7ad217..5adfae9347 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -283,6 +283,10 @@ impl OAuthPersistor { /// Persists the latest stored credentials if they have changed. /// Deletes the credentials if they are no longer present. + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] pub(crate) async fn persist_if_needed(&self) -> Result<()> { let (client_id, maybe_credentials) = { let manager = self.inner.authorization_manager.clone(); @@ -335,6 +339,10 @@ impl OAuthPersistor { Ok(()) } + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] pub(crate) async fn refresh_if_needed(&self) -> Result<()> { let expires_at = { let guard = self.inner.last_credentials.lock().await; diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 6bdc5dd29f..e23497ecd6 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -62,6 +62,7 @@ use serde_json::Value; use sse_stream::Sse; use sse_stream::SseStream; use tokio::sync::Mutex; +use tokio::sync::Semaphore; use tokio::sync::watch; use tokio::time; use tracing::warn; @@ -494,7 +495,7 @@ pub struct RmcpClient { state: Mutex, transport_recipe: TransportRecipe, initialize_context: Mutex>, - session_recovery_lock: Mutex<()>, + session_recovery_lock: Semaphore, elicitation_pause_state: ElicitationPauseState, } @@ -521,7 +522,7 @@ impl RmcpClient { }), transport_recipe, initialize_context: Mutex::new(None), - session_recovery_lock: Mutex::new(()), + session_recovery_lock: Semaphore::new(1), elicitation_pause_state: ElicitationPauseState::new(), }) } @@ -550,7 +551,7 @@ impl RmcpClient { }), transport_recipe, initialize_context: Mutex::new(None), - session_recovery_lock: Mutex::new(()), + session_recovery_lock: Semaphore::new(1), elicitation_pause_state: ElicitationPauseState::new(), }) } @@ -1097,7 +1098,11 @@ impl RmcpClient { &self, failed_service: &Arc>, ) -> Result<()> { - let _recovery_guard = self.session_recovery_lock.lock().await; + let _recovery_guard = self + .session_recovery_lock + .acquire() + .await + .map_err(|_| anyhow!("MCP client recovery semaphore closed"))?; { let guard = self.state.lock().await; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 98aa08c598..ec56c11874 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -4772,7 +4772,8 @@ impl App { app_server, cwd, PluginReadParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name, }, ); @@ -6502,7 +6503,6 @@ async fn fetch_plugins_list( request_id, params: PluginListParams { cwds: Some(vec![cwd]), - force_remote_sync: false, }, }) .await @@ -6540,9 +6540,9 @@ async fn fetch_plugin_install( .request_typed(ClientRequest::PluginInstall { request_id, params: PluginInstallParams { - marketplace_path, + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, plugin_name, - force_remote_sync: false, }, }) .await @@ -6557,10 +6557,7 @@ async fn fetch_plugin_uninstall( request_handle .request_typed(ClientRequest::PluginUninstall { request_id, - params: PluginUninstallParams { - plugin_id, - force_remote_sync: false, - }, + params: PluginUninstallParams { plugin_id }, }) .await .wrap_err("plugin/uninstall failed in TUI") @@ -6759,19 +6756,18 @@ mod tests { marketplaces: vec![ PluginMarketplaceEntry { name: "openai-bundled".to_string(), - path: test_absolute_path("/marketplaces/openai-bundled"), + path: Some(test_absolute_path("/marketplaces/openai-bundled")), interface: None, plugins: Vec::new(), }, PluginMarketplaceEntry { name: "openai-curated".to_string(), - path: test_absolute_path("/marketplaces/openai-curated"), + path: Some(test_absolute_path("/marketplaces/openai-curated")), interface: None, plugins: Vec::new(), }, ], marketplace_load_errors: Vec::new(), - remote_sync_error: None, featured_plugin_ids: Vec::new(), }; @@ -6781,7 +6777,7 @@ mod tests { response.marketplaces, vec![PluginMarketplaceEntry { name: "openai-curated".to_string(), - path: test_absolute_path("/marketplaces/openai-curated"), + path: Some(test_absolute_path("/marketplaces/openai-curated")), interface: None, plugins: Vec::new(), }] diff --git a/codex-rs/tui/src/chatwidget/plugins.rs b/codex-rs/tui/src/chatwidget/plugins.rs index 964a986314..bf5a930e2d 100644 --- a/codex-rs/tui/src/chatwidget/plugins.rs +++ b/codex-rs/tui/src/chatwidget/plugins.rs @@ -5,6 +5,7 @@ use std::time::Instant; use super::ChatWidget; use crate::app_event::AppEvent; use crate::bottom_pane::ColumnWidthMode; +use crate::bottom_pane::SelectionAction; use crate::bottom_pane::SelectionItem; use crate::bottom_pane::SelectionRowDisplay; use crate::bottom_pane::SelectionTab; @@ -760,7 +761,6 @@ impl ChatWidget { header: plugins_header( "Browse plugins from available marketplaces.".to_string(), format!("Installed {installed} of {total} available plugins."), - response.remote_sync_error.as_deref(), ), items: self.plugin_selection_items( all_entries, @@ -776,7 +776,6 @@ impl ChatWidget { header: plugins_header( "Installed plugins.".to_string(), format!("Showing {installed} installed plugins."), - response.remote_sync_error.as_deref(), ), items: self.plugin_selection_items( installed_entries, @@ -804,7 +803,6 @@ impl ChatWidget { header: plugins_header( "OpenAI Curated marketplace.".to_string(), format!("Installed {curated_installed} of {curated_total} OpenAI Curated plugins."), - response.remote_sync_error.as_deref(), ), items: self.plugin_selection_items( curated_entries, @@ -848,7 +846,6 @@ impl ChatWidget { format!( "Installed {marketplace_installed} of {marketplace_total} {label} plugins." ), - response.remote_sync_error.as_deref(), ), items: self.plugin_selection_items( entries, @@ -1047,24 +1044,35 @@ impl ChatWidget { let plugin_display_name = display_name.clone(); let marketplace_path = marketplace.path.clone(); let plugin_name = plugin.name.clone(); - - items.push(SelectionItem { - name: display_name, - description: Some(description), - selected_description: Some(selected_description), - search_value: Some(search_value), - actions: vec![Box::new(move |tx| { + let is_disabled = marketplace_path.is_none(); + let actions: Vec = if let Some(marketplace_path) = marketplace_path { + vec![Box::new(move |tx| { tx.send(AppEvent::OpenPluginDetailLoading { plugin_display_name: plugin_display_name.clone(), }); tx.send(AppEvent::FetchPluginDetail { cwd: cwd.clone(), params: codex_app_server_protocol::PluginReadParams { - marketplace_path: marketplace_path.clone(), + marketplace_path: Some(marketplace_path.clone()), + remote_marketplace_name: None, plugin_name: plugin_name.clone(), }, }); - })], + })] + } else { + Vec::new() + }; + let disabled_reason = + is_disabled.then(|| "remote plugin details are not available yet".to_string()); + + items.push(SelectionItem { + name: display_name, + description: Some(description), + selected_description: Some(selected_description), + search_value: Some(search_value), + actions, + is_disabled, + disabled_reason, ..Default::default() }); } @@ -1089,20 +1097,11 @@ fn plugin_detail_hint_line() -> Line<'static> { Line::from("Press esc to close.") } -fn plugins_header( - subtitle: String, - count_line: String, - remote_sync_error: Option<&str>, -) -> Box { +fn plugins_header(subtitle: String, count_line: String) -> Box { let mut header = ColumnRenderable::new(); header.push(Line::from("Plugins".bold())); header.push(Line::from(subtitle.dim())); header.push(Line::from(count_line.dim())); - if let Some(remote_sync_error) = remote_sync_error { - header.push(Line::from( - format!("Using cached marketplace data: {remote_sync_error}").dim(), - )); - } Box::new(header) } @@ -1138,7 +1137,10 @@ fn sort_plugin_entries(entries: &mut [(&PluginMarketplaceEntry, &PluginSummary, } fn marketplace_tab_id(marketplace: &PluginMarketplaceEntry) -> String { - format!("marketplace:{}", marketplace.path.display()) + match marketplace.path.as_ref() { + Some(path) => format!("marketplace:{}", path.display()), + None => format!("marketplace:{}", marketplace.name), + } } fn disambiguate_duplicate_tab_labels(labels: Vec) -> Vec { diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugins_popup_curated_marketplace.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugins_popup_curated_marketplace.snap index e43251eb9d..ee8c81db3f 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugins_popup_curated_marketplace.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugins_popup_curated_marketplace.snap @@ -1,11 +1,10 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/popups_and_settings.rs expression: popup --- Plugins Browse plugins from available marketplaces. Installed 1 of 4 available plugins. - Using cached marketplace data: remote sync timed out [All Plugins] Installed (1) OpenAI Curated Repo Marketplace diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index 89de317f72..66df972ebf 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -859,8 +859,11 @@ pub(super) fn plugins_test_interface( default_prompt: None, brand_color: None, composer_icon: None, + composer_icon_url: None, logo: None, + logo_url: None, screenshots: Vec::new(), + screenshot_urls: Vec::new(), } } @@ -896,7 +899,7 @@ pub(super) fn plugins_test_curated_marketplace( ) -> PluginMarketplaceEntry { PluginMarketplaceEntry { name: OPENAI_CURATED_MARKETPLACE_NAME.to_string(), - path: plugins_test_absolute_path("marketplaces/chatgpt"), + path: Some(plugins_test_absolute_path("marketplaces/chatgpt")), interface: Some(MarketplaceInterface { display_name: Some("ChatGPT Marketplace".to_string()), }), @@ -907,7 +910,7 @@ pub(super) fn plugins_test_curated_marketplace( pub(super) fn plugins_test_repo_marketplace(plugins: Vec) -> PluginMarketplaceEntry { PluginMarketplaceEntry { name: "repo".to_string(), - path: plugins_test_absolute_path("marketplaces/repo"), + path: Some(plugins_test_absolute_path("marketplaces/repo")), interface: Some(MarketplaceInterface { display_name: Some("Repo Marketplace".to_string()), }), @@ -921,7 +924,6 @@ pub(super) fn plugins_test_response( PluginListResponse { marketplaces, marketplace_load_errors: Vec::new(), - remote_sync_error: None, featured_plugin_ids: Vec::new(), } } diff --git a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs index 859be3b94a..e55881d3f7 100644 --- a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs +++ b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs @@ -105,7 +105,7 @@ async fn plugins_popup_snapshot_shows_all_marketplaces_and_sorts_installed_then_ let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.set_feature_enabled(Feature::Plugins, /*enabled*/ true); - let mut response = plugins_test_response(vec![ + let response = plugins_test_response(vec![ plugins_test_curated_marketplace(vec![ plugins_test_summary( "plugin-bravo", @@ -145,8 +145,6 @@ async fn plugins_popup_snapshot_shows_all_marketplaces_and_sorts_installed_then_ PluginInstallPolicy::Available, )]), ]); - response.remote_sync_error = Some("remote sync timed out".to_string()); - let popup = render_loaded_plugins_popup(&mut chat, response); assert_chatwidget_snapshot!("plugins_popup_curated_marketplace", popup); assert!( @@ -544,7 +542,9 @@ async fn plugins_popup_refresh_preserves_duplicate_marketplace_tab_by_path() { let response = plugins_test_response(vec![ PluginMarketplaceEntry { name: "duplicate".to_string(), - path: plugins_test_absolute_path("marketplaces/home/marketplace.json"), + path: Some(plugins_test_absolute_path( + "marketplaces/home/marketplace.json", + )), interface: Some(MarketplaceInterface { display_name: Some("Duplicate Marketplace".to_string()), }), @@ -560,7 +560,9 @@ async fn plugins_popup_refresh_preserves_duplicate_marketplace_tab_by_path() { }, PluginMarketplaceEntry { name: "duplicate".to_string(), - path: plugins_test_absolute_path("marketplaces/repo/marketplace.json"), + path: Some(plugins_test_absolute_path( + "marketplaces/repo/marketplace.json", + )), interface: Some(MarketplaceInterface { display_name: Some("Duplicate Marketplace".to_string()), }), diff --git a/codex-rs/utils/image/src/lib.rs b/codex-rs/utils/image/src/lib.rs index 0c0a026e41..a3f595a183 100644 --- a/codex-rs/utils/image/src/lib.rs +++ b/codex-rs/utils/image/src/lib.rs @@ -15,10 +15,8 @@ use image::codecs::jpeg::JpegEncoder; use image::codecs::png::PngEncoder; use image::codecs::webp::WebPEncoder; use image::imageops::FilterType; -/// Maximum width used when resizing images before uploading. -pub const MAX_WIDTH: u32 = 2048; -/// Maximum height used when resizing images before uploading. -pub const MAX_HEIGHT: u32 = 768; +/// Maximum width or height used when resizing images before uploading. +pub const MAX_DIMENSION: u32 = 2048; pub mod error; @@ -80,40 +78,41 @@ pub fn load_for_prompt_bytes( let (width, height) = dynamic.dimensions(); - let encoded = - if mode == PromptImageMode::Original || (width <= MAX_WIDTH && height <= MAX_HEIGHT) { - if let Some(format) = format.filter(|format| can_preserve_source_bytes(*format)) { - let mime = format_to_mime(format); - EncodedImage { - bytes: file_bytes, - mime, - width, - height, - } - } else { - let (bytes, output_format) = encode_image(&dynamic, ImageFormat::Png)?; - let mime = format_to_mime(output_format); - EncodedImage { - bytes, - mime, - width, - height, - } + let encoded = if mode == PromptImageMode::Original + || (width <= MAX_DIMENSION && height <= MAX_DIMENSION) + { + if let Some(format) = format.filter(|format| can_preserve_source_bytes(*format)) { + let mime = format_to_mime(format); + EncodedImage { + bytes: file_bytes, + mime, + width, + height, } } else { - let resized = dynamic.resize(MAX_WIDTH, MAX_HEIGHT, FilterType::Triangle); - let target_format = format - .filter(|format| can_preserve_source_bytes(*format)) - .unwrap_or(ImageFormat::Png); - let (bytes, output_format) = encode_image(&resized, target_format)?; + let (bytes, output_format) = encode_image(&dynamic, ImageFormat::Png)?; let mime = format_to_mime(output_format); EncodedImage { bytes, mime, - width: resized.width(), - height: resized.height(), + width, + height, } - }; + } + } else { + let resized = dynamic.resize(MAX_DIMENSION, MAX_DIMENSION, FilterType::Triangle); + let target_format = format + .filter(|format| can_preserve_source_bytes(*format)) + .unwrap_or(ImageFormat::Png); + let (bytes, output_format) = encode_image(&resized, target_format)?; + let mime = format_to_mime(output_format); + EncodedImage { + bytes, + mime, + width: resized.width(), + height: resized.height(), + } + }; Ok(encoded) }) @@ -251,8 +250,8 @@ mod tests { ) .expect("process image"); - assert!(processed.width <= MAX_WIDTH); - assert!(processed.height <= MAX_HEIGHT); + assert!(processed.width <= MAX_DIMENSION); + assert!(processed.height <= MAX_DIMENSION); assert_eq!(processed.mime, mime); let detected_format = @@ -265,6 +264,23 @@ mod tests { } } + #[tokio::test(flavor = "multi_thread")] + async fn downscales_tall_image_to_fit_square_bounds() { + let image = ImageBuffer::from_pixel(1024, 4096, Rgba([200u8, 10, 10, 255])); + let original_bytes = image_bytes(&image, ImageFormat::Png); + + let processed = load_for_prompt_bytes( + Path::new("in-memory-image"), + original_bytes, + PromptImageMode::ResizeToFit, + ) + .expect("process image"); + + assert_eq!(processed.width, 512); + assert_eq!(processed.height, MAX_DIMENSION); + assert_eq!(processed.mime, "image/png"); + } + #[tokio::test(flavor = "multi_thread")] async fn preserves_large_image_in_original_mode() { let image = ImageBuffer::from_pixel(4096, 2048, Rgba([180u8, 30, 30, 255]));