diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index cd2a7e99f9..fe0dc6f6a4 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2180,6 +2180,7 @@ dependencies = [ "url", "uuid", "wiremock", + "zip", ] [[package]] @@ -3463,6 +3464,7 @@ dependencies = [ "tracing", "tracing-subscriber", "wiremock", + "zip", ] [[package]] diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 4e9211b36a..00349def2e 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1197,6 +1197,59 @@ ], "type": "string" }, + "FeedbackSaveParams": { + "properties": { + "classification": { + "type": "string" + }, + "extraLogFiles": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + }, + "includeLogs": { + "type": "boolean" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "savePath": { + "allOf": [ + { + "$ref": "#/definitions/LegacyAppPathString" + } + ], + "description": "Absolute, previously unused ZIP path on the app-server host. The caller owns this file, including after a disconnected RPC." + }, + "tags": { + "additionalProperties": { + "type": "string" + }, + "type": [ + "object", + "null" + ] + }, + "threadId": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "classification", + "savePath" + ], + "type": "object" + }, "FeedbackUploadParams": { "properties": { "classification": { @@ -8434,6 +8487,30 @@ "title": "Feedback/uploadRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/RequestId" + }, + "method": { + "enum": [ + "feedback/save" + ], + "title": "Feedback/saveRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/FeedbackSaveParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Feedback/saveRequest", + "type": "object" + }, { "description": "Execute a standalone command (argv vector) under the server's sandbox.", "properties": { 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 93814141e5..fa5b3fe344 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 @@ -2394,6 +2394,30 @@ "title": "Feedback/uploadRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/v2/RequestId" + }, + "method": { + "enum": [ + "feedback/save" + ], + "title": "Feedback/saveRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/v2/FeedbackSaveParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Feedback/saveRequest", + "type": "object" + }, { "description": "Execute a standalone command (argv vector) under the server's sandbox.", "properties": { @@ -11807,6 +11831,85 @@ }, "type": "object" }, + "FeedbackSaveParams": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "classification": { + "type": "string" + }, + "extraLogFiles": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + }, + "includeLogs": { + "type": "boolean" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "savePath": { + "allOf": [ + { + "$ref": "#/definitions/v2/LegacyAppPathString" + } + ], + "description": "Absolute, previously unused ZIP path on the app-server host. The caller owns this file, including after a disconnected RPC." + }, + "tags": { + "additionalProperties": { + "type": "string" + }, + "type": [ + "object", + "null" + ] + }, + "threadId": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "classification", + "savePath" + ], + "title": "FeedbackSaveParams", + "type": "object" + }, + "FeedbackSaveResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "promptHash": { + "description": "Same prompt hash as feedback/upload; null when prompt metadata is absent.", + "type": [ + "string", + "null" + ] + }, + "savedPath": { + "$ref": "#/definitions/v2/LegacyAppPathString" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "savedPath", + "threadId" + ], + "title": "FeedbackSaveResponse", + "type": "object" + }, "FeedbackUploadParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { 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 cf6063c636..3f8c022d57 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 @@ -3967,6 +3967,30 @@ "title": "Feedback/uploadRequest", "type": "object" }, + { + "properties": { + "id": { + "$ref": "#/definitions/RequestId" + }, + "method": { + "enum": [ + "feedback/save" + ], + "title": "Feedback/saveRequestMethod", + "type": "string" + }, + "params": { + "$ref": "#/definitions/FeedbackSaveParams" + } + }, + "required": [ + "id", + "method", + "params" + ], + "title": "Feedback/saveRequest", + "type": "object" + }, { "description": "Execute a standalone command (argv vector) under the server's sandbox.", "properties": { @@ -7623,6 +7647,85 @@ }, "type": "object" }, + "FeedbackSaveParams": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "classification": { + "type": "string" + }, + "extraLogFiles": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + }, + "includeLogs": { + "type": "boolean" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "savePath": { + "allOf": [ + { + "$ref": "#/definitions/LegacyAppPathString" + } + ], + "description": "Absolute, previously unused ZIP path on the app-server host. The caller owns this file, including after a disconnected RPC." + }, + "tags": { + "additionalProperties": { + "type": "string" + }, + "type": [ + "object", + "null" + ] + }, + "threadId": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "classification", + "savePath" + ], + "title": "FeedbackSaveParams", + "type": "object" + }, + "FeedbackSaveResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "promptHash": { + "description": "Same prompt hash as feedback/upload; null when prompt metadata is absent.", + "type": [ + "string", + "null" + ] + }, + "savedPath": { + "$ref": "#/definitions/LegacyAppPathString" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "savedPath", + "threadId" + ], + "title": "FeedbackSaveResponse", + "type": "object" + }, "FeedbackUploadParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveParams.json b/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveParams.json new file mode 100644 index 0000000000..62132f7bd7 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveParams.json @@ -0,0 +1,60 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "LegacyAppPathString": { + "type": "string" + } + }, + "properties": { + "classification": { + "type": "string" + }, + "extraLogFiles": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + }, + "includeLogs": { + "type": "boolean" + }, + "reason": { + "type": [ + "string", + "null" + ] + }, + "savePath": { + "allOf": [ + { + "$ref": "#/definitions/LegacyAppPathString" + } + ], + "description": "Absolute, previously unused ZIP path on the app-server host. The caller owns this file, including after a disconnected RPC." + }, + "tags": { + "additionalProperties": { + "type": "string" + }, + "type": [ + "object", + "null" + ] + }, + "threadId": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "classification", + "savePath" + ], + "title": "FeedbackSaveParams", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveResponse.json b/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveResponse.json new file mode 100644 index 0000000000..29c8c44a02 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/json/v2/FeedbackSaveResponse.json @@ -0,0 +1,29 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "LegacyAppPathString": { + "type": "string" + } + }, + "properties": { + "promptHash": { + "description": "Same prompt hash as feedback/upload; null when prompt metadata is absent.", + "type": [ + "string", + "null" + ] + }, + "savedPath": { + "$ref": "#/definitions/LegacyAppPathString" + }, + "threadId": { + "type": "string" + } + }, + "required": [ + "savedPath", + "threadId" + ], + "title": "FeedbackSaveResponse", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst index 83c3f9612c..568cc4015d 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 9986b8e99d..0f969e8adc 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts b/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts index 73a495e240..8402762305 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts @@ -24,6 +24,7 @@ import type { ExperimentalFeatureListParams } from "./v2/ExperimentalFeatureList import type { ExternalAgentConfigDetectParams } from "./v2/ExternalAgentConfigDetectParams"; import type { ExternalAgentConfigImportHistoryRecordParams } from "./v2/ExternalAgentConfigImportHistoryRecordParams"; import type { ExternalAgentConfigImportParams } from "./v2/ExternalAgentConfigImportParams"; +import type { FeedbackSaveParams } from "./v2/FeedbackSaveParams"; import type { FeedbackUploadParams } from "./v2/FeedbackUploadParams"; import type { FsCopyParams } from "./v2/FsCopyParams"; import type { FsCreateDirectoryParams } from "./v2/FsCreateDirectoryParams"; @@ -105,4 +106,4 @@ import type { WindowsSandboxSetupStartParams } from "./v2/WindowsSandboxSetupSta /** * Request from the client to the server. */ -export type ClientRequest ={ "method": "initialize", id: RequestId, params: InitializeParams, } | { "method": "thread/start", id: RequestId, params: ThreadStartParams, } | { "method": "thread/resume", id: RequestId, params: ThreadResumeParams, } | { "method": "thread/fork", id: RequestId, params: ThreadForkParams, } | { "method": "thread/archive", id: RequestId, params: ThreadArchiveParams, } | { "method": "thread/delete", id: RequestId, params: ThreadDeleteParams, } | { "method": "thread/unsubscribe", id: RequestId, params: ThreadUnsubscribeParams, } | { "method": "thread/name/set", id: RequestId, params: ThreadSetNameParams, } | { "method": "thread/goal/set", id: RequestId, params: ThreadGoalSetParams, } | { "method": "thread/goal/get", id: RequestId, params: ThreadGoalGetParams, } | { "method": "thread/goal/clear", id: RequestId, params: ThreadGoalClearParams, } | { "method": "thread/metadata/update", id: RequestId, params: ThreadMetadataUpdateParams, } | { "method": "thread/attachment/add", id: RequestId, params: ThreadAttachmentAddParams, } | { "method": "thread/attachment/list", id: RequestId, params: ThreadAttachmentListParams, } | { "method": "thread/attachmentOwner/list", id: RequestId, params: ThreadAttachmentOwnerListParams, } | { "method": "thread/attachment/remove", id: RequestId, params: ThreadAttachmentRemoveParams, } | { "method": "thread/section/move", id: RequestId, params: ThreadSectionMoveParams, } | { "method": "thread/unarchive", id: RequestId, params: ThreadUnarchiveParams, } | { "method": "thread/compact/start", id: RequestId, params: ThreadCompactStartParams, } | { "method": "thread/shellCommand", id: RequestId, params: ThreadShellCommandParams, } | { "method": "thread/approveGuardianDeniedAction", id: RequestId, params: ThreadApproveGuardianDeniedActionParams, } | { "method": "thread/revert", id: RequestId, params: ThreadRevertParams, } | { "method": "thread/list", id: RequestId, params: ThreadListParams, } | { "method": "threadSection/list", id: RequestId, params: ThreadSectionListParams, } | { "method": "threadSection/create", id: RequestId, params: ThreadSectionCreateParams, } | { "method": "threadSection/update", id: RequestId, params: ThreadSectionUpdateParams, } | { "method": "threadSection/delete", id: RequestId, params: ThreadSectionDeleteParams, } | { "method": "thread/loaded/list", id: RequestId, params: ThreadLoadedListParams, } | { "method": "thread/read", id: RequestId, params: ThreadReadParams, } | { "method": "thread/turns/list", id: RequestId, params: ThreadTurnsListParams, } | { "method": "thread/items/list", id: RequestId, params: ThreadItemsListParams, } | { "method": "thread/inject_items", id: RequestId, params: ThreadInjectItemsParams, } | { "method": "skills/list", id: RequestId, params: SkillsListParams, } | { "method": "skills/extraRoots/set", id: RequestId, params: SkillsExtraRootsSetParams, } | { "method": "hooks/list", id: RequestId, params: HooksListParams, } | { "method": "marketplace/add", id: RequestId, params: MarketplaceAddParams, } | { "method": "marketplace/remove", id: RequestId, params: MarketplaceRemoveParams, } | { "method": "marketplace/upgrade", id: RequestId, params: MarketplaceUpgradeParams, } | { "method": "plugin/list", id: RequestId, params: PluginListParams, } | { "method": "plugin/installed", id: RequestId, params: PluginInstalledParams, } | { "method": "plugin/reconcile", id: RequestId, params: PluginReconcileParams, } | { "method": "plugin/read", id: RequestId, params: PluginReadParams, } | { "method": "plugin/skill/read", id: RequestId, params: PluginSkillReadParams, } | { "method": "plugin/share/save", id: RequestId, params: PluginShareSaveParams, } | { "method": "plugin/share/updateTargets", id: RequestId, params: PluginShareUpdateTargetsParams, } | { "method": "plugin/share/list", id: RequestId, params: PluginShareListParams, } | { "method": "plugin/share/checkout", id: RequestId, params: PluginShareCheckoutParams, } | { "method": "plugin/share/delete", id: RequestId, params: PluginShareDeleteParams, } | { "method": "app/read", id: RequestId, params: AppsReadParams, } | { "method": "app/list", id: RequestId, params: AppsListParams, } | { "method": "app/installed", id: RequestId, params: AppsInstalledParams, } | { "method": "fs/readFile", id: RequestId, params: FsReadFileParams, } | { "method": "fs/writeFile", id: RequestId, params: FsWriteFileParams, } | { "method": "fs/createDirectory", id: RequestId, params: FsCreateDirectoryParams, } | { "method": "fs/getMetadata", id: RequestId, params: FsGetMetadataParams, } | { "method": "fs/readDirectory", id: RequestId, params: FsReadDirectoryParams, } | { "method": "fs/remove", id: RequestId, params: FsRemoveParams, } | { "method": "fs/copy", id: RequestId, params: FsCopyParams, } | { "method": "fs/watch", id: RequestId, params: FsWatchParams, } | { "method": "fs/unwatch", id: RequestId, params: FsUnwatchParams, } | { "method": "skills/config/write", id: RequestId, params: SkillsConfigWriteParams, } | { "method": "plugin/install", id: RequestId, params: PluginInstallParams, } | { "method": "plugin/uninstall", id: RequestId, params: PluginUninstallParams, } | { "method": "turn/start", id: RequestId, params: TurnStartParams, } | { "method": "turn/steer", id: RequestId, params: TurnSteerParams, } | { "method": "turn/interrupt", id: RequestId, params: TurnInterruptParams, } | { "method": "review/start", id: RequestId, params: ReviewStartParams, } | { "method": "model/list", id: RequestId, params: ModelListParams, } | { "method": "account/gatewayOAuth/read", id: RequestId, params: undefined, } | { "method": "account/gatewayOAuth/login", id: RequestId, params: undefined, } | { "method": "account/gatewayOAuth/cancel", id: RequestId, params: undefined, } | { "method": "modelProvider/capabilities/read", id: RequestId, params: ModelProviderCapabilitiesReadParams, } | { "method": "experimentalFeature/list", id: RequestId, params: ExperimentalFeatureListParams, } | { "method": "permissionProfile/list", id: RequestId, params: PermissionProfileListParams, } | { "method": "experimentalFeature/enablement/set", id: RequestId, params: ExperimentalFeatureEnablementSetParams, } | { "method": "mcpServer/oauth/login", id: RequestId, params: McpServerOauthLoginParams, } | { "method": "config/mcpServer/reload", id: RequestId, params: undefined, } | { "method": "mcpServerStatus/list", id: RequestId, params: ListMcpServerStatusParams, } | { "method": "mcpServer/resource/read", id: RequestId, params: McpResourceReadParams, } | { "method": "mcpServer/tool/call", id: RequestId, params: McpServerToolCallParams, } | { "method": "windowsSandbox/setupStart", id: RequestId, params: WindowsSandboxSetupStartParams, } | { "method": "windowsSandbox/readiness", id: RequestId, params: undefined, } | { "method": "account/login/start", id: RequestId, params: LoginAccountParams, } | { "method": "account/login/cancel", id: RequestId, params: CancelLoginAccountParams, } | { "method": "account/logout", id: RequestId, params: undefined, } | { "method": "account/rateLimits/read", id: RequestId, params?: GetAccountRateLimitsParams | undefined, } | { "method": "account/rateLimitResetCredit/consume", id: RequestId, params: ConsumeAccountRateLimitResetCreditParams, } | { "method": "account/usage/read", id: RequestId, params?: GetAccountTokenUsageParams | undefined, } | { "method": "account/workspaceMessages/read", id: RequestId, params: undefined, } | { "method": "account/sendAddCreditsNudgeEmail", id: RequestId, params: SendAddCreditsNudgeEmailParams, } | { "method": "feedback/upload", id: RequestId, params: FeedbackUploadParams, } | { "method": "command/exec", id: RequestId, params: CommandExecParams, } | { "method": "command/exec/write", id: RequestId, params: CommandExecWriteParams, } | { "method": "command/exec/terminate", id: RequestId, params: CommandExecTerminateParams, } | { "method": "command/exec/resize", id: RequestId, params: CommandExecResizeParams, } | { "method": "config/read", id: RequestId, params: ConfigReadParams, } | { "method": "externalAgentConfig/detect", id: RequestId, params: ExternalAgentConfigDetectParams, } | { "method": "externalAgentConfig/import", id: RequestId, params: ExternalAgentConfigImportParams, } | { "method": "externalAgentConfig/import/recordHistory", id: RequestId, params: ExternalAgentConfigImportHistoryRecordParams, } | { "method": "externalAgentConfig/import/readHistories", id: RequestId, params: undefined, } | { "method": "config/value/write", id: RequestId, params: ConfigValueWriteParams, } | { "method": "config/batchWrite", id: RequestId, params: ConfigBatchWriteParams, } | { "method": "configRequirements/read", id: RequestId, params: undefined, } | { "method": "account/read", id: RequestId, params: GetAccountParams, } | { "method": "getConversationSummary", id: RequestId, params: GetConversationSummaryParams, } | { "method": "gitDiffToRemote", id: RequestId, params: GitDiffToRemoteParams, } | { "method": "getAuthStatus", id: RequestId, params: GetAuthStatusParams, } | { "method": "fuzzyFileSearch", id: RequestId, params: FuzzyFileSearchParams, }; +export type ClientRequest ={ "method": "initialize", id: RequestId, params: InitializeParams, } | { "method": "thread/start", id: RequestId, params: ThreadStartParams, } | { "method": "thread/resume", id: RequestId, params: ThreadResumeParams, } | { "method": "thread/fork", id: RequestId, params: ThreadForkParams, } | { "method": "thread/archive", id: RequestId, params: ThreadArchiveParams, } | { "method": "thread/delete", id: RequestId, params: ThreadDeleteParams, } | { "method": "thread/unsubscribe", id: RequestId, params: ThreadUnsubscribeParams, } | { "method": "thread/name/set", id: RequestId, params: ThreadSetNameParams, } | { "method": "thread/goal/set", id: RequestId, params: ThreadGoalSetParams, } | { "method": "thread/goal/get", id: RequestId, params: ThreadGoalGetParams, } | { "method": "thread/goal/clear", id: RequestId, params: ThreadGoalClearParams, } | { "method": "thread/metadata/update", id: RequestId, params: ThreadMetadataUpdateParams, } | { "method": "thread/attachment/add", id: RequestId, params: ThreadAttachmentAddParams, } | { "method": "thread/attachment/list", id: RequestId, params: ThreadAttachmentListParams, } | { "method": "thread/attachmentOwner/list", id: RequestId, params: ThreadAttachmentOwnerListParams, } | { "method": "thread/attachment/remove", id: RequestId, params: ThreadAttachmentRemoveParams, } | { "method": "thread/section/move", id: RequestId, params: ThreadSectionMoveParams, } | { "method": "thread/unarchive", id: RequestId, params: ThreadUnarchiveParams, } | { "method": "thread/compact/start", id: RequestId, params: ThreadCompactStartParams, } | { "method": "thread/shellCommand", id: RequestId, params: ThreadShellCommandParams, } | { "method": "thread/approveGuardianDeniedAction", id: RequestId, params: ThreadApproveGuardianDeniedActionParams, } | { "method": "thread/revert", id: RequestId, params: ThreadRevertParams, } | { "method": "thread/list", id: RequestId, params: ThreadListParams, } | { "method": "threadSection/list", id: RequestId, params: ThreadSectionListParams, } | { "method": "threadSection/create", id: RequestId, params: ThreadSectionCreateParams, } | { "method": "threadSection/update", id: RequestId, params: ThreadSectionUpdateParams, } | { "method": "threadSection/delete", id: RequestId, params: ThreadSectionDeleteParams, } | { "method": "thread/loaded/list", id: RequestId, params: ThreadLoadedListParams, } | { "method": "thread/read", id: RequestId, params: ThreadReadParams, } | { "method": "thread/turns/list", id: RequestId, params: ThreadTurnsListParams, } | { "method": "thread/items/list", id: RequestId, params: ThreadItemsListParams, } | { "method": "thread/inject_items", id: RequestId, params: ThreadInjectItemsParams, } | { "method": "skills/list", id: RequestId, params: SkillsListParams, } | { "method": "skills/extraRoots/set", id: RequestId, params: SkillsExtraRootsSetParams, } | { "method": "hooks/list", id: RequestId, params: HooksListParams, } | { "method": "marketplace/add", id: RequestId, params: MarketplaceAddParams, } | { "method": "marketplace/remove", id: RequestId, params: MarketplaceRemoveParams, } | { "method": "marketplace/upgrade", id: RequestId, params: MarketplaceUpgradeParams, } | { "method": "plugin/list", id: RequestId, params: PluginListParams, } | { "method": "plugin/installed", id: RequestId, params: PluginInstalledParams, } | { "method": "plugin/reconcile", id: RequestId, params: PluginReconcileParams, } | { "method": "plugin/read", id: RequestId, params: PluginReadParams, } | { "method": "plugin/skill/read", id: RequestId, params: PluginSkillReadParams, } | { "method": "plugin/share/save", id: RequestId, params: PluginShareSaveParams, } | { "method": "plugin/share/updateTargets", id: RequestId, params: PluginShareUpdateTargetsParams, } | { "method": "plugin/share/list", id: RequestId, params: PluginShareListParams, } | { "method": "plugin/share/checkout", id: RequestId, params: PluginShareCheckoutParams, } | { "method": "plugin/share/delete", id: RequestId, params: PluginShareDeleteParams, } | { "method": "app/read", id: RequestId, params: AppsReadParams, } | { "method": "app/list", id: RequestId, params: AppsListParams, } | { "method": "app/installed", id: RequestId, params: AppsInstalledParams, } | { "method": "fs/readFile", id: RequestId, params: FsReadFileParams, } | { "method": "fs/writeFile", id: RequestId, params: FsWriteFileParams, } | { "method": "fs/createDirectory", id: RequestId, params: FsCreateDirectoryParams, } | { "method": "fs/getMetadata", id: RequestId, params: FsGetMetadataParams, } | { "method": "fs/readDirectory", id: RequestId, params: FsReadDirectoryParams, } | { "method": "fs/remove", id: RequestId, params: FsRemoveParams, } | { "method": "fs/copy", id: RequestId, params: FsCopyParams, } | { "method": "fs/watch", id: RequestId, params: FsWatchParams, } | { "method": "fs/unwatch", id: RequestId, params: FsUnwatchParams, } | { "method": "skills/config/write", id: RequestId, params: SkillsConfigWriteParams, } | { "method": "plugin/install", id: RequestId, params: PluginInstallParams, } | { "method": "plugin/uninstall", id: RequestId, params: PluginUninstallParams, } | { "method": "turn/start", id: RequestId, params: TurnStartParams, } | { "method": "turn/steer", id: RequestId, params: TurnSteerParams, } | { "method": "turn/interrupt", id: RequestId, params: TurnInterruptParams, } | { "method": "review/start", id: RequestId, params: ReviewStartParams, } | { "method": "model/list", id: RequestId, params: ModelListParams, } | { "method": "account/gatewayOAuth/read", id: RequestId, params: undefined, } | { "method": "account/gatewayOAuth/login", id: RequestId, params: undefined, } | { "method": "account/gatewayOAuth/cancel", id: RequestId, params: undefined, } | { "method": "modelProvider/capabilities/read", id: RequestId, params: ModelProviderCapabilitiesReadParams, } | { "method": "experimentalFeature/list", id: RequestId, params: ExperimentalFeatureListParams, } | { "method": "permissionProfile/list", id: RequestId, params: PermissionProfileListParams, } | { "method": "experimentalFeature/enablement/set", id: RequestId, params: ExperimentalFeatureEnablementSetParams, } | { "method": "mcpServer/oauth/login", id: RequestId, params: McpServerOauthLoginParams, } | { "method": "config/mcpServer/reload", id: RequestId, params: undefined, } | { "method": "mcpServerStatus/list", id: RequestId, params: ListMcpServerStatusParams, } | { "method": "mcpServer/resource/read", id: RequestId, params: McpResourceReadParams, } | { "method": "mcpServer/tool/call", id: RequestId, params: McpServerToolCallParams, } | { "method": "windowsSandbox/setupStart", id: RequestId, params: WindowsSandboxSetupStartParams, } | { "method": "windowsSandbox/readiness", id: RequestId, params: undefined, } | { "method": "account/login/start", id: RequestId, params: LoginAccountParams, } | { "method": "account/login/cancel", id: RequestId, params: CancelLoginAccountParams, } | { "method": "account/logout", id: RequestId, params: undefined, } | { "method": "account/rateLimits/read", id: RequestId, params?: GetAccountRateLimitsParams | undefined, } | { "method": "account/rateLimitResetCredit/consume", id: RequestId, params: ConsumeAccountRateLimitResetCreditParams, } | { "method": "account/usage/read", id: RequestId, params?: GetAccountTokenUsageParams | undefined, } | { "method": "account/workspaceMessages/read", id: RequestId, params: undefined, } | { "method": "account/sendAddCreditsNudgeEmail", id: RequestId, params: SendAddCreditsNudgeEmailParams, } | { "method": "feedback/upload", id: RequestId, params: FeedbackUploadParams, } | { "method": "feedback/save", id: RequestId, params: FeedbackSaveParams, } | { "method": "command/exec", id: RequestId, params: CommandExecParams, } | { "method": "command/exec/write", id: RequestId, params: CommandExecWriteParams, } | { "method": "command/exec/terminate", id: RequestId, params: CommandExecTerminateParams, } | { "method": "command/exec/resize", id: RequestId, params: CommandExecResizeParams, } | { "method": "config/read", id: RequestId, params: ConfigReadParams, } | { "method": "externalAgentConfig/detect", id: RequestId, params: ExternalAgentConfigDetectParams, } | { "method": "externalAgentConfig/import", id: RequestId, params: ExternalAgentConfigImportParams, } | { "method": "externalAgentConfig/import/recordHistory", id: RequestId, params: ExternalAgentConfigImportHistoryRecordParams, } | { "method": "externalAgentConfig/import/readHistories", id: RequestId, params: undefined, } | { "method": "config/value/write", id: RequestId, params: ConfigValueWriteParams, } | { "method": "config/batchWrite", id: RequestId, params: ConfigBatchWriteParams, } | { "method": "configRequirements/read", id: RequestId, params: undefined, } | { "method": "account/read", id: RequestId, params: GetAccountParams, } | { "method": "getConversationSummary", id: RequestId, params: GetConversationSummaryParams, } | { "method": "gitDiffToRemote", id: RequestId, params: GitDiffToRemoteParams, } | { "method": "getAuthStatus", id: RequestId, params: GetAuthStatusParams, } | { "method": "fuzzyFileSearch", id: RequestId, params: FuzzyFileSearchParams, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveParams.ts new file mode 100644 index 0000000000..d78033f6d7 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveParams.ts @@ -0,0 +1,11 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { LegacyAppPathString } from "../LegacyAppPathString"; + +export type FeedbackSaveParams = { +/** + * Absolute, previously unused ZIP path on the app-server host. + * The caller owns this file, including after a disconnected RPC. + */ +savePath: LegacyAppPathString, classification: string, reason?: string | null, threadId?: string | null, includeLogs?: boolean, extraLogFiles?: Array | null, tags?: { [key in string]?: string } | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveResponse.ts new file mode 100644 index 0000000000..d26f3988ae --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FeedbackSaveResponse.ts @@ -0,0 +1,10 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { LegacyAppPathString } from "../LegacyAppPathString"; + +export type FeedbackSaveResponse = { savedPath: LegacyAppPathString, threadId: string, +/** + * Same prompt hash as feedback/upload; null when prompt metadata is absent. + */ +promptHash: string | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index 4072345343..52fead6173 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -163,6 +163,8 @@ export type { ExternalAgentDetectedConnectorSource } from "./ExternalAgentDetect export type { ExternalAgentImportedConnectorCandidate } from "./ExternalAgentImportedConnectorCandidate"; export type { ExternalAgentImportedConnectorSource } from "./ExternalAgentImportedConnectorSource"; export type { FeedbackRequirements } from "./FeedbackRequirements"; +export type { FeedbackSaveParams } from "./FeedbackSaveParams"; +export type { FeedbackSaveResponse } from "./FeedbackSaveResponse"; export type { FeedbackUploadParams } from "./FeedbackUploadParams"; export type { FeedbackUploadResponse } from "./FeedbackUploadResponse"; export type { FileChangeApprovalDecision } from "./FileChangeApprovalDecision"; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 541af12881..b183d6a2e5 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1387,6 +1387,12 @@ client_request_definitions! { response: v2::FeedbackUploadResponse, }, + FeedbackSave => "feedback/save" { + params: v2::FeedbackSaveParams, + serialization: None, + response: v2::FeedbackSaveResponse, + }, + /// Execute a standalone command (argv vector) under the server's sandbox. OneOffCommandExec => "command/exec" { params: v2::CommandExecParams, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/feedback.rs b/codex-rs/app-server-protocol/src/protocol/v2/feedback.rs index c055fc0358..f617d33303 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/feedback.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/feedback.rs @@ -1,5 +1,6 @@ use crate::JsonSchema; use crate::TS; +use codex_utils_path_uri::LegacyAppPathString; use serde::Deserialize; use serde::Serialize; use std::collections::BTreeMap; @@ -33,3 +34,24 @@ pub struct FeedbackUploadResponse { #[serde(default)] pub prompt_hash: Option, } + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct FeedbackSaveParams { + #[serde(flatten)] + pub feedback: FeedbackUploadParams, + /// Absolute, previously unused ZIP path on the app-server host. + /// The caller owns this file, including after a disconnected RPC. + pub save_path: LegacyAppPathString, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct FeedbackSaveResponse { + pub saved_path: LegacyAppPathString, + pub thread_id: String, + /// Same prompt hash as feedback/upload; null when prompt metadata is absent. + pub prompt_hash: Option, +} diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 8c44989d64..234636839d 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -5453,3 +5453,32 @@ fn tool_request_user_input_params_default_legacy_missing_is_blocking_to_true() { } ); } + +/// Native path spelling survives serialization so the receiving host can validate it. +#[test] +fn feedback_paths_preserve_native_wire_spelling_before_host_validation() { + for path in [ + "/tmp/feedback.zip", + r"C:\Users\test\feedback.zip", + r"\\server\share\feedback.zip", + "relative.zip", + ] { + let request: FeedbackSaveParams = serde_json::from_value(json!({ + "classification": "bug", "savePath": path + })) + .expect("wire paths remain uninterpreted"); + assert_eq!(request.save_path.as_str(), path); + assert_eq!( + serde_json::to_value(&request).expect("serialize request")["savePath"], + path + ); + let response: FeedbackSaveResponse = serde_json::from_value(json!({ + "threadId": "synthetic", "savedPath": path, "promptHash": null + })) + .expect("wire response"); + assert_eq!( + serde_json::to_value(response).expect("serialize response")["savedPath"], + path + ); + } +} diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index d0cf6a4e0c..bddfc75a0d 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -132,6 +132,7 @@ libc = { workspace = true } codex-windows-sandbox = { workspace = true } [dev-dependencies] +zip = { workspace = true } codex-uds = { workspace = true } app_test_support = { workspace = true } axum = { workspace = true, default-features = false, features = [ diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 147ef0a0ac..e1e5a7a948 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1924,6 +1924,9 @@ impl MessageProcessor { ClientRequest::FeedbackUpload { params, .. } => { self.feedback_processor.feedback_upload(params).await } + ClientRequest::FeedbackSave { params, .. } => { + self.feedback_processor.feedback_save(params).await + } }; match result { diff --git a/codex-rs/app-server/src/request_processors/feedback_processor.rs b/codex-rs/app-server/src/request_processors/feedback_processor.rs index 1cc789a18c..0e33d9a9cc 100644 --- a/codex-rs/app-server/src/request_processors/feedback_processor.rs +++ b/codex-rs/app-server/src/request_processors/feedback_processor.rs @@ -1,6 +1,8 @@ use super::feedback_thread_index::FeedbackThreadIndex; use super::*; use crate::error_code::OVERLOADED_ERROR_CODE; +use codex_app_server_protocol::FeedbackSaveParams; +use codex_app_server_protocol::FeedbackSaveResponse; use codex_connectors::ConnectorDirectoryCacheContext; use codex_connectors::ConnectorDirectoryCacheKey; use codex_connectors::connector_runtime_cache_path; @@ -11,8 +13,11 @@ use codex_feedback::FeedbackSnapshot; use codex_feedback::WINDOWS_SANDBOX_LOG_ATTACHMENT_FILENAME; use codex_feedback::guardian_review_failures; use codex_rollout::RolloutRecorder; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::LegacyAppPathString; use sha2::Digest; use sha2::Sha256; +use std::path::Path; use tokio::sync::Semaphore; #[derive(Clone)] @@ -50,20 +55,46 @@ impl FeedbackRequestProcessor { &self, params: FeedbackUploadParams, ) -> Result, JSONRPCErrorError> { - self.upload_feedback_response(params) - .await - .map(|response| Some(response.into())) - } - - async fn upload_feedback_response( - &self, - params: FeedbackUploadParams, - ) -> Result { if !self.config.feedback_enabled { return Err(invalid_request( "sending feedback is disabled by configuration", )); } + self.feedback_response(params, /*save_path*/ None) + .await + .map(|response| Some(response.into())) + } + + pub(crate) async fn feedback_save( + &self, + params: FeedbackSaveParams, + ) -> Result, JSONRPCErrorError> { + if !Path::new(params.save_path.as_str()).is_absolute() { + return Err(invalid_request( + "feedback savePath must be an absolute host path", + )); + } + let save_path = AbsolutePathBuf::try_from(params.save_path) + .map_err(|error| invalid_request(format!("invalid feedback savePath: {error}")))?; + let saved_path = LegacyAppPathString::from_abs_path(&save_path); + let response = self + .feedback_response(params.feedback, Some(save_path)) + .await?; + Ok(Some( + FeedbackSaveResponse { + saved_path, + thread_id: response.thread_id, + prompt_hash: response.prompt_hash, + } + .into(), + )) + } + + async fn feedback_response( + &self, + params: FeedbackUploadParams, + save_path: Option, + ) -> Result { let permit = self .uploads .clone() @@ -98,8 +129,8 @@ impl FeedbackRequestProcessor { let auth = self.auth_manager.auth_cached(); let turn_metadata = if let Some(conversation_id) = conversation_id && let Some(rollout_path) = self - .resolve_rollout_path(conversation_id, self.state_db.as_ref()) - .await + .resolve_rollout_path(conversation_id, self.state_db.as_ref(), save_path.as_ref()) + .await? { feedback_turn_metadata_from_rollout( &rollout_path, @@ -183,8 +214,12 @@ impl FeedbackRequestProcessor { .flat_map(|index| &mut index.threads) { if let Some(rollout_path) = self - .resolve_rollout_path(thread.thread_id, state_db_ctx.as_ref()) - .await + .resolve_rollout_path( + thread.thread_id, + state_db_ctx.as_ref(), + save_path.as_ref(), + ) + .await? && seen_attachment_paths.insert(rollout_path.clone()) { thread.rollout_filename = codex_rollout::plain_rollout_path(&rollout_path) @@ -208,7 +243,10 @@ impl FeedbackRequestProcessor { }); } } - if let Some(index) = feedback_index { + if let Some(mut index) = feedback_index { + if save_path.is_some() { + index.rollout_archive_filename = None; + } match index.attachment() { Ok(attachment) => extra_attachments.insert(0, attachment), Err(err) => warn!("failed to serialize feedback thread index: {err}"), @@ -271,6 +309,11 @@ impl FeedbackRequestProcessor { let runtime_handle = tokio::runtime::Handle::current(); let codex_home = self.config.codex_home.clone(); + let operation = if save_path.is_some() { + "save" + } else { + "upload" + }; let upload_result = tokio::task::spawn_blocking(move || { // Cancelling the RPC waiter must not release a still-running upload's slot. let _permit = permit; @@ -279,19 +322,22 @@ impl FeedbackRequestProcessor { .extend(codex_feedback::daemon_log_attachments(codex_home.as_path())); } let tags = (!upload_tags.is_empty()).then_some(&upload_tags); - runtime_handle.block_on(snapshot.upload_feedback( - FeedbackUploadOptions { - classification: &classification, - reason: reason.as_deref(), - tags, - include_logs, - extra_attachments, - extra_attachment_paths: &attachment_paths, - session_source: Some(session_source), - logs_override: sqlite_feedback_logs, - }, - &http_client_factory, - )) + let options = FeedbackUploadOptions { + classification: &classification, + reason: reason.as_deref(), + tags, + include_logs, + extra_attachments, + extra_attachment_paths: &attachment_paths, + session_source: Some(session_source), + logs_override: sqlite_feedback_logs, + }; + match save_path { + Some(path) => snapshot.save_feedback(options, path.as_path()), + None => { + runtime_handle.block_on(snapshot.upload_feedback(options, &http_client_factory)) + } + } }) .await; @@ -299,13 +345,13 @@ impl FeedbackRequestProcessor { Ok(result) => result, Err(join_err) => { return Err(internal_error(format!( - "failed to upload feedback: {join_err}" + "failed to {operation} feedback: {join_err}" ))); } }; upload_result - .map_err(|err| internal_error(format!("failed to upload feedback: {err:#}")))?; + .map_err(|err| internal_error(format!("failed to {operation} feedback: {err:#}")))?; Ok(FeedbackUploadResponse { thread_id, prompt_hash, @@ -316,21 +362,31 @@ impl FeedbackRequestProcessor { &self, conversation_id: ThreadId, state_db_ctx: Option<&StateDbHandle>, - ) -> Option { - if let Ok(conversation) = self.thread_manager.get_thread(conversation_id).await - && let Some(rollout_path) = conversation.rollout_path() - { - return Some(rollout_path); + save_path: Option<&AbsolutePathBuf>, + ) -> Result, JSONRPCErrorError> { + if let Ok(conversation) = self.thread_manager.get_thread(conversation_id).await { + if save_path.is_some() { + // Explicit extraLogFiles are collected separately and remain strict, even + // when they name this same not-yet-materialized automatic rollout. + return conversation.feedback_rollout_path().await.map_err(|error| { + internal_error(format!("failed to select feedback rollout: {error}")) + }); + } + if let Some(rollout_path) = conversation.rollout_path() { + return Ok(Some(rollout_path)); + } } - let state_db_ctx = state_db_ctx?; - state_db_ctx + let Some(state_db_ctx) = state_db_ctx else { + return Ok(None); + }; + Ok(state_db_ctx .find_rollout_path_by_id(conversation_id, /*archived_only*/ None) .await .unwrap_or_else(|err| { warn!("failed to resolve rollout path for thread_id={conversation_id}: {err}"); None - }) + })) } } diff --git a/codex-rs/app-server/src/request_processors/feedback_thread_index.rs b/codex-rs/app-server/src/request_processors/feedback_thread_index.rs index cb38f4f456..d3b09c2726 100644 --- a/codex-rs/app-server/src/request_processors/feedback_thread_index.rs +++ b/codex-rs/app-server/src/request_processors/feedback_thread_index.rs @@ -14,7 +14,7 @@ const MAX_LISTED_OMISSIONS: usize = 64; #[derive(Serialize)] pub(super) struct FeedbackThreadIndex { - rollout_archive_filename: &'static str, + pub(super) rollout_archive_filename: Option<&'static str>, pub threads: Vec, retained_failure_thread_ids: Vec, omitted_thread_ids: Vec, @@ -58,7 +58,7 @@ impl FeedbackThreadIndex { .take(MAX_LISTED_OMISSIONS) .collect::>(); Self { - rollout_archive_filename: "rollouts.tar.gz", + rollout_archive_filename: Some("rollouts.tar.gz"), threads: thread_ids .into_iter() .map(|thread_id| FeedbackThread { diff --git a/codex-rs/app-server/tests/suite/v2/feedback.rs b/codex-rs/app-server/tests/suite/v2/feedback.rs index 05cea49fe7..afe5d57ed3 100644 --- a/codex-rs/app-server/tests/suite/v2/feedback.rs +++ b/codex-rs/app-server/tests/suite/v2/feedback.rs @@ -352,3 +352,262 @@ async fn feedback_upload_recovers_persisted_evidence_and_reports_sqlite_failures pool.close().await; Ok(()) } + +/// Disabling uploads still permits local ZIPs, with invalid paths and collisions leaving existing files intact. +#[tokio::test] +async fn feedback_save_is_local_and_never_overwrites() -> Result<()> { + let proxy = TcpListener::bind("127.0.0.1:0").await?; + let proxy_uri = format!("http://{}", proxy.local_addr()?); + let mut app_server = TestAppServer::builder() + .with_args(&["-c", "feedback.enabled=false"]) + .with_env_overrides(&[ + ("HTTPS_PROXY", Some(proxy_uri.as_str())), + ("https_proxy", Some(proxy_uri.as_str())), + ("NO_PROXY", Some("")), + ("no_proxy", Some("")), + ]) + .build_initialized() + .await?; + let request = app_server + .send_raw_request("feedback/upload", Some(json!({ "classification": "bug" }))) + .await?; + let error = app_server + .read_stream_until_error_message(RequestId::Integer(request)) + .await?; + assert_eq!( + error.error.message, + "sending feedback is disabled by configuration" + ); + + let directory = tempfile::tempdir()?; + let saved = directory.path().join("local.zip"); + std::fs::write(&saved, b"existing export")?; + let foreign = if cfg!(windows) { + "/tmp/foreign.zip" + } else { + r"C:\Users\test\foreign.zip" + }; + for (path, message) in [ + (json!("relative.zip"), "absolute host path"), + (json!("~/feedback.zip"), "absolute host path"), + (json!(foreign), "absolute host path"), + (json!(&saved), "already exists"), + ( + json!(directory.path().join("missing/feedback.zip")), + "failed to save feedback", + ), + ] { + let request = app_server + .send_raw_request( + "feedback/save", + Some(json!({ + "classification": "bug", "savePath": path, + })), + ) + .await?; + let error = app_server + .read_stream_until_error_message(RequestId::Integer(request)) + .await?; + assert!(error.error.message.contains(message), "{error:?}"); + } + assert_eq!(std::fs::read(&saved)?, b"existing export"); + assert_eq!(std::fs::read_dir(directory.path())?.count(), 1); + std::fs::remove_file(&saved)?; + let request = app_server + .send_raw_request( + "feedback/save", + Some(json!({ + "classification": "bug", "savePath": saved, + })), + ) + .await?; + let response = app_server + .read_stream_until_response_message(RequestId::Integer(request)) + .await?; + assert_eq!(response.result["savedPath"], json!(saved)); + let mut archive = zip::ZipArchive::new(std::fs::File::open(&saved)?)?; + let report: serde_json::Value = serde_json::from_reader(archive.by_name("report.json")?)?; + assert_eq!(report["tags"]["classification"], "bug"); + assert!( + timeout(Duration::from_millis(100), proxy.accept()) + .await + .is_err() + ); + Ok(()) +} + +#[tokio::test] +async fn feedback_save_preserves_selected_rollout_or_fails_without_upload() -> Result<()> { + use std::io::Read; + + let proxy = TcpListener::bind("127.0.0.1:0").await?; + let proxy_uri = format!("http://{}", proxy.local_addr()?); + let mut app_server = TestAppServer::builder() + .with_env_overrides(&[ + ("HTTPS_PROXY", Some(proxy_uri.as_str())), + ("https_proxy", Some(proxy_uri.as_str())), + ("NO_PROXY", Some("")), + ("no_proxy", Some("")), + ]) + .build_initialized() + .await?; + let directory = tempfile::tempdir()?; + let filename = format!( + "rollout-2026-10-19T12-00-00-{}.jsonl", + codex_protocol::ThreadId::new() + ); + let rollout = directory.path().join(&filename); + let bytes = b"{\"synthetic\":true}\n"; + std::fs::write(&rollout, bytes)?; + let saved = directory.path().join("selected.zip"); + let request = app_server.send_raw_request("feedback/save", Some(json!({ + "classification": "bug", "includeLogs": true, "extraLogFiles": [rollout], "savePath": saved, + }))).await?; + let response = app_server + .read_stream_until_response_message(RequestId::Integer(request)) + .await?; + assert_eq!(response.result["savedPath"], json!(saved)); + let mut zip = zip::ZipArchive::new(std::fs::File::open(&saved)?)?; + let entry = zip + .file_names() + .collect::>>()? + .into_iter() + .find(|name| name.ends_with(&format!("/{filename}"))) + .expect("selected rollout") + .to_string(); + let mut actual = Vec::new(); + zip.by_name(&entry)?.read_to_end(&mut actual)?; + assert_eq!(actual, bytes); + drop(zip); + std::fs::remove_file(&rollout)?; + let missing = directory.path().join("missing.zip"); + let request = app_server.send_raw_request("feedback/save", Some(json!({ + "classification": "bug", "includeLogs": true, "extraLogFiles": [rollout], "savePath": missing, + }))).await?; + let error = app_server + .read_stream_until_error_message(RequestId::Integer(request)) + .await?; + assert_eq!(error.error.code, -32603); + assert!(error.error.message.starts_with("failed to save feedback:")); + assert!(error.error.message.contains("selected attachment")); + assert!(!missing.exists()); + assert_eq!(std::fs::read_dir(directory.path())?.count(), 1); + assert!( + timeout(Duration::from_millis(100), proxy.accept()) + .await + .is_err() + ); + Ok(()) +} + +/// Automatic untouched rollouts are omitted; explicit missing and deleted persisted files fail. +#[tokio::test] +async fn feedback_save_handles_rollout_materialization() -> Result<()> { + use codex_app_server_protocol::ThreadStartParams; + use std::io::Read; + let server = app_test_support::create_mock_responses_server_repeating_assistant("Done").await; + let home = tempfile::tempdir()?; + app_test_support::MockResponsesConfig::new(&server.uri()).write(home.path())?; + let proxy = TcpListener::bind("127.0.0.1:0").await?; + let proxy_uri = format!("http://{}", proxy.local_addr()?); + let mut app = TestAppServer::builder() + .with_codex_home(home.path()) + .with_args(&["-c", "feedback.enabled=false"]) + .with_env_overrides(&[ + ("HTTPS_PROXY", Some(proxy_uri.as_str())), + ("https_proxy", Some(proxy_uri.as_str())), + ("NO_PROXY", Some("127.0.0.1,localhost")), + ("no_proxy", Some("127.0.0.1,localhost")), + ]) + .build_initialized() + .await?; + let request = app + .send_thread_start_request_with_auto_env(ThreadStartParams::default()) + .await?; + let response = app + .read_stream_until_response_message(RequestId::Integer(request)) + .await?; + let thread = response.result["thread"]["id"].as_str().unwrap(); + let rollout = std::path::PathBuf::from(response.result["thread"]["path"].as_str().unwrap()); + assert!(!rollout.exists()); + let output = tempfile::tempdir()?; + for explicit in [false, true] { + let destination = output.path().join(format!("fresh-{explicit}.zip")); + let request = app.send_raw_request("feedback/save", Some(json!({"classification":"bug", "includeLogs":true, "threadId":thread, "extraLogFiles": if explicit { vec![&rollout] } else { vec![] }, "savePath":destination}))).await?; + if explicit { + let error = app + .read_stream_until_error_message(RequestId::Integer(request)) + .await?; + assert!(error.error.message.contains("selected attachment")); + assert!(!destination.exists()); + } else { + app.read_stream_until_response_message(RequestId::Integer(request)) + .await?; + let mut zip = zip::ZipArchive::new(std::fs::File::open(destination)?)?; + let names = zip + .file_names() + .collect::>>()? + .into_iter() + .map(std::borrow::Cow::into_owned) + .collect::>(); + assert!( + !names + .iter() + .any(|name| name.ends_with(".jsonl") || name.ends_with("rollouts.tar.gz")) + ); + let index_name = names + .iter() + .find(|name| name.ends_with("feedback-thread-index.json")) + .expect("thread index"); + let index: serde_json::Value = serde_json::from_reader(zip.by_name(index_name)?)?; + assert_eq!( + index["threads"][0]["rollout_filename"], + serde_json::Value::Null + ); + assert_eq!(index["rollout_archive_filename"], serde_json::Value::Null); + } + } + assert!(!rollout.exists(), "saving must not materialize the rollout"); + let request = app + .send_raw_request( + "turn/start", + Some(json!({"threadId":thread,"input":[{"type":"text","text":"hello"}]})), + ) + .await?; + app.read_stream_until_response_message(RequestId::Integer(request)) + .await?; + app.read_stream_until_notification_message("turn/completed") + .await?; + assert!(rollout.exists()); + let expected = std::fs::read(&rollout)?; + let saved = output.path().join("materialized.zip"); + let request = app.send_raw_request("feedback/save", Some(json!({"classification":"bug", "includeLogs":true, "threadId":thread, "savePath":saved}))).await?; + app.read_stream_until_response_message(RequestId::Integer(request)) + .await?; + let mut zip = zip::ZipArchive::new(std::fs::File::open(saved)?)?; + let suffix = format!("/{}", rollout.file_name().unwrap().to_string_lossy()); + let entry = zip + .file_names() + .collect::>>()? + .into_iter() + .find(|name| name.ends_with(&suffix)) + .unwrap() + .into_owned(); + let mut actual = Vec::new(); + zip.by_name(&entry)?.read_to_end(&mut actual)?; + assert_eq!(actual, expected); + std::fs::remove_file(&rollout)?; + let failed = output.path().join("deleted.zip"); + let request = app.send_raw_request("feedback/save", Some(json!({"classification":"bug", "includeLogs":true, "threadId":thread, "savePath":failed}))).await?; + let error = app + .read_stream_until_error_message(RequestId::Integer(request)) + .await?; + assert!(error.error.message.contains("selected attachment")); + assert!(!failed.exists()); + assert!( + timeout(Duration::from_millis(100), proxy.accept()) + .await + .is_err() + ); + Ok(()) +} diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 5c0ebe0e3c..a42d709777 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -254,7 +254,7 @@ pub struct AnalyticsConfigToml { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct FeedbackConfigToml { - /// When `false`, disables the feedback flow across Codex product surfaces. + /// When `false`, disables feedback uploads and background Guardian review retention. Explicit local exports remain available. pub enabled: Option, } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index bc6d091f69..9f35ee5c90 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1756,7 +1756,7 @@ "additionalProperties": false, "properties": { "enabled": { - "description": "When `false`, disables the feedback flow across Codex product surfaces.", + "description": "When `false`, disables feedback uploads and background Guardian review retention. Explicit local exports remain available.", "type": "boolean" } }, diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index e016026865..024eb76132 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -881,6 +881,18 @@ impl CodexThread { self.rollout_path.clone() } + /// Selects a local feedback rollout without materializing an untouched thread. + pub async fn feedback_rollout_path(&self) -> ThreadStoreResult> { + match self.session.live_thread() { + Some(thread) => thread.feedback_rollout_path().await.or_else(|error| { + // Shutdown can close or remove the writer before its thread leaves the manager. + // Retain the captured input; the archive reader still rejects missing files. + self.rollout_path().map(Some).ok_or(error) + }), + None => Ok(None), + } + } + /// Returns startup metadata without the one-time initial message replay. pub fn startup_metadata(&self) -> &ThreadStartupMetadata { &self.startup_metadata diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 31e7dc2c39..92787f11b4 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1182,7 +1182,7 @@ pub struct Config { /// Voluntarily left as Optional because the default value might depend on the client. pub analytics_enabled: Option, - /// When `false`, disables feedback collection across Codex product surfaces. + /// When `false`, disables feedback uploads and background Guardian review retention. Explicit local exports remain available. /// Defaults to `true`. pub feedback_enabled: bool, diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index a0ef8c25b1..c18040e798 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -2088,13 +2088,15 @@ async fn interrupted_guardian_review_across_model_change_does_not_execute_the_co #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tracing_test::traced_test] -#[test_case(None, false; "legacy_fallback")] -#[test_case(None, true; "otel_enabled")] -#[test_case(Some("Acting model rejection instructions."), false; "catalog_override")] -#[test_case(Some(""), false; "empty_override")] +#[test_case(None, false, true; "legacy_fallback")] +#[test_case(None, true, true; "otel_enabled")] +#[test_case(Some("Acting model rejection instructions."), false, true; "catalog_override")] +#[test_case(Some(""), false, true; "empty_override")] +#[test_case(None, false, false; "feedback_disabled")] async fn guardian_denial_rejects_tool_call_with_rationale( rejection_instructions: Option<&'static str>, log_assessments: bool, + feedback_enabled: bool, ) -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); @@ -2142,6 +2144,7 @@ async fn guardian_denial_rejects_tool_call_with_rationale( }); }) .with_config(move |config| { + config.feedback_enabled = feedback_enabled; config.otel.log_guardian_assessments = log_assessments; config.otel.exporter = OtelExporterKind::OtlpGrpc { endpoint: "http://127.0.0.1:1".to_string(), @@ -2241,80 +2244,98 @@ async fn guardian_denial_rejects_tool_call_with_rationale( ) .await?; let durable = state_db.list_guardian_review_records().await?; - assert_eq!( - durable.len(), - 1, - "the Guardian capture must persist, not just fall back to memory" - ); - let feedback = codex_feedback::guardian_review_failures( - Some(&state_db), - &[test.session_configured.thread_id], - ) - .await - .attachment - .expect("failed Guardian review"); - let record: serde_json::Value = serde_json::from_slice(&feedback.buffer)?; - assert_eq!( - record, - serde_json::from_slice::(&durable[0].record)? - ); - assert_eq!(record["instructions"], guardian_request.instructions_text()); - assert!( - guardian_request.body_contains_text(record["action"].as_str().expect("reviewed action")) - ); - let recorded_history: Vec = serde_json::from_value(record["history"].clone())?; - let inspection_request = requests - .iter() - .find(|request| { - request + if feedback_enabled { + assert_eq!( + durable.len(), + 1, + "the Guardian capture must persist, not just fall back to memory" + ); + let feedback = codex_feedback::guardian_review_failures( + Some(&state_db), + &[test.session_configured.thread_id], + ) + .await + .attachment + .expect("failed Guardian review"); + let record: serde_json::Value = serde_json::from_slice(&feedback.buffer)?; + assert_eq!( + record, + serde_json::from_slice::(&durable[0].record)? + ); + assert_eq!(record["instructions"], guardian_request.instructions_text()); + assert!( + guardian_request + .body_contains_text(record["action"].as_str().expect("reviewed action")) + ); + let recorded_history: Vec = + serde_json::from_value(record["history"].clone())?; + let inspection_request = requests + .iter() + .find(|request| { + request + .function_call_output_text("exec-guardian-inspect") + .is_some() + }) + .expect("Guardian request following its inspection tool"); + assert!( + inspection_request .function_call_output_text("exec-guardian-inspect") - .is_some() - }) - .expect("Guardian request following its inspection tool"); - assert!( - inspection_request - .function_call_output_text("exec-guardian-inspect") - .expect("inspection output") - .contains("guardian-inspection-evidence") - ); - let inspection_output = inspection_request - .input() - .into_iter() - .find(|item| { - item["type"] == "function_call_output" && item["call_id"] == "exec-guardian-inspect" - }) - .expect("inspection response item"); - assert!(recorded_history.contains(&serde_json::from_value(inspection_output)?)); - assert!(recorded_history.iter().any(|item| { - matches!(item, ResponseItem::Message { role, content, .. } - if role == "assistant" && content.iter().any(|part| { - matches!(part, ContentItem::OutputText { text , .. } - if Some(text.as_str()) == record["decision"].as_str()) - })) - })); - assert_eq!( - json!({ - "reviewed_thread_id": record["reviewed_thread_id"], - "reviewer_thread_id": record["reviewer_thread_id"], - "status": record["status"], - "decision": serde_json::from_str::( - record["decision"].as_str().expect("raw Guardian decision"), - )?, - "context_omitted": record["context_omitted"], - }), - json!({ - "reviewed_thread_id": test.session_configured.thread_id, - "reviewer_thread_id": guardian_request.body_json()["client_metadata"]["thread_id"], - "status": "denied", - "decision": { - "risk_level": "high", - "user_authorization": "low", - "outcome": "deny", - "rationale": "The requested write has unacceptable test risk.", - }, - "context_omitted": false, - }) - ); + .expect("inspection output") + .contains("guardian-inspection-evidence") + ); + let inspection_output = inspection_request + .input() + .into_iter() + .find(|item| { + item["type"] == "function_call_output" && item["call_id"] == "exec-guardian-inspect" + }) + .expect("inspection response item"); + assert!(recorded_history.contains(&serde_json::from_value(inspection_output)?)); + assert!(recorded_history.iter().any(|item| { + matches!(item, ResponseItem::Message { role, content, .. } + if role == "assistant" && content.iter().any(|part| { + matches!(part, ContentItem::OutputText { text , .. } + if Some(text.as_str()) == record["decision"].as_str()) + })) + })); + assert_eq!( + json!({ + "reviewed_thread_id": record["reviewed_thread_id"], + "reviewer_thread_id": record["reviewer_thread_id"], + "status": record["status"], + "decision": serde_json::from_str::( + record["decision"].as_str().expect("raw Guardian decision"), + )?, + "context_omitted": record["context_omitted"], + }), + json!({ + "reviewed_thread_id": test.session_configured.thread_id, + "reviewer_thread_id": guardian_request.body_json()["client_metadata"]["thread_id"], + "status": "denied", + "decision": { + "risk_level": "high", + "user_authorization": "low", + "outcome": "deny", + "rationale": "The requested write has unacceptable test risk.", + }, + "context_omitted": false, + }) + ); + } else { + assert!( + durable.is_empty(), + "disabled feedback must not retain Guardian evidence" + ); + assert!( + codex_feedback::guardian_review_failures( + Some(&state_db), + &[test.session_configured.thread_id] + ) + .await + .attachment + .is_none() + ); + } let tool_output = requests .iter() diff --git a/codex-rs/core/tests/suite/managed_threads_tests.rs b/codex-rs/core/tests/suite/managed_threads_tests.rs index e116aad03d..288ffce8c4 100644 --- a/codex-rs/core/tests/suite/managed_threads_tests.rs +++ b/codex-rs/core/tests/suite/managed_threads_tests.rs @@ -443,3 +443,26 @@ async fn startup_tool_policy_controls_additional_permission_parameters( fixture.codex.shutdown_and_wait().await?; Ok(()) } + +#[tokio::test] +async fn feedback_rollout_selection_survives_writer_shutdown() -> anyhow::Result<()> { + let server = responses::start_mock_server().await; + let fixture = test_codex().build_with_auto_env(&server).await?; + let thread_id = fixture.session_configured.thread_id; + assert_eq!(fixture.codex.feedback_rollout_path().await?, None); + fixture + .thread_store + .persist_thread(thread_id, codex_thread_store::PersistContext::Standard) + .await?; + let path = fixture.codex.rollout_path().expect("local rollout"); + fixture.codex.shutdown_and_wait().await?; + let expected = std::fs::read(&path)?; + assert_eq!( + fixture.codex.feedback_rollout_path().await?, + Some(path.clone()) + ); + assert_eq!(std::fs::read(&path)?, expected); + std::fs::remove_file(&path)?; + assert_eq!(fixture.codex.feedback_rollout_path().await?, Some(path)); + Ok(()) +} diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index ee56043b1c..80641bdd29 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -26,6 +26,7 @@ tempfile = { workspace = true } tokio = { workspace = true, features = ["time"] } tracing = { workspace = true } tracing-subscriber = { workspace = true } +zip = { workspace = true } [dev-dependencies] log = { workspace = true } diff --git a/codex-rs/feedback/src/archive.rs b/codex-rs/feedback/src/archive.rs new file mode 100644 index 0000000000..82a7e22f59 --- /dev/null +++ b/codex-rs/feedback/src/archive.rs @@ -0,0 +1,127 @@ +//! Atomic local feedback export with bounded contents and no file replacement. + +use std::io; +use std::io::Write; +use std::path::Path; + +use crate::FeedbackAttachment; +use crate::FeedbackAttachmentPath; +use anyhow::Context; +use anyhow::Result; + +pub(crate) enum Input<'a> { + Owned(FeedbackAttachment), + Path(&'a FeedbackAttachmentPath), +} +use zip::ZipWriter; +use zip::write::SimpleFileOptions; + +pub(crate) struct Limits { + pub max_bytes: usize, + pub max_attachments: usize, +} + +impl Default for Limits { + fn default() -> Self { + Self { + max_bytes: 200 * 1024 * 1024, + max_attachments: 128, + } + } +} + +// Count uncompressed entry contents, including report and manifest serialization. +struct BudgetWriter<'a, W> { + inner: &'a mut W, + remaining: &'a mut usize, +} + +impl Write for BudgetWriter<'_, W> { + fn write(&mut self, buffer: &[u8]) -> io::Result { + let remaining = self.remaining.checked_sub(buffer.len()).ok_or_else(|| { + io::Error::other("feedback archive exceeds the uncompressed size limit") + })?; + self.inner.write_all(buffer)?; + *self.remaining = remaining; + Ok(buffer.len()) + } + + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +pub(crate) fn save<'a>( + path: &Path, + report: &[u8], + attachments: impl Iterator>, + limits: Limits, +) -> Result<()> { + anyhow::ensure!(path.is_absolute(), "feedback save path must be absolute"); + anyhow::ensure!(!path.exists(), "feedback save path already exists"); + let parent = path.parent().context("feedback save path has no parent")?; + let mut temporary = tempfile::NamedTempFile::new_in(parent)?; + let mut writer = ZipWriter::new(temporary.as_file_mut()); + let options = SimpleFileOptions::default() + .compression_method(zip::CompressionMethod::Deflated) + .large_file(true); + let mut remaining = limits.max_bytes; + writer.start_file("report.json", options)?; + BudgetWriter { + inner: &mut writer, + remaining: &mut remaining, + } + .write_all(report)?; + let mut manifest = Vec::new(); + for (index, attachment) in attachments.enumerate() { + // Inspect metadata before reading the next file or compressing its contents. + anyhow::ensure!( + index < limits.max_attachments, + "feedback archive exceeds the attachment count limit" + ); + let attachment = match attachment { + Input::Owned(attachment) => attachment, + Input::Path(path) => path.read_attachment(remaining) + .with_context(|| format!("failed to read selected attachment {}", path.path.display()))? + .with_context(|| format!("selected attachment {} is missing, nonregular, or exceeds the remaining decoded size limit", path.path.display()))?, + }; + let basename = attachment + .filename + .rsplit(['/', '\\']) + .next() + .filter(|name| !name.is_empty() && *name != "." && *name != "..") + .unwrap_or("attachment"); + // Numbered directories retain duplicate names without unsafe ZIP paths. + let entry = format!("attachments/{index:04}/{basename}"); + writer.start_file(&entry, options)?; + BudgetWriter { + inner: &mut writer, + remaining: &mut remaining, + } + .write_all(&attachment.buffer)?; + manifest.push(serde_json::json!({ + "entry": entry, + "filename": attachment.filename, + "contentType": attachment.content_type, + })); + } + writer.start_file("manifest.json", options)?; + serde_json::to_writer( + BudgetWriter { + inner: &mut writer, + remaining: &mut remaining, + }, + &manifest, + )?; + writer.finish()?; + temporary.as_file().sync_all()?; + // Same-directory publication is atomic and must not replace an existing file. + temporary + .persist_noclobber(path) + .map_err(|error| error.error)?; + Ok(()) +} + +#[cfg(test)] +#[path = "archive_tests.rs"] +mod tests; diff --git a/codex-rs/feedback/src/archive_tests.rs b/codex-rs/feedback/src/archive_tests.rs new file mode 100644 index 0000000000..b789cdb3ed --- /dev/null +++ b/codex-rs/feedback/src/archive_tests.rs @@ -0,0 +1,175 @@ +use super::*; +use crate::CodexFeedback; +use crate::FeedbackUploadOptions; +use pretty_assertions::assert_eq; +use std::fs::File; +use std::io::Read; +use zip::ZipArchive; + +fn options() -> FeedbackUploadOptions<'static> { + FeedbackUploadOptions { + classification: "bug", + reason: Some("synthetic feedback"), + tags: None, + include_logs: false, + extra_attachments: Vec::new(), + extra_attachment_paths: &[], + session_source: None, + logs_override: Some(b"consented log bytes".to_vec()), + } +} + +/// Exports preserve attachment bytes and duplicate names without unsafe ZIP paths. +#[test] +fn archive_preserves_selected_bytes_and_safe_names() -> Result<()> { + let directory = tempfile::tempdir()?; + let path = directory.path().join("feedback.zip"); + let expected = [ + ("../../same.log", b"first".as_slice()), + ("../../same.log", b"second"), + ("rollouts.tar.gz", b"compressed rollout bytes"), + ]; + super::save( + &path, + b"{}", + expected.iter().map(|(filename, buffer)| { + Input::Owned(FeedbackAttachment { + filename: filename.to_string(), + buffer: buffer.to_vec(), + content_type: None, + }) + }), + Limits::default(), + )?; + let mut archive = ZipArchive::new(File::open(&path)?)?; + assert_eq!(archive.len(), 5); + let manifest: serde_json::Value = serde_json::from_reader(archive.by_name("manifest.json")?)?; + for (index, (filename, bytes)) in expected.iter().enumerate() { + let basename = filename.rsplit('/').next().unwrap(); + let mut entry = archive.by_name(&format!("attachments/{index:04}/{basename}"))?; + let mut actual = Vec::new(); + entry.read_to_end(&mut actual)?; + assert_eq!(actual, *bytes); + assert_eq!(manifest[index]["filename"], *filename); + assert_eq!(entry.compression(), zip::CompressionMethod::Deflated); + } + Ok(()) +} + +/// Byte and count budgets include metadata and reject excess input without leaving a partial ZIP. +#[test] +fn archive_bounds_all_contents_and_cleans_failed_exports() -> Result<()> { + let directory = tempfile::tempdir()?; + let path = directory.path().join("feedback.zip"); + // Filtering removes the exact size hint, exercising streaming attachment limits. + let attachments = |count| { + (0..count).filter(|_| true).map(|_| { + Input::Owned(FeedbackAttachment { + filename: "test.log".to_string(), + buffer: b"synthetic".to_vec(), + content_type: None, + }) + }) + }; + super::save( + &path, + b"{}", + attachments(2), + Limits { + max_bytes: 1024, + max_attachments: 2, + }, + )?; + let mut archive = ZipArchive::new(File::open(&path)?)?; + let bytes = (0..archive.len()).try_fold(0, |sum, index| { + Ok::<_, zip::result::ZipError>(sum + archive.by_index(index)?.size() as usize) + })?; + drop(archive); + std::fs::remove_file(&path)?; + super::save( + &path, + b"{}", + attachments(2), + Limits { + max_bytes: bytes, + max_attachments: 2, + }, + )?; + std::fs::remove_file(&path)?; + for (count, max_bytes, error) in [ + (2, bytes - 1, "uncompressed size limit"), + (0, 1, "uncompressed size limit"), + (3, 1024, "attachment count limit"), + ] { + let failure = super::save( + &path, + b"{}", + attachments(count), + Limits { + max_bytes, + max_attachments: 2, + }, + ) + .unwrap_err(); + assert!(failure.to_string().contains(error), "{failure:#}"); + assert_eq!(std::fs::read_dir(directory.path())?.count(), 0); + } + Ok(()) +} + +/// A local report keeps its description and includes log bytes only with consent. +#[test] +fn save_preserves_report_and_log_consent() -> Result<()> { + let directory = tempfile::tempdir()?; + for include_logs in [false, true] { + let path = directory.path().join(format!("{include_logs}.zip")); + CodexFeedback::new() + .snapshot(/*session_id*/ None) + .save_feedback( + FeedbackUploadOptions { + include_logs, + ..options() + }, + &path, + )?; + let mut archive = ZipArchive::new(File::open(path)?)?; + let report: serde_json::Value = serde_json::from_reader(archive.by_name("report.json")?)?; + assert_eq!( + report["exception"]["values"][0]["value"], + "synthetic feedback" + ); + let logs = archive + .file_names() + .collect::>>()? + .into_iter() + .find(|name| name.ends_with("/codex-logs.log")) + .map(std::borrow::Cow::into_owned); + assert_eq!(logs.is_some(), include_logs); + if let Some(logs) = logs { + let mut bytes = Vec::new(); + archive.by_name(&logs)?.read_to_end(&mut bytes)?; + assert_eq!(bytes, b"consented log bytes"); + } + } + Ok(()) +} + +/// Oversized reports fail before publishing, using the serialized report size. +#[test] +fn save_rejects_oversized_events_without_publishing() -> Result<()> { + let directory = tempfile::tempdir()?; + let reason = "x".repeat(crate::MAX_EVENT_BYTES); + let error = CodexFeedback::new() + .snapshot(/*session_id*/ None) + .save_feedback( + FeedbackUploadOptions { + reason: Some(&reason), + ..options() + }, + &directory.path().join("oversized.zip"), + ) + .unwrap_err(); + assert_eq!(error.to_string(), "feedback event exceeds the size limit"); + assert_eq!(std::fs::read_dir(directory.path())?.count(), 0); + Ok(()) +} diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index 9920fbf022..3bc0c329a2 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -31,6 +31,7 @@ use tracing_subscriber::filter::Targets; use tracing_subscriber::fmt::writer::MakeWriter; use tracing_subscriber::registry::LookupSpan; +mod archive; mod daemon_logs; pub use daemon_logs::daemon_log_attachments; pub(crate) mod feedback_diagnostics; @@ -652,6 +653,46 @@ impl FeedbackSnapshot { .await } + /// Atomically saves the consent-selected report and attachments without uploading. + /// Existing paths are rejected and failures never publish a partial archive. + pub fn save_feedback(&self, options: FeedbackUploadOptions<'_>, path: &Path) -> Result<()> { + let report = serde_json::to_vec(&self.feedback_event( + options.classification, + options.reason, + options.tags, + options.session_source.as_ref(), + ))?; + anyhow::ensure!( + report.len() <= MAX_EVENT_BYTES, + "feedback event exceeds the size limit" + ); + let logs = options + .include_logs + .then(|| self.log_attachment(options.logs_override)); + let diagnostics = self + .feedback_diagnostics_attachment_text(options.include_logs) + .map(|text| FeedbackAttachment { + buffer: text.into_bytes(), + filename: FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.to_string(), + content_type: Some("text/plain".to_string()), + }); + archive::save( + path, + &report, + logs.into_iter() + .chain(options.extra_attachments) + .chain(diagnostics) + .map(archive::Input::Owned) + .chain( + options + .extra_attachment_paths + .iter() + .map(archive::Input::Path), + ), + archive::Limits::default(), + ) + } + async fn upload_feedback_with_dsn( &self, options: FeedbackUploadOptions<'_>, @@ -1863,3 +1904,6 @@ mod tests { ); } } + +#[cfg(test)] +mod save_rollout_tests; diff --git a/codex-rs/feedback/src/save_rollout_tests.rs b/codex-rs/feedback/src/save_rollout_tests.rs new file mode 100644 index 0000000000..730c2a4637 --- /dev/null +++ b/codex-rs/feedback/src/save_rollout_tests.rs @@ -0,0 +1,107 @@ +//! Regression coverage for complete, bounded file selection during local saving. +use super::*; +use pretty_assertions::assert_eq; + +fn rollout_name() -> String { + format!("rollout-2026-10-19T12-00-00-{}.jsonl", ThreadId::new()) +} + +#[test] +fn save_bounds_files_before_reading_or_compression() -> Result<()> { + let inputs = tempfile::tempdir()?; + let output = tempfile::tempdir()?; + let path = inputs.path().join(rollout_name()); + fs::write(&path, vec![b' '; 1024])?; + let path = FeedbackAttachmentPath { + path, + attachment_filename_override: None, + }; + let missing = FeedbackAttachmentPath { + path: inputs.path().join("missing"), + attachment_filename_override: None, + }; + for (max_bytes, max_attachments, second, expected_error) in [ + (1500, 2, &path, "decoded size limit"), + (4096, 1, &missing, "count limit"), + ] { + let error = archive::save( + &output.path().join("feedback.zip"), + b"{}", + [ + archive::Input::Owned(FeedbackAttachment { + filename: rollout_name(), + content_type: None, + buffer: vec![b' '; 1024], + }), + archive::Input::Path(second), + ] + .into_iter(), + archive::Limits { + max_bytes, + max_attachments, + }, + ) + .unwrap_err(); + assert!(format!("{error:#}").contains(expected_error), "{error:#}"); + assert_eq!(fs::read_dir(output.path())?.count(), 0); + } + Ok(()) +} + +#[test] +fn save_preserves_duplicate_names_and_rejects_missing_selected_files() -> Result<()> { + let inputs = tempfile::tempdir()?; + let output = tempfile::tempdir()?; + let name = rollout_name(); + let paths = [ + inputs.path().join(&name), + inputs.path().join("nested").join(&name), + ]; + fs::create_dir(paths[1].parent().unwrap())?; + fs::write(&paths[0], b"first\n")?; + fs::write(&paths[1], b"second\n")?; + let paths = paths.map(|path| FeedbackAttachmentPath { + path, + attachment_filename_override: None, + }); + let destination = output.path().join("feedback.zip"); + archive::save( + &destination, + b"{}", + paths.iter().map(archive::Input::Path), + archive::Limits::default(), + )?; + let mut zip = zip::ZipArchive::new(fs::File::open(&destination)?)?; + for (index, expected) in [b"first\n".as_slice(), b"second\n"].iter().enumerate() { + let mut actual = Vec::new(); + zip.by_name(&format!("attachments/{index:04}/{name}"))? + .read_to_end(&mut actual)?; + assert_eq!(&actual, expected); + } + drop(zip); + fs::remove_file(&destination)?; + fs::remove_file(&paths[1].path)?; + let error = archive::save( + &destination, + b"{}", + paths.iter().map(archive::Input::Path), + archive::Limits::default(), + ) + .unwrap_err(); + assert!(format!("{error:#}").contains("selected attachment")); + assert_eq!(fs::read_dir(output.path())?.count(), 0); + // Upload still uses its existing best-effort aggregation and skips the missing rollout. + let snapshot = CodexFeedback::new().snapshot(/*session_id*/ None); + let uploaded = snapshot + .feedback_attachments( + &sentry::protocol::EnvelopeHeaders::default(), + /*include_logs*/ false, + Vec::new(), + &paths, + /*logs_override*/ None, + ) + .collect::>>()?; + assert_eq!(uploaded.len(), 1); + assert_eq!(uploaded[0].filename, "rollouts.tar.gz"); + Ok(()) +} diff --git a/codex-rs/thread-store/src/live_thread.rs b/codex-rs/thread-store/src/live_thread.rs index a6de95b297..0c45c2ea6d 100644 --- a/codex-rs/thread-store/src/live_thread.rs +++ b/codex-rs/thread-store/src/live_thread.rs @@ -411,6 +411,18 @@ impl LiveThread { .map(Some) } + /// Selects a local feedback input only after its writer has materialized it. + pub async fn feedback_rollout_path(&self) -> ThreadStoreResult> { + let Some(local_store) = self + .thread_store + .as_any() + .downcast_ref::() + else { + return Ok(None); + }; + local_store.feedback_rollout_path(self.thread_id).await + } + async fn flush_pending_metadata_update(&self) -> ThreadStoreResult<()> { let update = self.metadata_sync.lock().await.take_pending_update(); self.apply_pending_metadata_update(update, PersistContext::Standard) diff --git a/codex-rs/thread-store/src/local/mod.rs b/codex-rs/thread-store/src/local/mod.rs index 326cec8139..17e58a6180 100644 --- a/codex-rs/thread-store/src/local/mod.rs +++ b/codex-rs/thread-store/src/local/mod.rs @@ -339,6 +339,26 @@ impl LocalThreadStore { live_writer::rollout_path(self, thread_id).await } + /// Select only materialized rollout inputs without creating a file for untouched threads. + pub(crate) async fn feedback_rollout_path( + &self, + thread_id: ThreadId, + ) -> ThreadStoreResult> { + let (recorder, _, _) = live_writer::live_writer_parts(self, thread_id).await?; + if recorder + .unpersisted_session_meta() + .await + .map_err(|error| ThreadStoreError::Internal { + message: error.to_string(), + })? + .is_some() + { + return Ok(None); + } + // Keep the path even if a materialized file was deleted: collection must report that error. + Ok(Some(recorder.rollout_path().to_path_buf())) + } + pub(super) async fn ensure_live_recorder_absent( &self, thread_id: ThreadId, diff --git a/sdk/python/src/openai_codex/generated/v2_all.py b/sdk/python/src/openai_codex/generated/v2_all.py index 00c94c9c6a..0ebfe8ad47 100644 --- a/sdk/python/src/openai_codex/generated/v2_all.py +++ b/sdk/python/src/openai_codex/generated/v2_all.py @@ -8336,6 +8336,40 @@ class ExternalAgentImportedConnectorCandidate(BaseModel): source: ExternalAgentImportedConnectorSource +class FeedbackSaveParams(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + ) + classification: str + extra_log_files: Annotated[list[str] | None, Field(alias="extraLogFiles")] = None + include_logs: Annotated[bool | None, Field(alias="includeLogs")] = None + reason: str | None = None + save_path: Annotated[ + LegacyAppPathString, + Field( + alias="savePath", + description="Absolute, previously unused ZIP path on the app-server host. The caller owns this file, including after a disconnected RPC.", + ), + ] + tags: dict[str, Any] | None = None + thread_id: Annotated[str | None, Field(alias="threadId")] = None + + +class FeedbackSaveResponse(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + ) + prompt_hash: Annotated[ + str | None, + Field( + alias="promptHash", + description="Same prompt hash as feedback/upload; null when prompt metadata is absent.", + ), + ] = None + saved_path: Annotated[LegacyAppPathString, Field(alias="savedPath")] + thread_id: Annotated[str, Field(alias="threadId")] + + class PathFileSystemPath(BaseModel): model_config = ConfigDict( populate_by_name=True, @@ -10889,6 +10923,15 @@ class AccountLoginStartRequest(BaseModel): params: LoginAccountParams +class FeedbackSaveRequest(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + ) + id: RequestId + method: Annotated[Literal["feedback/save"], Field(title="Feedback/saveRequestMethod")] + params: FeedbackSaveParams + + class CommandExecRequest(BaseModel): model_config = ConfigDict( populate_by_name=True, @@ -12936,6 +12979,7 @@ class ClientRequest( | AccountWorkspaceMessagesReadRequest | AccountSendAddCreditsNudgeEmailRequest | FeedbackUploadRequest + | FeedbackSaveRequest | CommandExecRequest | CommandExecWriteRequest | CommandExecTerminateRequest @@ -13047,6 +13091,7 @@ class ClientRequest( | AccountWorkspaceMessagesReadRequest | AccountSendAddCreditsNudgeEmailRequest | FeedbackUploadRequest + | FeedbackSaveRequest | CommandExecRequest | CommandExecWriteRequest | CommandExecTerminateRequest