diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2d217ea063..ee778fb5a5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1239,6 +1239,7 @@ dependencies = [ "core_test_support", "futures", "os_info", + "owo-colors", "pretty_assertions", "rmcp", "serde", @@ -2006,6 +2007,7 @@ dependencies = [ "codex-protocol", "codex-utils-cargo-bin", "codex-utils-home-dir", + "codex-utils-pty", "futures", "keyring", "oauth2", diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 7ffaadbfcb..aa6c86d879 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1064,11 +1064,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", diff --git a/codex-rs/app-server-protocol/schema/json/EventMsg.json b/codex-rs/app-server-protocol/schema/json/EventMsg.json index f399912e03..905d6af657 100644 --- a/codex-rs/app-server-protocol/schema/json/EventMsg.json +++ b/codex-rs/app-server-protocol/schema/json/EventMsg.json @@ -3169,11 +3169,23 @@ ] }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -4685,6 +4697,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -4695,6 +4708,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 0e2f94f4d8..46a47ac762 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -3948,11 +3948,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -6686,6 +6698,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -6696,6 +6709,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" 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 ff31b0d5e0..5e73227a7f 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 @@ -6064,11 +6064,23 @@ ] }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -9146,6 +9158,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -9156,6 +9169,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" @@ -10518,17 +10542,6 @@ } ] }, - "apps": { - "anyOf": [ - { - "$ref": "#/definitions/v2/AppsConfig" - }, - { - "type": "null" - } - ], - "default": null - }, "compact_prompt": { "type": [ "string", @@ -12198,11 +12211,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", diff --git a/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json b/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json index a5838e89e7..3b5ed750b7 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json @@ -3169,11 +3169,23 @@ ] }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -4685,6 +4697,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -4695,6 +4708,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" diff --git a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationParams.json b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationParams.json index 9ce52963d9..5c7f943786 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationParams.json +++ b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationParams.json @@ -271,11 +271,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "NewConversationParams": { "properties": { diff --git a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json index 718b17aa28..9c5c3653e8 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json @@ -3169,11 +3169,23 @@ ] }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -4685,6 +4697,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -4695,6 +4708,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" diff --git a/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json b/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json index a85b78281b..a008e02538 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json @@ -3169,11 +3169,23 @@ ] }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ModeKind": { "description": "Initial collaboration mode to use when the TUI starts.", @@ -4685,6 +4697,7 @@ "type": "object" }, { + "description": "Assistant-authored message payload used in turn-item streams.\n\n`phase` is optional because not all providers/models emit it. Consumers should use it when present, but retain legacy completion semantics when it is `None`.", "properties": { "content": { "items": { @@ -4695,6 +4708,17 @@ "id": { "type": "string" }, + "phase": { + "anyOf": [ + { + "$ref": "#/definitions/MessagePhase" + }, + { + "type": "null" + } + ], + "description": "Optional phase metadata carried through from `ResponseItem::Message`.\n\nThis is currently used by TUI rendering to distinguish mid-turn commentary from a final answer and avoid status-indicator jitter." + }, "type": { "enum": [ "AgentMessage" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json index 48af978271..d934df7f58 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json @@ -78,17 +78,6 @@ } ] }, - "apps": { - "anyOf": [ - { - "$ref": "#/definitions/AppsConfig" - }, - { - "type": "null" - } - ], - "default": null - }, "compact_prompt": { "type": [ "string", diff --git a/codex-rs/app-server-protocol/schema/json/v2/RawResponseItemCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/RawResponseItemCompletedNotification.json index c1e36ad8e8..748eeaab46 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/RawResponseItemCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/RawResponseItemCompletedNotification.json @@ -238,11 +238,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "ReasoningItemContent": { "oneOf": [ diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json index 58e20e58e2..fc6593b186 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.json @@ -247,11 +247,23 @@ "type": "string" }, "MessagePhase": { - "enum": [ - "commentary", - "final_answer" - ], - "type": "string" + "description": "Classifies an assistant message as interim commentary or final answer text.\n\nProviders do not emit this consistently, so callers must treat `None` as \"phase unknown\" and keep compatibility behavior for legacy models.", + "oneOf": [ + { + "description": "Mid-turn assistant text (for example preamble/progress narration).\n\nAdditional tool calls or assistant output may follow before turn completion.", + "enum": [ + "commentary" + ], + "type": "string" + }, + { + "description": "The assistant's terminal answer text for the current turn.", + "enum": [ + "final_answer" + ], + "type": "string" + } + ] }, "Personality": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/typescript/AgentMessageItem.ts b/codex-rs/app-server-protocol/schema/typescript/AgentMessageItem.ts index f884067581..ee67a3e23b 100644 --- a/codex-rs/app-server-protocol/schema/typescript/AgentMessageItem.ts +++ b/codex-rs/app-server-protocol/schema/typescript/AgentMessageItem.ts @@ -2,5 +2,20 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AgentMessageContent } from "./AgentMessageContent"; +import type { MessagePhase } from "./MessagePhase"; -export type AgentMessageItem = { id: string, content: Array, }; +/** + * Assistant-authored message payload used in turn-item streams. + * + * `phase` is optional because not all providers/models emit it. Consumers + * should use it when present, but retain legacy completion semantics when it + * is `None`. + */ +export type AgentMessageItem = { id: string, content: Array, +/** + * Optional phase metadata carried through from `ResponseItem::Message`. + * + * This is currently used by TUI rendering to distinguish mid-turn + * commentary from a final answer and avoid status-indicator jitter. + */ +phase?: MessagePhase, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/MessagePhase.ts b/codex-rs/app-server-protocol/schema/typescript/MessagePhase.ts index d339c0fa83..9e16021b54 100644 --- a/codex-rs/app-server-protocol/schema/typescript/MessagePhase.ts +++ b/codex-rs/app-server-protocol/schema/typescript/MessagePhase.ts @@ -2,4 +2,10 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +/** + * Classifies an assistant message as interim commentary or final answer text. + * + * Providers do not emit this consistently, so callers must treat `None` as + * "phase unknown" and keep compatibility behavior for legacy models. + */ export type MessagePhase = "commentary" | "final_answer"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/Config.ts b/codex-rs/app-server-protocol/schema/typescript/v2/Config.ts index 00493b6e68..aee0ac3383 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/Config.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/Config.ts @@ -8,11 +8,10 @@ import type { Verbosity } from "../Verbosity"; import type { WebSearchMode } from "../WebSearchMode"; import type { JsonValue } from "../serde_json/JsonValue"; import type { AnalyticsConfig } from "./AnalyticsConfig"; -import type { AppsConfig } from "./AppsConfig"; import type { AskForApproval } from "./AskForApproval"; import type { ProfileV2 } from "./ProfileV2"; import type { SandboxMode } from "./SandboxMode"; import type { SandboxWorkspaceWrite } from "./SandboxWorkspaceWrite"; import type { ToolsV2 } from "./ToolsV2"; -export type Config = { model: string | null, review_model: string | null, model_context_window: bigint | null, model_auto_compact_token_limit: bigint | null, model_provider: string | null, approval_policy: AskForApproval | null, sandbox_mode: SandboxMode | null, sandbox_workspace_write: SandboxWorkspaceWrite | null, forced_chatgpt_workspace_id: string | null, forced_login_method: ForcedLoginMethod | null, web_search: WebSearchMode | null, tools: ToolsV2 | null, profile: string | null, profiles: { [key in string]?: ProfileV2 }, instructions: string | null, developer_instructions: string | null, compact_prompt: string | null, model_reasoning_effort: ReasoningEffort | null, model_reasoning_summary: ReasoningSummary | null, model_verbosity: Verbosity | null, analytics: AnalyticsConfig | null, apps: AppsConfig | null, } & ({ [key in string]?: number | string | boolean | Array | { [key in string]?: JsonValue } | null }); +export type Config = {model: string | null, review_model: string | null, model_context_window: bigint | null, model_auto_compact_token_limit: bigint | null, model_provider: string | null, approval_policy: AskForApproval | null, sandbox_mode: SandboxMode | null, sandbox_workspace_write: SandboxWorkspaceWrite | null, forced_chatgpt_workspace_id: string | null, forced_login_method: ForcedLoginMethod | null, web_search: WebSearchMode | null, tools: ToolsV2 | null, profile: string | null, profiles: { [key in string]?: ProfileV2 }, instructions: string | null, developer_instructions: string | null, compact_prompt: string | null, model_reasoning_effort: ReasoningEffort | null, model_reasoning_summary: ReasoningSummary | null, model_verbosity: Verbosity | null, analytics: AnalyticsConfig | null} & ({ [key in string]?: number | string | boolean | Array | { [key in string]?: JsonValue } | null }); diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index 6de7c114ca..5c4954b3cc 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -238,7 +238,10 @@ fn filter_client_request_ts(out_dir: &Path, experimental_methods: &[&str]) -> Re .collect(); let new_body = filtered_arms.join(" | "); content = format!("{prefix}{new_body}{suffix}"); - content = prune_unused_type_imports(content, &new_body); + let import_usage_scope = split_type_alias(&content) + .map(|(_, body, _)| body) + .unwrap_or_else(|| new_body.clone()); + content = prune_unused_type_imports(content, &import_usage_scope); fs::write(&path, content).with_context(|| format!("Failed to write {}", path.display()))?; Ok(()) @@ -296,7 +299,10 @@ fn filter_experimental_fields_in_ts_file( let prefix = &content[..open_brace + 1]; let suffix = &content[close_brace..]; content = format!("{prefix}{new_inner}{suffix}"); - content = prune_unused_type_imports(content, &new_inner); + let import_usage_scope = split_type_alias(&content) + .map(|(_, body, _)| body) + .unwrap_or_else(|| new_inner.clone()); + content = prune_unused_type_imports(content, &import_usage_scope); fs::write(path, content).with_context(|| format!("Failed to write {}", path.display()))?; Ok(()) } @@ -1745,6 +1751,50 @@ mod tests { Ok(()) } + #[test] + fn experimental_type_fields_ts_filter_keeps_imports_used_in_intersection_suffix() -> Result<()> + { + let output_dir = std::env::temp_dir().join(format!("codex_ts_filter_{}", Uuid::now_v7())); + fs::create_dir_all(&output_dir)?; + + struct TempDirGuard(PathBuf); + + impl Drop for TempDirGuard { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + + let _guard = TempDirGuard(output_dir.clone()); + let path = output_dir.join("Config.ts"); + let content = r#"import type { JsonValue } from "../serde_json/JsonValue"; +import type { Keep } from "./Keep"; + +export type Config = { stableField: Keep, unstableField: string | null } & ({ [key in string]?: number | string | boolean | Array | { [key in string]?: JsonValue } | null }); +"#; + fs::write(&path, content)?; + + static CUSTOM_FIELD: crate::experimental_api::ExperimentalField = + crate::experimental_api::ExperimentalField { + type_name: "Config", + field_name: "unstableField", + reason: "custom/unstableField", + }; + filter_experimental_type_fields_ts(&output_dir, &[&CUSTOM_FIELD])?; + + let filtered = fs::read_to_string(&path)?; + assert_eq!(filtered.contains("unstableField"), false); + assert_eq!( + filtered.contains(r#"import type { JsonValue } from "../serde_json/JsonValue";"#), + true + ); + assert_eq!( + filtered.contains(r#"import type { Keep } from "./Keep";"#), + true + ); + Ok(()) + } + #[test] fn stable_schema_filter_removes_mock_experimental_method() -> Result<()> { let output_dir = std::env::temp_dir().join(format!("codex_schema_{}", Uuid::now_v7())); diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index c3fd335941..4f5b3df5b6 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -404,7 +404,7 @@ const fn default_enabled() -> bool { true } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS, ExperimentalApi)] #[serde(rename_all = "snake_case")] #[ts(export_to = "v2/")] pub struct Config { @@ -430,6 +430,7 @@ pub struct Config { pub model_reasoning_summary: Option, pub model_verbosity: Option, pub analytics: Option, + #[experimental("config/read.apps")] #[serde(default)] pub apps: Option, #[serde(default, flatten)] @@ -3249,6 +3250,7 @@ mod tests { text: "world".to_string(), }, ], + phase: None, }); assert_eq!( diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index 2f6011a634..f68e078775 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -35,6 +35,7 @@ codex-utils-json-to-toml = { workspace = true } chrono = { workspace = true } clap = { workspace = true, features = ["derive"] } futures = { workspace = true } +owo-colors = { workspace = true, features = ["supports-colors"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/app-server/src/transport.rs b/codex-rs/app-server/src/transport.rs index 52f30055d0..39fd13212c 100644 --- a/codex-rs/app-server/src/transport.rs +++ b/codex-rs/app-server/src/transport.rs @@ -5,6 +5,9 @@ use crate::outgoing_message::OutgoingMessage; use codex_app_server_protocol::JSONRPCMessage; use futures::SinkExt; use futures::StreamExt; +use owo_colors::OwoColorize; +use owo_colors::Stream; +use owo_colors::Style; use std::collections::HashMap; use std::io::ErrorKind; use std::io::Result as IoResult; @@ -33,6 +36,36 @@ use tracing::warn; /// plenty for an interactive CLI. pub(crate) const CHANNEL_CAPACITY: usize = 128; +fn colorize(text: &str, style: Style) -> String { + text.if_supports_color(Stream::Stderr, |value| value.style(style)) + .to_string() +} + +#[allow(clippy::print_stderr)] +fn print_websocket_startup_banner(addr: SocketAddr) { + let title = colorize("codex app-server (WebSockets)", Style::new().bold().cyan()); + let listening_label = colorize("listening on:", Style::new().dimmed()); + let listen_url = colorize(&format!("ws://{addr}"), Style::new().green()); + let note_label = colorize("note:", Style::new().dimmed()); + eprintln!("{title}"); + eprintln!(" {listening_label} {listen_url}"); + if addr.ip().is_loopback() { + eprintln!( + " {note_label} binds localhost only (use SSH port-forwarding for remote access)" + ); + } else { + eprintln!( + " {note_label} this is a raw WS server; consider running behind TLS/auth for real remote use" + ); + } +} + +#[allow(clippy::print_stderr)] +fn print_websocket_connection(peer_addr: SocketAddr) { + let connected_label = colorize("websocket client connected from", Style::new().dimmed()); + eprintln!("{connected_label} {peer_addr}"); +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum AppServerTransport { Stdio, @@ -191,13 +224,15 @@ pub(crate) async fn start_websocket_acceptor( ) -> IoResult> { let listener = TcpListener::bind(bind_address).await?; let local_addr = listener.local_addr()?; + print_websocket_startup_banner(local_addr); info!("app-server websocket listening on ws://{local_addr}"); let connection_counter = Arc::new(AtomicU64::new(1)); Ok(tokio::spawn(async move { loop { match listener.accept().await { - Ok((stream, _peer_addr)) => { + Ok((stream, peer_addr)) => { + print_websocket_connection(peer_addr); let connection_id = ConnectionId(connection_counter.fetch_add(1, Ordering::Relaxed)); let transport_event_tx_for_connection = transport_event_tx.clone(); diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 265db809e6..5b4b04297e 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -407,6 +407,9 @@ sandbox_mode = "read-only" model_provider = "mock_provider" +[features] +remote_models = false + [model_providers.mock_provider] name = "Mock provider" base_url = "{server_uri}/v1" diff --git a/codex-rs/codex-api/src/common.rs b/codex-rs/codex-api/src/common.rs index f1a996f12c..b8471c020e 100644 --- a/codex-rs/codex-api/src/common.rs +++ b/codex-rs/codex-api/src/common.rs @@ -172,6 +172,8 @@ pub struct ResponsesApiRequest<'a> { pub struct ResponseCreateWsRequest { pub model: String, pub instructions: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub previous_response_id: Option, pub input: Vec, pub tools: Vec, pub tool_choice: String, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index fac5442403..53ab772492 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -239,6 +239,9 @@ "responses_websockets": { "type": "boolean" }, + "responses_websockets_v2": { + "type": "boolean" + }, "runtime_metrics": { "type": "boolean" }, @@ -1279,6 +1282,9 @@ "responses_websockets": { "type": "boolean" }, + "responses_websockets_v2": { + "type": "boolean" + }, "runtime_metrics": { "type": "boolean" }, diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 31ef3fd0bf..d8dfad9621 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -89,6 +89,8 @@ use reqwest::StatusCode; use serde_json::Value; use std::time::Duration; use tokio::sync::mpsc; +use tokio::sync::oneshot; +use tokio::sync::oneshot::error::TryRecvError; use tokio::task::JoinHandle; use tokio_tungstenite::tungstenite::Error; use tokio_tungstenite::tungstenite::Message; @@ -117,6 +119,7 @@ pub const X_CODEX_TURN_STATE_HEADER: &str = "x-codex-turn-state"; pub const X_CODEX_TURN_METADATA_HEADER: &str = "x-codex-turn-metadata"; pub const X_RESPONSESAPI_INCLUDE_TIMING_METRICS_HEADER: &str = "x-responsesapi-include-timing-metrics"; +const RESPONSES_WEBSOCKETS_V2_BETA_HEADER_VALUE: &str = "responses_websockets=2026-02-06"; /// Session-scoped state shared by all [`ModelClient`] clones. /// @@ -129,6 +132,7 @@ struct ModelClientState { session_source: SessionSource, model_verbosity: Option, enable_responses_websockets: bool, + enable_responses_websockets_v2: bool, enable_request_compression: bool, include_timing_metrics: bool, beta_features_header: Option, @@ -238,6 +242,8 @@ pub struct ModelClientSession { client: ModelClient, connection: Option, websocket_last_items: Vec, + websocket_last_response_id: Option, + websocket_last_response_id_rx: Option>, /// Turn state for sticky routing. /// /// This is an `OnceLock` that stores the turn state value received from the server @@ -264,6 +270,7 @@ impl ModelClient { session_source: SessionSource, model_verbosity: Option, enable_responses_websockets: bool, + enable_responses_websockets_v2: bool, enable_request_compression: bool, include_timing_metrics: bool, beta_features_header: Option, @@ -276,6 +283,7 @@ impl ModelClient { session_source, model_verbosity, enable_responses_websockets, + enable_responses_websockets_v2, enable_request_compression, include_timing_metrics, beta_features_header, @@ -295,6 +303,8 @@ impl ModelClient { client: self.clone(), connection: None, websocket_last_items: Vec::new(), + websocket_last_response_id: None, + websocket_last_response_id_rx: None, turn_state: Arc::new(OnceLock::new()), } } @@ -479,6 +489,10 @@ impl ModelClient { self.state.provider.supports_websockets && self.state.enable_responses_websockets } + fn responses_websockets_v2_enabled(&self) -> bool { + self.state.enable_responses_websockets_v2 + } + /// Returns whether websocket transport has been permanently disabled for this session. /// /// Once set by fallback activation, subsequent turns must stay on HTTP transport. @@ -544,9 +558,14 @@ impl ModelClient { headers.extend(build_conversation_headers(Some( self.state.conversation_id.to_string(), ))); + let responses_websockets_beta_header = if self.responses_websockets_v2_enabled() { + RESPONSES_WEBSOCKETS_V2_BETA_HEADER_VALUE + } else { + OPENAI_BETA_RESPONSES_WEBSOCKETS + }; headers.insert( OPENAI_BETA_HEADER, - HeaderValue::from_static(OPENAI_BETA_RESPONSES_WEBSOCKETS), + HeaderValue::from_static(responses_websockets_beta_header), ); if self.state.include_timing_metrics { headers.insert( @@ -789,18 +808,37 @@ impl ModelClientSession { } } - fn prepare_websocket_request( + fn refresh_websocket_last_response_id(&mut self) { + if let Some(mut receiver) = self.websocket_last_response_id_rx.take() { + match receiver.try_recv() { + Ok(response_id) if !response_id.is_empty() => { + self.websocket_last_response_id = Some(response_id); + } + Ok(_) | Err(TryRecvError::Closed) => { + self.websocket_last_response_id = None; + } + Err(TryRecvError::Empty) => { + self.websocket_last_response_id_rx = Some(receiver); + } + } + } + } + + fn websocket_previous_response_id(&mut self) -> Option { + self.refresh_websocket_last_response_id(); + self.websocket_last_response_id + .clone() + .filter(|id| !id.is_empty()) + } + + fn prepare_websocket_create_request( &self, model_slug: &str, api_prompt: &ApiPrompt, options: &ApiResponsesOptions, + input: Vec, + previous_response_id: Option, ) -> ResponsesWsRequest { - if let Some(append_items) = self.get_incremental_items(&api_prompt.input) { - return ResponsesWsRequest::ResponseAppend(ResponseAppendWsRequest { - input: append_items, - }); - } - let ApiResponsesOptions { reasoning, include, @@ -814,7 +852,8 @@ impl ModelClientSession { let payload = ResponseCreateWsRequest { model: model_slug.to_string(), instructions: api_prompt.instructions.clone(), - input: api_prompt.input.clone(), + previous_response_id, + input, tools: api_prompt.tools.clone(), tool_choice: "auto".to_string(), parallel_tool_calls: api_prompt.parallel_tool_calls, @@ -829,6 +868,43 @@ impl ModelClientSession { ResponsesWsRequest::ResponseCreate(payload) } + fn prepare_websocket_request( + &mut self, + model_slug: &str, + api_prompt: &ApiPrompt, + options: &ApiResponsesOptions, + ) -> ResponsesWsRequest { + let responses_websockets_v2_enabled = self.client.responses_websockets_v2_enabled(); + let incremental_items = self.get_incremental_items(&api_prompt.input); + if let Some(append_items) = incremental_items { + if responses_websockets_v2_enabled + && let Some(previous_response_id) = self.websocket_previous_response_id() + { + return self.prepare_websocket_create_request( + model_slug, + api_prompt, + options, + append_items, + Some(previous_response_id), + ); + } + + if !responses_websockets_v2_enabled { + return ResponsesWsRequest::ResponseAppend(ResponseAppendWsRequest { + input: append_items, + }); + } + } + + self.prepare_websocket_create_request( + model_slug, + api_prompt, + options, + api_prompt.input.clone(), + None, + ) + } + /// Returns a websocket connection for this turn, reusing preconnect when possible. /// /// This method first tries to adopt the session-level preconnect slot, then falls back to a @@ -863,6 +939,9 @@ impl ModelClientSession { if needs_new { self.client.clear_preconnect(); + self.websocket_last_items.clear(); + self.websocket_last_response_id = None; + self.websocket_last_response_id_rx = None; let turn_state = options .turn_state .clone() @@ -1023,9 +1102,8 @@ impl ModelClientSession { turn_metadata_header, compression, ); - let request = self.prepare_websocket_request(&model_info.slug, &api_prompt, &options); - let connection = match self + match self .websocket_connection( otel_manager, client_setup.api_provider, @@ -1035,7 +1113,7 @@ impl ModelClientSession { ) .await { - Ok(connection) => connection, + Ok(_) => {} Err(ApiError::Transport( unauthorized_transport @ TransportError::Http { status, .. }, )) if status == StatusCode::UNAUTHORIZED => { @@ -1043,13 +1121,33 @@ impl ModelClientSession { continue; } Err(err) => return Err(map_api_error(err)), - }; + } - let stream_result = connection + let request = self.prepare_websocket_request(&model_info.slug, &api_prompt, &options); + + let stream_result = self + .connection + .as_ref() + .ok_or_else(|| { + map_api_error(ApiError::Stream( + "websocket connection is unavailable".to_string(), + )) + })? .stream_request(request) .await .map_err(map_api_error)?; self.websocket_last_items = api_prompt.input.clone(); + let (last_response_id_sender, last_response_id_receiver) = oneshot::channel(); + self.websocket_last_response_id_rx = Some(last_response_id_receiver); + let mut last_response_id_sender = Some(last_response_id_sender); + let stream_result = stream_result.inspect(move |event| { + if let Ok(ResponseEvent::Completed { response_id, .. }) = event + && !response_id.is_empty() + && let Some(sender) = last_response_id_sender.take() + { + let _ = sender.send(response_id.clone()); + } + }); return Ok(map_response_stream(stream_result, otel_manager.clone())); } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c1c3c59b68..9fbbcd312d 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -993,6 +993,7 @@ impl Session { auth.and_then(CodexAuth::get_account_id), auth.and_then(CodexAuth::get_account_email), auth_mode, + crate::default_client::originator().value, config.otel.log_user_prompt, terminal::user_agent(), session_configuration.session_source.clone(), @@ -1072,7 +1073,9 @@ impl Session { session_configuration.provider.clone(), session_configuration.session_source.clone(), config.model_verbosity, - config.features.enabled(Feature::ResponsesWebsockets), + config.features.enabled(Feature::ResponsesWebsockets) + || config.features.enabled(Feature::ResponsesWebsocketsV2), + config.features.enabled(Feature::ResponsesWebsocketsV2), config.features.enabled(Feature::EnableRequestCompression), config.features.enabled(Feature::RuntimeMetrics), Self::build_model_client_beta_features_header(config.as_ref()), @@ -4500,6 +4503,7 @@ async fn emit_agent_message_in_plan_mode( TurnItem::AgentMessage(codex_protocol::items::AgentMessageItem { id: agent_message_id.clone(), content: Vec::new(), + phase: None, }) }); sess.emit_turn_item_started(turn_context, &start_item).await; @@ -5793,6 +5797,7 @@ mod tests { None, Some("test@test.com".to_string()), Some(TelemetryAuthMode::Chatgpt), + "test_originator".to_string(), false, "test".to_string(), session_source, @@ -5891,7 +5896,9 @@ mod tests { session_configuration.provider.clone(), session_configuration.session_source.clone(), config.model_verbosity, - config.features.enabled(Feature::ResponsesWebsockets), + config.features.enabled(Feature::ResponsesWebsockets) + || config.features.enabled(Feature::ResponsesWebsocketsV2), + config.features.enabled(Feature::ResponsesWebsocketsV2), config.features.enabled(Feature::EnableRequestCompression), config.features.enabled(Feature::RuntimeMetrics), Session::build_model_client_beta_features_header(config.as_ref()), @@ -6021,7 +6028,9 @@ mod tests { session_configuration.provider.clone(), session_configuration.session_source.clone(), config.model_verbosity, - config.features.enabled(Feature::ResponsesWebsockets), + config.features.enabled(Feature::ResponsesWebsockets) + || config.features.enabled(Feature::ResponsesWebsocketsV2), + config.features.enabled(Feature::ResponsesWebsocketsV2), config.features.enabled(Feature::EnableRequestCompression), config.features.enabled(Feature::RuntimeMetrics), Session::build_model_client_beta_features_header(config.as_ref()), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index a75c48fc96..9d7d9195cf 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2739,25 +2739,27 @@ profile = "project" } #[test] - fn responses_websockets_feature_does_not_change_wire_api() -> std::io::Result<()> { - let codex_home = TempDir::new()?; - let mut entries = BTreeMap::new(); - entries.insert("responses_websockets".to_string(), true); - let cfg = ConfigToml { - features: Some(crate::features::FeaturesToml { entries }), - ..Default::default() - }; + fn responses_websocket_features_do_not_change_wire_api() -> std::io::Result<()> { + for feature_key in ["responses_websockets", "responses_websockets_v2"] { + let codex_home = TempDir::new()?; + let mut entries = BTreeMap::new(); + entries.insert(feature_key.to_string(), true); + let cfg = ConfigToml { + features: Some(crate::features::FeaturesToml { entries }), + ..Default::default() + }; - let config = Config::load_from_base_config_with_overrides( - cfg, - ConfigOverrides::default(), - codex_home.path().to_path_buf(), - )?; + let config = Config::load_from_base_config_with_overrides( + cfg, + ConfigOverrides::default(), + codex_home.path().to_path_buf(), + )?; - assert_eq!( - config.model_provider.wire_api, - crate::model_provider_info::WireApi::Responses - ); + assert_eq!( + config.model_provider.wire_api, + crate::model_provider_info::WireApi::Responses + ); + } Ok(()) } diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 2ad19d3df5..16f7e1c47a 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -5,6 +5,7 @@ use codex_protocol::items::TurnItem; use codex_protocol::items::UserMessageItem; use codex_protocol::items::WebSearchItem; use codex_protocol::models::ContentItem; +use codex_protocol::models::MessagePhase; use codex_protocol::models::ReasoningItemContent; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::models::ResponseItem; @@ -69,7 +70,11 @@ fn parse_user_message(message: &[ContentItem]) -> Option { Some(UserMessageItem::new(&content)) } -fn parse_agent_message(id: Option<&String>, message: &[ContentItem]) -> AgentMessageItem { +fn parse_agent_message( + id: Option<&String>, + message: &[ContentItem], + phase: Option, +) -> AgentMessageItem { let mut content: Vec = Vec::new(); for content_item in message.iter() { match content_item { @@ -85,18 +90,23 @@ fn parse_agent_message(id: Option<&String>, message: &[ContentItem]) -> AgentMes } } let id = id.cloned().unwrap_or_else(|| Uuid::new_v4().to_string()); - AgentMessageItem { id, content } + AgentMessageItem { id, content, phase } } pub fn parse_turn_item(item: &ResponseItem) -> Option { match item { ResponseItem::Message { - role, content, id, .. + role, + content, + id, + phase, + .. } => match role.as_str() { "user" => parse_user_message(content).map(TurnItem::UserMessage), "assistant" => Some(TurnItem::AgentMessage(parse_agent_message( id.as_ref(), content, + phase.clone(), ))), "system" => None, _ => None, diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 2ea28d321a..e965076504 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -127,6 +127,8 @@ pub enum Feature { Personality, /// Use the Responses API WebSocket transport for OpenAI by default. ResponsesWebsockets, + /// Enable Responses API websocket v2 mode. + ResponsesWebsocketsV2, } impl Feature { @@ -569,6 +571,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, + FeatureSpec { + id: Feature::ResponsesWebsocketsV2, + key: "responses_websockets_v2", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, ]; /// Push a warning event if any under-development features are enabled. diff --git a/codex-rs/core/src/skills/injection.rs b/codex-rs/core/src/skills/injection.rs index 77b3ceea10..19ccdb2078 100644 --- a/codex-rs/core/src/skills/injection.rs +++ b/codex-rs/core/src/skills/injection.rs @@ -78,14 +78,17 @@ fn emit_skill_injected_metric(otel: Option<&OtelManager>, skill: &SkillMetadata, ); } -/// Collect explicitly mentioned skills from `$name` text mentions. +/// Collect explicitly mentioned skills from structured and text mentions. /// -/// Text inputs are scanned once to extract `$skill-name` tokens, then we iterate `skills` -/// in their existing order to preserve prior ordering semantics. Explicit links are -/// resolved by path and plain names are only used when the match is unambiguous. +/// Structured `UserInput::Skill` selections are resolved first by path against +/// enabled skills. Text inputs are then scanned to extract `$skill-name` tokens, and we +/// iterate `skills` in their existing order to preserve prior ordering semantics. +/// Explicit links are resolved by path and plain names are only used when the match +/// is unambiguous. /// -/// Complexity: `O(S + T + N_t * S)` time, `O(S)` space, where: -/// `S` = number of skills, `T` = total text length, `N_t` = number of text inputs. +/// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where: +/// `S` = number of skills, `T` = total text length, `N_s` = number of structured skill inputs, +/// `N_t` = number of text inputs, `M` = max mentions parsed from a single text input. pub(crate) fn collect_explicit_skill_mentions( inputs: &[UserInput], skills: &[SkillMetadata], @@ -102,12 +105,33 @@ pub(crate) fn collect_explicit_skill_mentions( let mut selected: Vec = Vec::new(); let mut seen_names: HashSet = HashSet::new(); let mut seen_paths: HashSet = HashSet::new(); + let mut blocked_plain_names: HashSet = HashSet::new(); + + for input in inputs { + if let UserInput::Skill { name, path } = input { + blocked_plain_names.insert(name.clone()); + if selection_context.disabled_paths.contains(path) || seen_paths.contains(path) { + continue; + } + + if let Some(skill) = selection_context + .skills + .iter() + .find(|skill| skill.path.as_path() == path.as_path()) + { + seen_paths.insert(skill.path.clone()); + seen_names.insert(skill.name.clone()); + selected.push(skill.clone()); + } + } + } for input in inputs { if let UserInput::Text { text, .. } = input { let mentioned_names = extract_tool_mentions(text); select_skills_from_mentions( &selection_context, + &blocked_plain_names, &mentioned_names, &mut seen_names, &mut seen_paths, @@ -254,6 +278,7 @@ pub(crate) fn extract_tool_mentions(text: &str) -> ToolMentions<'_> { /// Select mentioned skills while preserving the order of `skills`. fn select_skills_from_mentions( selection_context: &SkillSelectionContext<'_>, + blocked_plain_names: &HashSet, mentions: &ToolMentions<'_>, seen_names: &mut HashSet, seen_paths: &mut HashSet, @@ -296,6 +321,9 @@ fn select_skills_from_mentions( continue; } + if blocked_plain_names.contains(skill.name.as_str()) { + continue; + } if !mentions.plain_names.contains(skill.name.as_str()) { continue; } @@ -586,10 +614,10 @@ mod tests { } #[test] - fn collect_explicit_skill_mentions_ignores_structured_inputs() { + fn collect_explicit_skill_mentions_prioritizes_structured_inputs() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); let beta = make_skill("beta-skill", "/tmp/beta"); - let skills = vec![alpha.clone(), beta]; + let skills = vec![alpha.clone(), beta.clone()]; let inputs = vec![ UserInput::Text { text: "please run $alpha-skill".to_string(), @@ -604,7 +632,50 @@ mod tests { let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - assert_eq!(selected, vec![alpha]); + assert_eq!(selected, vec![beta, alpha]); + } + + #[test] + fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fallback() { + let alpha = make_skill("alpha-skill", "/tmp/alpha"); + let skills = vec![alpha]; + let inputs = vec![ + UserInput::Text { + text: "please run $alpha-skill".to_string(), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: "alpha-skill".to_string(), + path: PathBuf::from("/tmp/missing"), + }, + ]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); + + assert_eq!(selected, Vec::new()); + } + + #[test] + fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() { + let alpha = make_skill("alpha-skill", "/tmp/alpha"); + let skills = vec![alpha]; + let inputs = vec![ + UserInput::Text { + text: "please run $alpha-skill".to_string(), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: "alpha-skill".to_string(), + path: PathBuf::from("/tmp/alpha"), + }, + ]; + let disabled = HashSet::from([PathBuf::from("/tmp/alpha")]); + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); + + assert_eq!(selected, Vec::new()); } #[test] diff --git a/codex-rs/core/tests/responses_headers.rs b/codex-rs/core/tests/responses_headers.rs index 83f1d10423..ecd353a20d 100644 --- a/codex-rs/core/tests/responses_headers.rs +++ b/codex-rs/core/tests/responses_headers.rs @@ -80,6 +80,7 @@ async fn responses_stream_includes_subagent_header_on_review() { None, Some("test@test.com".to_string()), Some(auth_mode), + "test_originator".to_string(), false, "test".to_string(), session_source.clone(), @@ -94,6 +95,7 @@ async fn responses_stream_includes_subagent_header_on_review() { false, false, false, + false, None, ); let mut client_session = client.new_session(); @@ -182,6 +184,7 @@ async fn responses_stream_includes_subagent_header_on_other() { None, Some("test@test.com".to_string()), Some(auth_mode), + "test_originator".to_string(), false, "test".to_string(), session_source.clone(), @@ -196,6 +199,7 @@ async fn responses_stream_includes_subagent_header_on_other() { false, false, false, + false, None, ); let mut client_session = client.new_session(); @@ -283,6 +287,7 @@ async fn responses_respects_model_info_overrides_from_config() { None, Some("test@test.com".to_string()), auth_mode, + "test_originator".to_string(), false, "test".to_string(), session_source.clone(), @@ -297,6 +302,7 @@ async fn responses_respects_model_info_overrides_from_config() { false, false, false, + false, None, ); let mut client_session = client.new_session(); diff --git a/codex-rs/core/tests/suite/agent_websocket.rs b/codex-rs/core/tests/suite/agent_websocket.rs index 9fafcdd23b..f9f33bb56a 100644 --- a/codex-rs/core/tests/suite/agent_websocket.rs +++ b/codex-rs/core/tests/suite/agent_websocket.rs @@ -1,4 +1,5 @@ use anyhow::Result; +use codex_core::features::Feature; use core_test_support::responses::WebSocketConnectionConfig; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -13,6 +14,8 @@ use pretty_assertions::assert_eq; use serde_json::Value; use std::time::Duration; +const WS_V2_BETA_HEADER_VALUE: &str = "responses_websockets=2026-02-06"; + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn websocket_test_codex_shell_chain() -> Result<()> { skip_if_no_network!(Ok(())); @@ -120,3 +123,70 @@ async fn websocket_first_turn_waits_for_inflight_preconnect() -> Result<()> { server.shutdown().await; Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn websocket_v2_test_codex_shell_chain() -> Result<()> { + skip_if_no_network!(Ok(())); + + let call_id = "shell-command-call"; + let server = start_websocket_server(vec![vec![ + vec![ + ev_response_created("resp-1"), + ev_shell_command_call(call_id, "echo websocket"), + ev_completed("resp-1"), + ], + vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ], + ]]) + .await; + + let mut builder = test_codex().with_config(|config| { + config.features.enable(Feature::ResponsesWebsocketsV2); + }); + + let test = builder.build_with_websocket_server(&server).await?; + test.submit_turn("run the echo command").await?; + + let connection = server.single_connection(); + assert_eq!(connection.len(), 2); + + let first = connection + .first() + .expect("missing first request") + .body_json(); + let second = connection + .get(1) + .expect("missing second request") + .body_json(); + + assert_eq!(first["type"].as_str(), Some("response.create")); + assert_eq!(second["type"].as_str(), Some("response.create")); + assert_eq!(second["previous_response_id"].as_str(), Some("resp-1")); + + let create_items = second + .get("input") + .and_then(Value::as_array) + .expect("response.create input array"); + assert!(!create_items.is_empty()); + + let output_item = create_items + .iter() + .find(|item| item.get("type").and_then(Value::as_str) == Some("function_call_output")) + .expect("function_call_output in create"); + assert_eq!( + output_item.get("call_id").and_then(Value::as_str), + Some(call_id) + ); + + let handshake = server.single_handshake(); + assert_eq!( + handshake.header("openai-beta"), + Some(WS_V2_BETA_HEADER_VALUE.to_string()) + ); + + server.shutdown().await; + Ok(()) +} diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index fafa8b0865..2b7753c294 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1259,6 +1259,7 @@ async fn azure_responses_request_includes_store_and_reasoning_ids() { None, Some("test@test.com".to_string()), auth_manager.auth_mode().map(TelemetryAuthMode::from), + "test_originator".to_string(), false, "test".to_string(), SessionSource::Exec, @@ -1273,6 +1274,7 @@ async fn azure_responses_request_includes_store_and_reasoning_ids() { false, false, false, + false, None, ); let mut client_session = client.new_session(); diff --git a/codex-rs/core/tests/suite/client_websockets.rs b/codex-rs/core/tests/suite/client_websockets.rs index 345c213331..a3810a2a3c 100644 --- a/codex-rs/core/tests/suite/client_websockets.rs +++ b/codex-rs/core/tests/suite/client_websockets.rs @@ -42,6 +42,7 @@ use tracing_test::traced_test; const MODEL: &str = "gpt-5.2-codex"; const OPENAI_BETA_HEADER: &str = "OpenAI-Beta"; const OPENAI_BETA_RESPONSES_WEBSOCKETS: &str = "responses_websockets=2026-02-04"; +const WS_V2_BETA_HEADER_VALUE: &str = "responses_websockets=2026-02-06"; struct WebsocketTestHarness { _codex_home: TempDir, @@ -456,6 +457,165 @@ async fn responses_websocket_creates_on_non_prefix() { server.shutdown().await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_websocket_v2_creates_with_previous_response_id_on_prefix() { + skip_if_no_network!(); + + let server = start_websocket_server(vec![vec![ + vec![ev_response_created("resp-1"), ev_completed("resp-1")], + vec![ev_response_created("resp-2"), ev_completed("resp-2")], + ]]) + .await; + + let harness = websocket_harness_with_v2(&server, true).await; + let mut session = harness.client.new_session(); + let prompt_one = prompt_with_input(vec![message_item("hello")]); + let prompt_two = prompt_with_input(vec![message_item("hello"), message_item("second")]); + + stream_until_complete(&mut session, &harness, &prompt_one).await; + stream_until_complete(&mut session, &harness, &prompt_two).await; + + let connection = server.single_connection(); + assert_eq!(connection.len(), 2); + let first = connection.first().expect("missing request").body_json(); + let second = connection.get(1).expect("missing request").body_json(); + + assert_eq!(first["type"].as_str(), Some("response.create")); + assert_eq!(second["type"].as_str(), Some("response.create")); + assert_eq!(second["previous_response_id"].as_str(), Some("resp-1")); + assert_eq!( + second["input"], + serde_json::to_value(&prompt_two.input[1..]).unwrap() + ); + + server.shutdown().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_websocket_v2_after_error_uses_full_create_without_previous_response_id() { + skip_if_no_network!(); + + let server = start_websocket_server(vec![ + vec![ + vec![ev_response_created("resp-1"), ev_completed("resp-1")], + vec![json!({ + "type": "response.failed", + "response": { + "error": { + "code": "invalid_prompt", + "message": "synthetic websocket failure" + } + } + })], + ], + vec![vec![ev_response_created("resp-3"), ev_completed("resp-3")]], + ]) + .await; + + let harness = websocket_harness_with_v2(&server, true).await; + let mut session = harness.client.new_session(); + let prompt_one = prompt_with_input(vec![message_item("hello")]); + let prompt_two = prompt_with_input(vec![message_item("hello"), message_item("second")]); + let prompt_three = prompt_with_input(vec![ + message_item("hello"), + message_item("second"), + message_item("third"), + ]); + + stream_until_complete(&mut session, &harness, &prompt_one).await; + + let mut second_stream = session + .stream( + &prompt_two, + &harness.model_info, + &harness.otel_manager, + harness.effort, + harness.summary, + None, + ) + .await + .expect("websocket stream failed"); + let mut saw_error = false; + while let Some(event) = second_stream.next().await { + if event.is_err() { + saw_error = true; + break; + } + } + assert!(saw_error, "expected second websocket stream to error"); + + stream_until_complete(&mut session, &harness, &prompt_three).await; + + assert_eq!(server.handshakes().len(), 2); + + let connections = server.connections(); + assert_eq!(connections.len(), 2); + let first_connection = connections.first().expect("missing first connection"); + assert_eq!(first_connection.len(), 2); + + let first = first_connection + .first() + .expect("missing first request") + .body_json(); + let second = first_connection + .get(1) + .expect("missing second request") + .body_json(); + let third = connections + .get(1) + .and_then(|connection| connection.first()) + .expect("missing third request") + .body_json(); + + assert_eq!(first["type"].as_str(), Some("response.create")); + assert_eq!(second["type"].as_str(), Some("response.create")); + assert_eq!(second["previous_response_id"].as_str(), Some("resp-1")); + assert_eq!(third["type"].as_str(), Some("response.create")); + assert_eq!(third.get("previous_response_id"), None); + assert_eq!( + third["input"], + serde_json::to_value(&prompt_three.input).unwrap() + ); + + server.shutdown().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_websocket_v2_sets_openai_beta_header() { + skip_if_no_network!(); + + let server = start_websocket_server(vec![vec![vec![ + ev_response_created("resp-1"), + ev_completed("resp-1"), + ]]]) + .await; + + let harness = websocket_harness_with_v2(&server, true).await; + let mut session = harness.client.new_session(); + let prompt = prompt_with_input(vec![message_item("hello")]); + + stream_until_complete(&mut session, &harness, &prompt).await; + + let handshake = server.single_handshake(); + let openai_beta_header = handshake + .header(OPENAI_BETA_HEADER) + .expect("missing OpenAI-Beta header"); + assert!( + openai_beta_header + .split(',') + .map(str::trim) + .any(|value| value == WS_V2_BETA_HEADER_VALUE) + ); + assert!( + !openai_beta_header + .split(',') + .map(str::trim) + .any(|value| value == OPENAI_BETA_RESPONSES_WEBSOCKETS) + ); + + server.shutdown().await; +} + fn message_item(text: &str) -> ResponseItem { ResponseItem::Message { id: None, @@ -498,6 +658,21 @@ async fn websocket_harness(server: &WebSocketTestServer) -> WebsocketTestHarness async fn websocket_harness_with_runtime_metrics( server: &WebSocketTestServer, runtime_metrics_enabled: bool, +) -> WebsocketTestHarness { + websocket_harness_with_options(server, runtime_metrics_enabled, false).await +} + +async fn websocket_harness_with_v2( + server: &WebSocketTestServer, + websocket_v2_enabled: bool, +) -> WebsocketTestHarness { + websocket_harness_with_options(server, false, websocket_v2_enabled).await +} + +async fn websocket_harness_with_options( + server: &WebSocketTestServer, + runtime_metrics_enabled: bool, + websocket_v2_enabled: bool, ) -> WebsocketTestHarness { let provider = websocket_provider(server); let codex_home = TempDir::new().unwrap(); @@ -507,6 +682,9 @@ async fn websocket_harness_with_runtime_metrics( if runtime_metrics_enabled { config.features.enable(Feature::RuntimeMetrics); } + if websocket_v2_enabled { + config.features.enable(Feature::ResponsesWebsocketsV2); + } let config = Arc::new(config); let model_info = ModelsManager::construct_model_info_offline(MODEL, &config); let conversation_id = ThreadId::new(); @@ -524,6 +702,7 @@ async fn websocket_harness_with_runtime_metrics( None, Some("test@test.com".to_string()), auth_manager.auth_mode().map(TelemetryAuthMode::from), + "test_originator".to_string(), false, "test".to_string(), SessionSource::Exec, @@ -538,6 +717,7 @@ async fn websocket_harness_with_runtime_metrics( SessionSource::Exec, config.model_verbosity, true, + websocket_v2_enabled, false, runtime_metrics_enabled, None, diff --git a/codex-rs/core/tests/suite/live_reload.rs b/codex-rs/core/tests/suite/live_reload.rs index e46c77128f..0ebcb332ef 100644 --- a/codex-rs/core/tests/suite/live_reload.rs +++ b/codex-rs/core/tests/suite/live_reload.rs @@ -101,7 +101,7 @@ async fn live_skills_reload_refreshes_skill_cache_after_skill_change() -> Result }); let test = builder.build(&server).await?; - let skill_path = std::fs::canonicalize(test.codex_home_path().join("skills/demo/SKILL.md"))?; + let skill_path = dunce::canonicalize(test.codex_home_path().join("skills/demo/SKILL.md"))?; submit_skill_turn(&test, skill_path.clone(), "please use $demo").await?; let first_request = responses diff --git a/codex-rs/network-proxy/README.md b/codex-rs/network-proxy/README.md index 3d8c20307d..d1df71ca3d 100644 --- a/codex-rs/network-proxy/README.md +++ b/codex-rs/network-proxy/README.md @@ -17,7 +17,7 @@ It enforces an allow/deny policy and a "limited" mode intended for read-only net Example config: ```toml -[network_proxy] +[network] enabled = true proxy_url = "http://127.0.0.1:3128" admin_url = "http://127.0.0.1:8080" @@ -35,7 +35,6 @@ dangerously_allow_non_loopback_proxy = false dangerously_allow_non_loopback_admin = false mode = "full" # default when unset; use "limited" for read-only mode -[network_proxy.policy] # Hosts must match the allowlist (unless denied). # If `allowed_domains` is empty, the proxy blocks requests until an allowlist is configured. allowed_domains = ["*.openai.com"] diff --git a/codex-rs/network-proxy/src/config.rs b/codex-rs/network-proxy/src/config.rs index ae4a450179..e4ef202a45 100644 --- a/codex-rs/network-proxy/src/config.rs +++ b/codex-rs/network-proxy/src/config.rs @@ -11,7 +11,7 @@ use url::Url; #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct NetworkProxyConfig { #[serde(default)] - pub network_proxy: NetworkProxySettings, + pub network: NetworkProxySettings, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -37,7 +37,13 @@ pub struct NetworkProxySettings { #[serde(default)] pub mode: NetworkMode, #[serde(default)] - pub policy: NetworkPolicy, + pub allowed_domains: Vec, + #[serde(default)] + pub denied_domains: Vec, + #[serde(default)] + pub allow_unix_sockets: Vec, + #[serde(default)] + pub allow_local_binding: bool, } impl Default for NetworkProxySettings { @@ -53,23 +59,14 @@ impl Default for NetworkProxySettings { dangerously_allow_non_loopback_proxy: false, dangerously_allow_non_loopback_admin: false, mode: NetworkMode::default(), - policy: NetworkPolicy::default(), + allowed_domains: Vec::new(), + denied_domains: Vec::new(), + allow_unix_sockets: Vec::new(), + allow_local_binding: false, } } } -#[derive(Debug, Clone, Serialize, Deserialize, Default)] -pub struct NetworkPolicy { - #[serde(default)] - pub allowed_domains: Vec, - #[serde(default)] - pub denied_domains: Vec, - #[serde(default)] - pub allow_unix_sockets: Vec, - #[serde(default)] - pub allow_local_binding: bool, -} - #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)] #[serde(rename_all = "lowercase")] pub enum NetworkMode { @@ -142,7 +139,7 @@ pub(crate) fn clamp_bind_addrs( cfg.dangerously_allow_non_loopback_admin, "admin API", ); - if cfg.policy.allow_unix_sockets.is_empty() { + if cfg.allow_unix_sockets.is_empty() { return (http_addr, socks_addr, admin_addr); } @@ -179,26 +176,14 @@ pub struct RuntimeConfig { } pub fn resolve_runtime(cfg: &NetworkProxyConfig) -> Result { - let http_addr = resolve_addr(&cfg.network_proxy.proxy_url, 3128).with_context(|| { - format!( - "invalid network_proxy.proxy_url: {}", - cfg.network_proxy.proxy_url - ) - })?; - let socks_addr = resolve_addr(&cfg.network_proxy.socks_url, 8081).with_context(|| { - format!( - "invalid network_proxy.socks_url: {}", - cfg.network_proxy.socks_url - ) - })?; - let admin_addr = resolve_addr(&cfg.network_proxy.admin_url, 8080).with_context(|| { - format!( - "invalid network_proxy.admin_url: {}", - cfg.network_proxy.admin_url - ) - })?; + let http_addr = resolve_addr(&cfg.network.proxy_url, 3128) + .with_context(|| format!("invalid network.proxy_url: {}", cfg.network.proxy_url))?; + let socks_addr = resolve_addr(&cfg.network.socks_url, 8081) + .with_context(|| format!("invalid network.socks_url: {}", cfg.network.socks_url))?; + let admin_addr = resolve_addr(&cfg.network.admin_url, 8080) + .with_context(|| format!("invalid network.admin_url: {}", cfg.network.admin_url))?; let (http_addr, socks_addr, admin_addr) = - clamp_bind_addrs(http_addr, socks_addr, admin_addr, &cfg.network_proxy); + clamp_bind_addrs(http_addr, socks_addr, admin_addr, &cfg.network); Ok(RuntimeConfig { http_addr, @@ -453,10 +438,7 @@ mod tests { let cfg = NetworkProxySettings { dangerously_allow_non_loopback_proxy: true, dangerously_allow_non_loopback_admin: true, - policy: NetworkPolicy { - allow_unix_sockets: vec!["/tmp/docker.sock".to_string()], - ..Default::default() - }, + allow_unix_sockets: vec!["/tmp/docker.sock".to_string()], ..Default::default() }; let http_addr = "0.0.0.0:3128".parse::().unwrap(); diff --git a/codex-rs/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index 08499490f3..e9bb4a6c94 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -688,7 +688,7 @@ mod tests { use super::*; use crate::config::NetworkMode; - use crate::config::NetworkPolicy; + use crate::config::NetworkProxySettings; use crate::runtime::network_proxy_state_for_policy; use pretty_assertions::assert_eq; use rama_http::Method; @@ -697,7 +697,7 @@ mod tests { #[tokio::test] async fn http_connect_accept_blocks_in_limited_mode() { - let policy = NetworkPolicy { + let policy = NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], ..Default::default() }; diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index aa79acbd1f..b70c3e0b17 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -217,7 +217,7 @@ fn map_decider_decision(decision: NetworkDecision) -> NetworkDecision { mod tests { use super::*; - use crate::config::NetworkPolicy; + use crate::config::NetworkProxySettings; use crate::reasons::REASON_DENIED; use crate::reasons::REASON_NOT_ALLOWED_LOCAL; use crate::state::network_proxy_state_for_policy; @@ -228,7 +228,7 @@ mod tests { #[tokio::test] async fn evaluate_host_policy_invokes_decider_for_not_allowed() { - let state = network_proxy_state_for_policy(NetworkPolicy::default()); + let state = network_proxy_state_for_policy(NetworkProxySettings::default()); let calls = Arc::new(AtomicUsize::new(0)); let decider: Arc = Arc::new({ let calls = calls.clone(); @@ -259,10 +259,10 @@ mod tests { #[tokio::test] async fn evaluate_host_policy_skips_decider_for_denied() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], denied_domains: vec!["blocked.com".to_string()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); let calls = Arc::new(AtomicUsize::new(0)); let decider: Arc = Arc::new({ @@ -299,10 +299,10 @@ mod tests { #[tokio::test] async fn evaluate_host_policy_skips_decider_for_not_allowed_local() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); let calls = Arc::new(AtomicUsize::new(0)); let decider: Arc = Arc::new({ diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index cb09b9959d..ab347c2c03 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -66,7 +66,7 @@ impl NetworkProxyBuilder { self.http_addr.unwrap_or(runtime.http_addr), runtime.socks_addr, self.admin_addr.unwrap_or(runtime.admin_addr), - ¤t_cfg.network_proxy, + ¤t_cfg.network, ); Ok(NetworkProxy { @@ -95,8 +95,8 @@ impl NetworkProxy { pub async fn run(&self) -> Result { let current_cfg = self.state.current_cfg().await?; - if !current_cfg.network_proxy.enabled { - warn!("network_proxy.enabled is false; skipping proxy listeners"); + if !current_cfg.network.enabled { + warn!("network.enabled is false; skipping proxy listeners"); return Ok(NetworkProxyHandle::noop()); } @@ -109,12 +109,12 @@ impl NetworkProxy { self.http_addr, self.policy_decider.clone(), )); - let socks_task = if current_cfg.network_proxy.enable_socks5 { + let socks_task = if current_cfg.network.enable_socks5 { Some(tokio::spawn(socks5::run_socks5( self.state.clone(), self.socks_addr, self.policy_decider.clone(), - current_cfg.network_proxy.enable_socks5_udp, + current_cfg.network.enable_socks5_udp, ))) } else { None diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index 1c29672cfa..e529d9dfce 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -161,15 +161,15 @@ impl NetworkProxyState { self.reload_if_needed().await?; let guard = self.state.read().await; Ok(( - guard.config.network_proxy.policy.allowed_domains.clone(), - guard.config.network_proxy.policy.denied_domains.clone(), + guard.config.network.allowed_domains.clone(), + guard.config.network.denied_domains.clone(), )) } pub async fn enabled(&self) -> Result { self.reload_if_needed().await?; let guard = self.state.read().await; - Ok(guard.config.network_proxy.enabled) + Ok(guard.config.network.enabled) } pub async fn force_reload(&self) -> Result<()> { @@ -209,9 +209,9 @@ impl NetworkProxyState { ( guard.deny_set.clone(), guard.allow_set.clone(), - guard.config.network_proxy.policy.allow_local_binding, - guard.config.network_proxy.policy.allowed_domains.is_empty(), - guard.config.network_proxy.policy.allowed_domains.clone(), + guard.config.network.allow_local_binding, + guard.config.network.allowed_domains.is_empty(), + guard.config.network.allowed_domains.clone(), ) }; @@ -305,7 +305,7 @@ impl NetworkProxyState { Err(_) => return Ok(false), }; let requested_canonical = std::fs::canonicalize(requested_abs.as_path()).ok(); - for allowed in &guard.config.network_proxy.policy.allow_unix_sockets { + for allowed in &guard.config.network.allow_unix_sockets { if allowed == path { return Ok(true); } @@ -327,19 +327,19 @@ impl NetworkProxyState { pub async fn method_allowed(&self, method: &str) -> Result { self.reload_if_needed().await?; let guard = self.state.read().await; - Ok(guard.config.network_proxy.mode.allows_method(method)) + Ok(guard.config.network.mode.allows_method(method)) } pub async fn allow_upstream_proxy(&self) -> Result { self.reload_if_needed().await?; let guard = self.state.read().await; - Ok(guard.config.network_proxy.allow_upstream_proxy) + Ok(guard.config.network.allow_upstream_proxy) } pub async fn network_mode(&self) -> Result { self.reload_if_needed().await?; let guard = self.state.read().await; - Ok(guard.config.network_proxy.mode) + Ok(guard.config.network.mode) } pub async fn set_network_mode(&self, mode: NetworkMode) -> Result<()> { @@ -348,19 +348,19 @@ impl NetworkProxyState { let (candidate, constraints) = { let guard = self.state.read().await; let mut candidate = guard.config.clone(); - candidate.network_proxy.mode = mode; + candidate.network.mode = mode; (candidate, guard.constraints.clone()) }; validate_policy_against_constraints(&candidate, &constraints) - .context("network_proxy.mode constrained by managed config")?; + .context("network.mode constrained by managed config")?; let mut guard = self.state.write().await; if guard.constraints != constraints { drop(guard); continue; } - guard.config.network_proxy.mode = mode; + guard.config.network.mode = mode; info!("updated network mode to {mode:?}"); return Ok(()); } @@ -417,13 +417,13 @@ async fn host_resolves_to_non_public_ip(host: &str, port: u16) -> bool { fn log_policy_changes(previous: &NetworkProxyConfig, next: &NetworkProxyConfig) { log_domain_list_changes( "allowlist", - &previous.network_proxy.policy.allowed_domains, - &next.network_proxy.policy.allowed_domains, + &previous.network.allowed_domains, + &next.network.allowed_domains, ); log_domain_list_changes( "denylist", - &previous.network_proxy.policy.denied_domains, - &next.network_proxy.policy.denied_domains, + &previous.network.denied_domains, + &next.network.denied_domains, ); } @@ -483,21 +483,14 @@ fn unix_timestamp() -> i64 { #[cfg(test)] pub(crate) fn network_proxy_state_for_policy( - policy: crate::config::NetworkPolicy, + mut network: crate::config::NetworkProxySettings, ) -> NetworkProxyState { - let config = NetworkProxyConfig { - network_proxy: crate::config::NetworkProxySettings { - enabled: true, - mode: NetworkMode::Full, - policy, - ..crate::config::NetworkProxySettings::default() - }, - }; + network.enabled = true; + network.mode = NetworkMode::Full; + let config = NetworkProxyConfig { network }; - let allow_set = - crate::policy::compile_globset(&config.network_proxy.policy.allowed_domains).unwrap(); - let deny_set = - crate::policy::compile_globset(&config.network_proxy.policy.denied_domains).unwrap(); + let allow_set = crate::policy::compile_globset(&config.network.allowed_domains).unwrap(); + let deny_set = crate::policy::compile_globset(&config.network.denied_domains).unwrap(); let state = ConfigState { config, @@ -518,7 +511,6 @@ pub(crate) fn network_proxy_state_for_policy( mod tests { use super::*; - use crate::config::NetworkPolicy; use crate::config::NetworkProxyConfig; use crate::config::NetworkProxySettings; use crate::policy::compile_globset; @@ -528,10 +520,10 @@ mod tests { #[tokio::test] async fn host_blocked_denied_wins_over_allowed() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], denied_domains: vec!["example.com".to_string()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -542,9 +534,9 @@ mod tests { #[tokio::test] async fn host_blocked_requires_allowlist_match() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -561,9 +553,9 @@ mod tests { #[tokio::test] async fn host_blocked_subdomain_wildcards_exclude_apex() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["*.openai.com".to_string()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -578,10 +570,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_loopback_when_local_binding_disabled() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -596,10 +588,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_loopback_when_allowlist_is_wildcard() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["*".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -610,10 +602,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_private_ip_literal_when_allowlist_is_wildcard() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["*".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -624,10 +616,10 @@ mod tests { #[tokio::test] async fn host_blocked_allows_loopback_when_explicitly_allowlisted_and_local_binding_disabled() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["localhost".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -638,10 +630,10 @@ mod tests { #[tokio::test] async fn host_blocked_allows_private_ip_literal_when_explicitly_allowlisted() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["10.0.0.1".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -652,10 +644,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_scoped_ipv6_literal_when_not_allowlisted() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -666,10 +658,10 @@ mod tests { #[tokio::test] async fn host_blocked_allows_scoped_ipv6_literal_when_explicitly_allowlisted() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["fe80::1%lo0".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -680,10 +672,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_private_ip_literals_when_local_binding_disabled() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -694,10 +686,10 @@ mod tests { #[tokio::test] async fn host_blocked_rejects_loopback_when_allowlist_empty() { - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec![], allow_local_binding: false, - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert_eq!( @@ -714,12 +706,9 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, - policy: NetworkPolicy { - allowed_domains: vec!["example.com".to_string(), "evil.com".to_string()], - ..NetworkPolicy::default() - }, + allowed_domains: vec!["example.com".to_string(), "evil.com".to_string()], ..NetworkProxySettings::default() }, }; @@ -735,7 +724,7 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, mode: NetworkMode::Full, ..NetworkProxySettings::default() @@ -753,12 +742,9 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, - policy: NetworkPolicy { - allowed_domains: vec!["api.example.com".to_string()], - ..NetworkPolicy::default() - }, + allowed_domains: vec!["api.example.com".to_string()], ..NetworkProxySettings::default() }, }; @@ -774,12 +760,9 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, - policy: NetworkPolicy { - allowed_domains: vec!["**.example.com".to_string()], - ..NetworkPolicy::default() - }, + allowed_domains: vec!["**.example.com".to_string()], ..NetworkProxySettings::default() }, }; @@ -795,12 +778,9 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, - policy: NetworkPolicy { - denied_domains: vec![], - ..NetworkPolicy::default() - }, + denied_domains: vec![], ..NetworkProxySettings::default() }, }; @@ -816,7 +796,7 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, ..NetworkProxySettings::default() }, @@ -833,12 +813,9 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, - policy: NetworkPolicy { - allow_local_binding: true, - ..NetworkPolicy::default() - }, + allow_local_binding: true, ..NetworkProxySettings::default() }, }; @@ -854,7 +831,7 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, dangerously_allow_non_loopback_admin: true, ..NetworkProxySettings::default() @@ -872,7 +849,7 @@ mod tests { }; let config = NetworkProxyConfig { - network_proxy: NetworkProxySettings { + network: NetworkProxySettings { enabled: true, dangerously_allow_non_loopback_admin: true, ..NetworkProxySettings::default() @@ -935,10 +912,10 @@ mod tests { #[tokio::test] async fn unix_socket_allowlist_is_respected_on_macos() { let socket_path = "/tmp/example.sock".to_string(); - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_unix_sockets: vec![socket_path.clone()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert!(state.is_unix_socket_allowed(&socket_path).await.unwrap()); @@ -970,10 +947,10 @@ mod tests { let real_s = real.to_str().unwrap().to_string(); let link_s = link.to_str().unwrap().to_string(); - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_unix_sockets: vec![real_s], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert!(state.is_unix_socket_allowed(&link_s).await.unwrap()); @@ -983,10 +960,10 @@ mod tests { #[tokio::test] async fn unix_socket_allowlist_is_rejected_on_non_macos() { let socket_path = "/tmp/example.sock".to_string(); - let state = network_proxy_state_for_policy(NetworkPolicy { + let state = network_proxy_state_for_policy(NetworkProxySettings { allowed_domains: vec!["example.com".to_string()], allow_unix_sockets: vec![socket_path.clone()], - ..NetworkPolicy::default() + ..NetworkProxySettings::default() }); assert!(!state.is_unix_socket_allowed(&socket_path).await.unwrap()); diff --git a/codex-rs/network-proxy/src/state.rs b/codex-rs/network-proxy/src/state.rs index 20c328c750..179d33dec4 100644 --- a/codex-rs/network-proxy/src/state.rs +++ b/codex-rs/network-proxy/src/state.rs @@ -8,7 +8,6 @@ use anyhow::Context; use anyhow::Result; use codex_app_server_protocol::ConfigLayerSource; use codex_core::config::CONFIG_TOML_FILE; -use codex_core::config::Constrained; use codex_core::config::ConstraintError; use codex_core::config::find_codex_home; use codex_core::config_loader::CloudRequirementsLoader; @@ -56,8 +55,8 @@ pub(crate) async fn build_config_state() -> Result { let constraints = enforce_trusted_constraints(&config_layer_stack, &config)?; let layer_mtimes = collect_layer_mtimes(&config_layer_stack); - let deny_set = compile_globset(&config.network_proxy.policy.denied_domains)?; - let allow_set = compile_globset(&config.network_proxy.policy.allowed_domains)?; + let deny_set = compile_globset(&config.network.denied_domains)?; + let allow_set = compile_globset(&config.network.allowed_domains)?; Ok(ConfigState { config, allow_set, @@ -94,22 +93,16 @@ fn collect_layer_mtimes(stack: &ConfigLayerStack) -> Vec { #[derive(Debug, Default, Deserialize)] struct PartialConfig { #[serde(default)] - network_proxy: PartialNetworkProxyConfig, + network: PartialNetworkConfig, } #[derive(Debug, Default, Deserialize)] -struct PartialNetworkProxyConfig { +struct PartialNetworkConfig { enabled: Option, mode: Option, allow_upstream_proxy: Option, dangerously_allow_non_loopback_proxy: Option, dangerously_allow_non_loopback_admin: Option, - #[serde(default)] - policy: PartialNetworkPolicy, -} - -#[derive(Debug, Default, Deserialize)] -struct PartialNetworkPolicy { #[serde(default)] allowed_domains: Option>, #[serde(default)] @@ -137,13 +130,13 @@ fn enforce_trusted_constraints( layers: &codex_core::config_loader::ConfigLayerStack, config: &NetworkProxyConfig, ) -> Result { - let constraints = network_proxy_constraints_from_trusted_layers(layers)?; + let constraints = network_constraints_from_trusted_layers(layers)?; validate_policy_against_constraints(config, &constraints) .context("network proxy constraints")?; Ok(constraints) } -fn network_proxy_constraints_from_trusted_layers( +fn network_constraints_from_trusted_layers( layers: &codex_core::config_loader::ConfigLayerStack, ) -> Result { let mut constraints = NetworkProxyConstraints::default(); @@ -163,38 +156,38 @@ fn network_proxy_constraints_from_trusted_layers( .try_into() .context("failed to deserialize trusted config layer")?; - if let Some(enabled) = partial.network_proxy.enabled { + if let Some(enabled) = partial.network.enabled { constraints.enabled = Some(enabled); } - if let Some(mode) = partial.network_proxy.mode { + if let Some(mode) = partial.network.mode { constraints.mode = Some(mode); } - if let Some(allow_upstream_proxy) = partial.network_proxy.allow_upstream_proxy { + if let Some(allow_upstream_proxy) = partial.network.allow_upstream_proxy { constraints.allow_upstream_proxy = Some(allow_upstream_proxy); } if let Some(dangerously_allow_non_loopback_proxy) = - partial.network_proxy.dangerously_allow_non_loopback_proxy + partial.network.dangerously_allow_non_loopback_proxy { constraints.dangerously_allow_non_loopback_proxy = Some(dangerously_allow_non_loopback_proxy); } if let Some(dangerously_allow_non_loopback_admin) = - partial.network_proxy.dangerously_allow_non_loopback_admin + partial.network.dangerously_allow_non_loopback_admin { constraints.dangerously_allow_non_loopback_admin = Some(dangerously_allow_non_loopback_admin); } - if let Some(allowed_domains) = partial.network_proxy.policy.allowed_domains { + if let Some(allowed_domains) = partial.network.allowed_domains { constraints.allowed_domains = Some(allowed_domains); } - if let Some(denied_domains) = partial.network_proxy.policy.denied_domains { + if let Some(denied_domains) = partial.network.denied_domains { constraints.denied_domains = Some(denied_domains); } - if let Some(allow_unix_sockets) = partial.network_proxy.policy.allow_unix_sockets { + if let Some(allow_unix_sockets) = partial.network.allow_unix_sockets { constraints.allow_unix_sockets = Some(allow_unix_sockets); } - if let Some(allow_local_binding) = partial.network_proxy.policy.allow_local_binding { + if let Some(allow_local_binding) = partial.network.allow_local_binding { constraints.allow_local_binding = Some(allow_local_binding); } } @@ -227,12 +220,19 @@ pub(crate) fn validate_policy_against_constraints( } } - let enabled = config.network_proxy.enabled; + fn validate( + candidate: T, + validator: impl FnOnce(&T) -> std::result::Result<(), ConstraintError>, + ) -> std::result::Result<(), ConstraintError> { + validator(&candidate) + } + + let enabled = config.network.enabled; if let Some(max_enabled) = constraints.enabled { - let _ = Constrained::new(enabled, move |candidate| { + validate(enabled, move |candidate| { if *candidate && !max_enabled { Err(invalid_value( - "network_proxy.enabled", + "network.enabled", "true", "false (disabled by managed config)", )) @@ -243,10 +243,10 @@ pub(crate) fn validate_policy_against_constraints( } if let Some(max_mode) = constraints.mode { - let _ = Constrained::new(config.network_proxy.mode, move |candidate| { + validate(config.network.mode, move |candidate| { if network_mode_rank(*candidate) > network_mode_rank(max_mode) { Err(invalid_value( - "network_proxy.mode", + "network.mode", format!("{candidate:?}"), format!("{max_mode:?} or more restrictive"), )) @@ -257,14 +257,14 @@ pub(crate) fn validate_policy_against_constraints( } let allow_upstream_proxy = constraints.allow_upstream_proxy; - let _ = Constrained::new( - config.network_proxy.allow_upstream_proxy, + validate( + config.network.allow_upstream_proxy, move |candidate| match allow_upstream_proxy { Some(true) | None => Ok(()), Some(false) => { if *candidate { Err(invalid_value( - "network_proxy.allow_upstream_proxy", + "network.allow_upstream_proxy", "true", "false (disabled by managed config)", )) @@ -276,14 +276,14 @@ pub(crate) fn validate_policy_against_constraints( )?; let allow_non_loopback_admin = constraints.dangerously_allow_non_loopback_admin; - let _ = Constrained::new( - config.network_proxy.dangerously_allow_non_loopback_admin, + validate( + config.network.dangerously_allow_non_loopback_admin, move |candidate| match allow_non_loopback_admin { Some(true) | None => Ok(()), Some(false) => { if *candidate { Err(invalid_value( - "network_proxy.dangerously_allow_non_loopback_admin", + "network.dangerously_allow_non_loopback_admin", "true", "false (disabled by managed config)", )) @@ -295,14 +295,14 @@ pub(crate) fn validate_policy_against_constraints( )?; let allow_non_loopback_proxy = constraints.dangerously_allow_non_loopback_proxy; - let _ = Constrained::new( - config.network_proxy.dangerously_allow_non_loopback_proxy, + validate( + config.network.dangerously_allow_non_loopback_proxy, move |candidate| match allow_non_loopback_proxy { Some(true) | None => Ok(()), Some(false) => { if *candidate { Err(invalid_value( - "network_proxy.dangerously_allow_non_loopback_proxy", + "network.dangerously_allow_non_loopback_proxy", "true", "false (disabled by managed config)", )) @@ -314,20 +314,17 @@ pub(crate) fn validate_policy_against_constraints( )?; if let Some(allow_local_binding) = constraints.allow_local_binding { - let _ = Constrained::new( - config.network_proxy.policy.allow_local_binding, - move |candidate| { - if *candidate && !allow_local_binding { - Err(invalid_value( - "network_proxy.policy.allow_local_binding", - "true", - "false (disabled by managed config)", - )) - } else { - Ok(()) - } - }, - )?; + validate(config.network.allow_local_binding, move |candidate| { + if *candidate && !allow_local_binding { + Err(invalid_value( + "network.allow_local_binding", + "true", + "false (disabled by managed config)", + )) + } else { + Ok(()) + } + })?; } if let Some(allowed_domains) = &constraints.allowed_domains { @@ -335,30 +332,27 @@ pub(crate) fn validate_policy_against_constraints( .iter() .map(|entry| DomainPattern::parse_for_constraints(entry)) .collect(); - let _ = Constrained::new( - config.network_proxy.policy.allowed_domains.clone(), - move |candidate| { - let mut invalid = Vec::new(); - for entry in candidate { - let candidate_pattern = DomainPattern::parse_for_constraints(entry); - if !managed_patterns - .iter() - .any(|managed| managed.allows(&candidate_pattern)) - { - invalid.push(entry.clone()); - } + validate(config.network.allowed_domains.clone(), move |candidate| { + let mut invalid = Vec::new(); + for entry in candidate { + let candidate_pattern = DomainPattern::parse_for_constraints(entry); + if !managed_patterns + .iter() + .any(|managed| managed.allows(&candidate_pattern)) + { + invalid.push(entry.clone()); } - if invalid.is_empty() { - Ok(()) - } else { - Err(invalid_value( - "network_proxy.policy.allowed_domains", - format!("{invalid:?}"), - "subset of managed allowed_domains", - )) - } - }, - )?; + } + if invalid.is_empty() { + Ok(()) + } else { + Err(invalid_value( + "network.allowed_domains", + format!("{invalid:?}"), + "subset of managed allowed_domains", + )) + } + })?; } if let Some(denied_domains) = &constraints.denied_domains { @@ -366,27 +360,24 @@ pub(crate) fn validate_policy_against_constraints( .iter() .map(|s| s.to_ascii_lowercase()) .collect(); - let _ = Constrained::new( - config.network_proxy.policy.denied_domains.clone(), - move |candidate| { - let candidate_set: HashSet = - candidate.iter().map(|s| s.to_ascii_lowercase()).collect(); - let missing: Vec = required_set - .iter() - .filter(|entry| !candidate_set.contains(*entry)) - .cloned() - .collect(); - if missing.is_empty() { - Ok(()) - } else { - Err(invalid_value( - "network_proxy.policy.denied_domains", - "missing managed denied_domains entries", - format!("{missing:?}"), - )) - } - }, - )?; + validate(config.network.denied_domains.clone(), move |candidate| { + let candidate_set: HashSet = + candidate.iter().map(|s| s.to_ascii_lowercase()).collect(); + let missing: Vec = required_set + .iter() + .filter(|entry| !candidate_set.contains(*entry)) + .cloned() + .collect(); + if missing.is_empty() { + Ok(()) + } else { + Err(invalid_value( + "network.denied_domains", + "missing managed denied_domains entries", + format!("{missing:?}"), + )) + } + })?; } if let Some(allow_unix_sockets) = &constraints.allow_unix_sockets { @@ -394,8 +385,8 @@ pub(crate) fn validate_policy_against_constraints( .iter() .map(|s| s.to_ascii_lowercase()) .collect(); - let _ = Constrained::new( - config.network_proxy.policy.allow_unix_sockets.clone(), + validate( + config.network.allow_unix_sockets.clone(), move |candidate| { let mut invalid = Vec::new(); for entry in candidate { @@ -407,7 +398,7 @@ pub(crate) fn validate_policy_against_constraints( Ok(()) } else { Err(invalid_value( - "network_proxy.policy.allow_unix_sockets", + "network.allow_unix_sockets", format!("{invalid:?}"), "subset of managed allow_unix_sockets", )) diff --git a/codex-rs/otel/src/lib.rs b/codex-rs/otel/src/lib.rs index a86e5ef525..cbedc4836c 100644 --- a/codex-rs/otel/src/lib.rs +++ b/codex-rs/otel/src/lib.rs @@ -44,6 +44,7 @@ pub struct OtelEventMetadata { pub(crate) auth_mode: Option, pub(crate) account_id: Option, pub(crate) account_email: Option, + pub(crate) originator: String, pub(crate) session_source: String, pub(crate) model: String, pub(crate) slug: String, diff --git a/codex-rs/otel/src/traces/otel_manager.rs b/codex-rs/otel/src/traces/otel_manager.rs index c54c64c6cb..8c0f5d2950 100644 --- a/codex-rs/otel/src/traces/otel_manager.rs +++ b/codex-rs/otel/src/traces/otel_manager.rs @@ -66,6 +66,7 @@ impl OtelManager { account_id: Option, account_email: Option, auth_mode: Option, + originator: String, log_user_prompts: bool, terminal_type: String, session_source: SessionSource, @@ -76,6 +77,7 @@ impl OtelManager { auth_mode: auth_mode.map(|m| m.to_string()), account_id, account_email, + originator, session_source: session_source.to_string(), model: model.to_owned(), slug: slug.to_owned(), @@ -134,6 +136,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -198,6 +201,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -229,6 +233,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -330,6 +335,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -407,6 +413,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -440,6 +447,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -455,6 +463,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -478,6 +487,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -503,6 +513,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -538,6 +549,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -562,6 +574,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -617,6 +630,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, @@ -654,6 +668,7 @@ impl OtelManager { conversation.id = %self.metadata.conversation_id, app.version = %self.metadata.app_version, auth_mode = self.metadata.auth_mode, + originator = %self.metadata.originator, user.account_id = self.metadata.account_id, user.email = self.metadata.account_email, terminal.type = %self.metadata.terminal_type, diff --git a/codex-rs/otel/tests/suite/manager_metrics.rs b/codex-rs/otel/tests/suite/manager_metrics.rs index 164bb43b3a..3dbc334417 100644 --- a/codex-rs/otel/tests/suite/manager_metrics.rs +++ b/codex-rs/otel/tests/suite/manager_metrics.rs @@ -23,6 +23,7 @@ fn manager_attaches_metadata_tags_to_metrics() -> Result<()> { Some("account-id".to_string()), None, Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), true, "tty".to_string(), SessionSource::Cli, @@ -77,6 +78,7 @@ fn manager_allows_disabling_metadata_tags() -> Result<()> { Some("account-id".to_string()), None, Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), true, "tty".to_string(), SessionSource::Cli, diff --git a/codex-rs/otel/tests/suite/runtime_summary.rs b/codex-rs/otel/tests/suite/runtime_summary.rs index 79c6b258e7..0737e0797f 100644 --- a/codex-rs/otel/tests/suite/runtime_summary.rs +++ b/codex-rs/otel/tests/suite/runtime_summary.rs @@ -27,6 +27,7 @@ fn runtime_metrics_summary_collects_tool_api_and_streaming_metrics() -> Result<( Some("account-id".to_string()), None, Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), true, "tty".to_string(), SessionSource::Cli, diff --git a/codex-rs/otel/tests/suite/snapshot.rs b/codex-rs/otel/tests/suite/snapshot.rs index aa7990d062..0c6ef7e914 100644 --- a/codex-rs/otel/tests/suite/snapshot.rs +++ b/codex-rs/otel/tests/suite/snapshot.rs @@ -76,6 +76,7 @@ fn manager_snapshot_metrics_collects_without_shutdown() -> Result<()> { Some("account-id".to_string()), None, Some(TelemetryAuthMode::ApiKey), + "test_originator".to_string(), true, "tty".to_string(), SessionSource::Cli, diff --git a/codex-rs/protocol/src/items.rs b/codex-rs/protocol/src/items.rs index 9a387a9d22..35bed2ab47 100644 --- a/codex-rs/protocol/src/items.rs +++ b/codex-rs/protocol/src/items.rs @@ -1,3 +1,4 @@ +use crate::models::MessagePhase; use crate::models::WebSearchAction; use crate::protocol::AgentMessageEvent; use crate::protocol::AgentReasoningEvent; @@ -40,9 +41,21 @@ pub enum AgentMessageContent { } #[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)] +/// Assistant-authored message payload used in turn-item streams. +/// +/// `phase` is optional because not all providers/models emit it. Consumers +/// should use it when present, but retain legacy completion semantics when it +/// is `None`. pub struct AgentMessageItem { pub id: String, pub content: Vec, + /// Optional phase metadata carried through from `ResponseItem::Message`. + /// + /// This is currently used by TUI rendering to distinguish mid-turn + /// commentary from a final answer and avoid status-indicator jitter. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub phase: Option, } #[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)] @@ -172,10 +185,13 @@ impl AgentMessageItem { Self { id: uuid::Uuid::new_v4().to_string(), content: content.to_vec(), + phase: None, } } pub fn as_legacy_events(&self) -> Vec { + // Legacy events only preserve visible assistant text; `phase` has no + // representation in the v1 event stream. self.content .iter() .map(|c| match c { diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index ae38a7890c..7f33199702 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -74,8 +74,17 @@ pub enum ContentItem { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] +/// Classifies an assistant message as interim commentary or final answer text. +/// +/// Providers do not emit this consistently, so callers must treat `None` as +/// "phase unknown" and keep compatibility behavior for legacy models. pub enum MessagePhase { + /// Mid-turn assistant text (for example preamble/progress narration). + /// + /// Additional tool calls or assistant output may follow before turn + /// completion. Commentary, + /// The assistant's terminal answer text for the current turn. FinalAnswer, } @@ -93,7 +102,8 @@ pub enum ResponseItem { #[ts(optional)] end_turn: Option, // Optional output-message phase (for example: "commentary", "final_answer"). - // Do not use directly; availability can vary by provider and model. + // Availability varies by provider/model, so downstream consumers must + // preserve fallback behavior when this is absent. #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] phase: Option, diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index 2b8752fcea..951bb0dd1a 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -15,6 +15,7 @@ axum = { workspace = true, default-features = false, features = [ ] } codex-keyring-store = { workspace = true } codex-protocol = { workspace = true } +codex-utils-pty = { workspace = true } codex-utils-home-dir = { workspace = true } futures = { workspace = true, default-features = false, features = ["std"] } keyring = { workspace = true, features = ["crypto-rust"] } diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 12b4b52b91..a2dc42ad3b 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -60,7 +60,10 @@ use crate::utils::create_env_for_mcp_server; use crate::utils::run_with_timeout; enum PendingTransport { - ChildProcess(TokioChildProcess), + ChildProcess { + transport: TokioChildProcess, + process_group_guard: Option, + }, StreamableHttp { transport: StreamableHttpClientTransport, }, @@ -75,11 +78,71 @@ enum ClientState { transport: Option, }, Ready { + _process_group_guard: Option, service: Arc>, oauth: Option, }, } +#[cfg(unix)] +const PROCESS_GROUP_TERM_GRACE_PERIOD: Duration = Duration::from_secs(2); + +#[cfg(unix)] +struct ProcessGroupGuard { + process_group_id: u32, +} + +#[cfg(not(unix))] +struct ProcessGroupGuard; + +impl ProcessGroupGuard { + fn new(process_group_id: u32) -> Self { + #[cfg(unix)] + { + Self { process_group_id } + } + #[cfg(not(unix))] + { + let _ = process_group_id; + Self + } + } + + #[cfg(unix)] + fn maybe_terminate_process_group(&self) { + let process_group_id = self.process_group_id; + let should_escalate = + match codex_utils_pty::process_group::terminate_process_group(process_group_id) { + Ok(exists) => exists, + Err(error) => { + warn!("Failed to terminate MCP process group {process_group_id}: {error}"); + false + } + }; + if should_escalate { + std::thread::spawn(move || { + std::thread::sleep(PROCESS_GROUP_TERM_GRACE_PERIOD); + if let Err(error) = + codex_utils_pty::process_group::kill_process_group(process_group_id) + { + warn!("Failed to kill MCP process group {process_group_id}: {error}"); + } + }); + } + } + + #[cfg(not(unix))] + fn maybe_terminate_process_group(&self) {} +} + +impl Drop for ProcessGroupGuard { + fn drop(&mut self) { + if cfg!(unix) { + self.maybe_terminate_process_group(); + } + } +} + pub type Elicitation = CreateElicitationRequestParam; pub type ElicitationResponse = CreateElicitationResult; @@ -129,6 +192,8 @@ impl RmcpClient { .env_clear() .envs(envs) .args(&args); + #[cfg(unix)] + command.process_group(0); if let Some(cwd) = cwd { command.current_dir(cwd); } @@ -136,6 +201,7 @@ impl RmcpClient { let (transport, stderr) = TokioChildProcess::builder(command) .stderr(Stdio::piped()) .spawn()?; + let process_group_guard = transport.id().map(ProcessGroupGuard::new); if let Some(stderr) = stderr { tokio::spawn(async move { @@ -157,7 +223,10 @@ impl RmcpClient { Ok(Self { state: Mutex::new(ClientState::Connecting { - transport: Some(PendingTransport::ChildProcess(transport)), + transport: Some(PendingTransport::ChildProcess { + transport, + process_group_guard, + }), }), }) } @@ -226,17 +295,22 @@ impl RmcpClient { ) -> Result { let client_handler = LoggingClientHandler::new(params.clone(), send_elicitation); - let (transport, oauth_persistor) = { + let (transport, oauth_persistor, process_group_guard) = { let mut guard = self.state.lock().await; match &mut *guard { ClientState::Connecting { transport } => match transport.take() { - Some(PendingTransport::ChildProcess(transport)) => ( + Some(PendingTransport::ChildProcess { + transport, + process_group_guard, + }) => ( service::serve_client(client_handler.clone(), transport).boxed(), None, + process_group_guard, ), Some(PendingTransport::StreamableHttp { transport }) => ( service::serve_client(client_handler.clone(), transport).boxed(), None, + None, ), Some(PendingTransport::StreamableHttpWithOAuth { transport, @@ -244,6 +318,7 @@ impl RmcpClient { }) => ( service::serve_client(client_handler.clone(), transport).boxed(), Some(oauth_persistor), + None, ), None => return Err(anyhow!("client already initializing")), }, @@ -270,6 +345,7 @@ impl RmcpClient { { let mut guard = self.state.lock().await; *guard = ClientState::Ready { + _process_group_guard: process_group_guard, service: Arc::new(service), oauth: oauth_persistor.clone(), }; @@ -448,7 +524,7 @@ impl RmcpClient { match &*guard { ClientState::Ready { oauth: Some(runtime), - service: _, + .. } => Some(runtime.clone()), _ => None, } diff --git a/codex-rs/rmcp-client/tests/process_group_cleanup.rs b/codex-rs/rmcp-client/tests/process_group_cleanup.rs new file mode 100644 index 0000000000..8ed9ba305a --- /dev/null +++ b/codex-rs/rmcp-client/tests/process_group_cleanup.rs @@ -0,0 +1,88 @@ +#![cfg(unix)] + +use std::collections::HashMap; +use std::ffi::OsString; +use std::fs; +use std::path::Path; +use std::time::Duration; + +use anyhow::Context; +use anyhow::Result; +use codex_rmcp_client::RmcpClient; + +fn process_exists(pid: u32) -> bool { + std::process::Command::new("kill") + .arg("-0") + .arg(pid.to_string()) + .stderr(std::process::Stdio::null()) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + +async fn wait_for_pid_file(path: &Path) -> Result { + for _ in 0..50 { + match fs::read_to_string(path) { + Ok(content) => { + let pid = content + .trim() + .parse::() + .with_context(|| format!("failed to parse pid from {}", path.display()))?; + return Ok(pid); + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + tokio::time::sleep(Duration::from_millis(100)).await; + } + Err(error) => { + return Err(error).with_context(|| format!("failed to read {}", path.display())); + } + } + } + + anyhow::bail!("timed out waiting for child pid file at {}", path.display()); +} + +async fn wait_for_process_exit(pid: u32) -> Result<()> { + for _ in 0..50 { + if !process_exists(pid) { + return Ok(()); + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + + anyhow::bail!("process {pid} still running after timeout"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn drop_kills_wrapper_process_group() -> Result<()> { + let temp_dir = tempfile::tempdir()?; + let child_pid_file = temp_dir.path().join("child.pid"); + let child_pid_file_str = child_pid_file.to_string_lossy().into_owned(); + + let client = RmcpClient::new_stdio_client( + OsString::from("/bin/sh"), + vec![ + OsString::from("-c"), + OsString::from( + "sleep 300 & child_pid=$!; echo \"$child_pid\" > \"$CHILD_PID_FILE\"; cat >/dev/null", + ), + ], + Some(HashMap::from([( + "CHILD_PID_FILE".to_string(), + child_pid_file_str, + )])), + &[], + None, + ) + .await?; + + let grandchild_pid = wait_for_pid_file(&child_pid_file).await?; + assert!( + process_exists(grandchild_pid), + "expected grandchild process {grandchild_pid} to be running before dropping client" + ); + + drop(client); + + wait_for_process_exit(grandchild_pid).await +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 718615237d..ea41749e92 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -986,6 +986,7 @@ impl App { auth_ref.and_then(CodexAuth::get_account_id), auth_ref.and_then(CodexAuth::get_account_email), auth_mode, + codex_core::default_client::originator().value, config.otel.log_user_prompt, codex_core::terminal::user_agent(), SessionSource::Cli, @@ -2789,6 +2790,7 @@ mod tests { None, None, None, + "test_originator".to_string(), false, "test".to_string(), SessionSource::Cli, diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 4afbfea01a..5ce5ebafb4 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -157,6 +157,7 @@ use crate::app_event::AppEvent; use crate::app_event::ConnectorsSnapshot; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::LocalImageAttachment; +use crate::bottom_pane::MentionBinding; use crate::bottom_pane::textarea::TextArea; use crate::bottom_pane::textarea::TextAreaState; use crate::clipboard_paste::normalize_pasted_path; @@ -290,7 +291,8 @@ pub(crate) struct ChatComposer { skills: Option>, connectors_snapshot: Option, dismissed_mention_popup_token: Option, - mention_paths: HashMap, + mention_bindings: HashMap, + recent_submission_mention_bindings: Vec, /// When enabled, `Enter` submits immediately and `Tab` requests queuing behavior. steer_enabled: bool, collaboration_modes_enabled: bool, @@ -309,6 +311,12 @@ struct FooterFlash { expires_at: Instant, } +#[derive(Clone, Debug)] +struct ComposerMentionBinding { + mention: String, + path: String, +} + /// Popup state – at most one can be visible at any time. enum ActivePopup { None, @@ -382,7 +390,8 @@ impl ChatComposer { skills: None, connectors_snapshot: None, dismissed_mention_popup_token: None, - mention_paths: HashMap::new(), + mention_bindings: HashMap::new(), + recent_submission_mention_bindings: Vec::new(), steer_enabled: false, collaboration_modes_enabled: false, config, @@ -414,8 +423,21 @@ impl ChatComposer { self.connectors_snapshot = connectors_snapshot; } - pub(crate) fn take_mention_paths(&mut self) -> HashMap { - std::mem::take(&mut self.mention_paths) + pub(crate) fn take_mention_bindings(&mut self) -> Vec { + let elements = self.current_mention_elements(); + let mut ordered = Vec::new(); + for (id, mention) in elements { + if let Some(binding) = self.mention_bindings.remove(&id) + && binding.mention == mention + { + ordered.push(MentionBinding { + mention: binding.mention, + path: binding.path, + }); + } + } + self.mention_bindings.clear(); + ordered } /// Enables or disables "Steer" behavior for submission keys. @@ -518,7 +540,12 @@ impl ChatComposer { }; // Persistent ↑/↓ history is text-only (backwards-compatible and avoids persisting // attachments), but local in-session ↑/↓ history can rehydrate elements and image paths. - self.set_text_content(entry.text, entry.text_elements, entry.local_image_paths); + self.set_text_content_with_mention_bindings( + entry.text, + entry.text_elements, + entry.local_image_paths, + entry.mention_bindings, + ); true } @@ -732,41 +759,41 @@ impl ChatComposer { /// This is the "fresh draft" path: it clears pending paste payloads and /// mention link targets. Callers restoring a previously submitted draft /// that must keep `$name -> path` resolution should use - /// [`Self::set_text_content_with_mention_paths`] instead. + /// [`Self::set_text_content_with_mention_bindings`] instead. pub(crate) fn set_text_content( &mut self, text: String, text_elements: Vec, local_image_paths: Vec, ) { - self.set_text_content_with_mention_paths( + self.set_text_content_with_mention_bindings( text, text_elements, local_image_paths, - HashMap::new(), + Vec::new(), ); } /// Replace the entire composer content while restoring mention link targets. /// /// Mention popup insertion stores both visible text (for example `$file`) - /// and hidden `mention_paths` used to resolve the canonical target during + /// and hidden mention bindings used to resolve the canonical target during /// submission. Use this method when restoring an interrupted or blocked /// draft; if callers restore only text and images, mentions can appear /// intact to users while resolving to the wrong target or dropping on /// retry. - pub(crate) fn set_text_content_with_mention_paths( + pub(crate) fn set_text_content_with_mention_bindings( &mut self, text: String, text_elements: Vec, local_image_paths: Vec, - mention_paths: HashMap, + mention_bindings: Vec, ) { // Clear any existing content, placeholders, and attachments first. self.textarea.set_text_clearing_elements(""); self.pending_pastes.clear(); self.attached_images.clear(); - self.mention_paths = mention_paths; + self.mention_bindings.clear(); self.textarea.set_text_with_elements(&text, &text_elements); @@ -782,6 +809,8 @@ impl ChatComposer { } } + self.bind_mentions_from_snapshot(mention_bindings); + self.textarea.set_cursor(0); self.sync_popups(); } @@ -808,12 +837,14 @@ impl ChatComposer { .iter() .map(|img| img.path.clone()) .collect(); + let mention_bindings = self.snapshot_mention_bindings(); self.set_text_content(String::new(), Vec::new(), Vec::new()); self.history.reset_navigation(); self.history.record_local_submission(HistoryEntry { text: previous.clone(), text_elements, local_image_paths, + mention_bindings, }); Some(previous) } @@ -845,6 +876,14 @@ impl ChatComposer { .collect() } + pub(crate) fn mention_bindings(&self) -> Vec { + self.snapshot_mention_bindings() + } + + pub(crate) fn take_recent_submission_mention_bindings(&mut self) -> Vec { + std::mem::take(&mut self.recent_submission_mention_bindings) + } + fn prune_attached_images_for_submission(&mut self, text: &str, text_elements: &[TextElement]) { if self.attached_images.is_empty() { return; @@ -1476,10 +1515,7 @@ impl ChatComposer { if close_popup { if let Some((insert_text, path)) = selected_mention { - if let Some(path) = path.as_deref() { - self.record_mention_path(&insert_text, path); - } - self.insert_selected_mention(&insert_text); + self.insert_selected_mention(&insert_text, path.as_deref()); } self.active_popup = ActivePopup::None; } @@ -1786,7 +1822,7 @@ impl ChatComposer { self.textarea.set_cursor(new_cursor); } - fn insert_selected_mention(&mut self, insert_text: &str) { + fn insert_selected_mention(&mut self, insert_text: &str, path: Option<&str>) { let cursor_offset = self.textarea.cursor(); let text = self.textarea.text(); let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); @@ -1807,28 +1843,30 @@ impl ChatComposer { .unwrap_or(after_cursor.len()); let end_idx = safe_cursor + end_rel_idx; - let inserted = insert_text.to_string(); + // Remove the active token and insert the selected mention as an atomic element. + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); + let id = self.textarea.insert_element(insert_text); - let mut new_text = - String::with_capacity(text.len() - (end_idx - start_idx) + inserted.len() + 1); - new_text.push_str(&text[..start_idx]); - new_text.push_str(&inserted); - new_text.push(' '); - new_text.push_str(&text[end_idx..]); + if let (Some(path), Some(mention)) = + (path, Self::mention_name_from_insert_text(insert_text)) + { + self.mention_bindings.insert( + id, + ComposerMentionBinding { + mention, + path: path.to_string(), + }, + ); + } - // Mention insertion rebuilds plain text, so drop existing elements. - self.textarea.set_text_clearing_elements(&new_text); - let new_cursor = start_idx.saturating_add(inserted.len()).saturating_add(1); + self.textarea.insert_str(" "); + let new_cursor = start_idx + .saturating_add(insert_text.len()) + .saturating_add(1); self.textarea.set_cursor(new_cursor); } - fn record_mention_path(&mut self, insert_text: &str, path: &str) { - let Some(name) = Self::mention_name_from_insert_text(insert_text) else { - return; - }; - self.mention_paths.insert(name, path.to_string()); - } - fn mention_name_from_insert_text(insert_text: &str) -> Option { let name = insert_text.strip_prefix('$')?; if name.is_empty() { @@ -1845,6 +1883,67 @@ impl ChatComposer { } } + fn current_mention_elements(&self) -> Vec<(u64, String)> { + self.textarea + .text_element_snapshots() + .into_iter() + .filter_map(|snapshot| { + Self::mention_name_from_insert_text(snapshot.text.as_str()) + .map(|mention| (snapshot.id, mention)) + }) + .collect() + } + + fn snapshot_mention_bindings(&self) -> Vec { + let mut ordered = Vec::new(); + for (id, mention) in self.current_mention_elements() { + if let Some(binding) = self.mention_bindings.get(&id) + && binding.mention == mention + { + ordered.push(MentionBinding { + mention: binding.mention.clone(), + path: binding.path.clone(), + }); + } + } + ordered + } + + fn bind_mentions_from_snapshot(&mut self, mention_bindings: Vec) { + self.mention_bindings.clear(); + if mention_bindings.is_empty() { + return; + } + + let text = self.textarea.text().to_string(); + let mut scan_from = 0usize; + for binding in mention_bindings { + let token = format!("${}", binding.mention); + let Some(range) = + find_next_mention_token_range(text.as_str(), token.as_str(), scan_from) + else { + continue; + }; + + let id = if let Some(id) = self.textarea.add_element_range(range.clone()) { + Some(id) + } else { + self.textarea.element_id_for_exact_range(range.clone()) + }; + + if let Some(id) = id { + self.mention_bindings.insert( + id, + ComposerMentionBinding { + mention: binding.mention, + path: binding.path, + }, + ); + scan_from = range.end; + } + } + } + /// Prepare text for submission/queuing. Returns None if submission should be suppressed. /// On success, clears pending paste payloads because placeholders have been expanded. /// @@ -1856,6 +1955,7 @@ impl ChatComposer { let mut text = self.textarea.text().to_string(); let original_input = text.clone(); let original_text_elements = self.textarea.text_elements(); + let original_mention_bindings = self.snapshot_mention_bindings(); let original_local_image_paths = self .attached_images .iter() @@ -1864,6 +1964,7 @@ impl ChatComposer { let original_pending_pastes = self.pending_pastes.clone(); let mut text_elements = original_text_elements.clone(); let input_starts_with_space = original_input.starts_with(' '); + self.recent_submission_mention_bindings.clear(); self.textarea.set_text_clearing_elements(""); if !self.pending_pastes.is_empty() { @@ -1909,10 +2010,11 @@ impl ChatComposer { self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( history_cell::new_info_event(message, None), ))); - self.set_text_content( + self.set_text_content_with_mention_bindings( original_input.clone(), original_text_elements, original_local_image_paths, + original_mention_bindings, ); self.pending_pastes.clone_from(&original_pending_pastes); self.textarea.set_cursor(original_input.len()); @@ -1929,10 +2031,11 @@ impl ChatComposer { self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( history_cell::new_error_event(err.user_message()), ))); - self.set_text_content( + self.set_text_content_with_mention_bindings( original_input.clone(), original_text_elements, original_local_image_paths, + original_mention_bindings, ); self.pending_pastes.clone_from(&original_pending_pastes); self.textarea.set_cursor(original_input.len()); @@ -1950,6 +2053,7 @@ impl ChatComposer { if text.is_empty() && self.attached_images.is_empty() { return None; } + self.recent_submission_mention_bindings = original_mention_bindings.clone(); if record_history && (!text.is_empty() || !self.attached_images.is_empty()) { let local_image_paths = self .attached_images @@ -1960,6 +2064,7 @@ impl ChatComposer { text: text.clone(), text_elements: text_elements.clone(), local_image_paths, + mention_bindings: original_mention_bindings, }); } self.pending_pastes.clear(); @@ -2022,6 +2127,7 @@ impl ChatComposer { let original_input = self.textarea.text().to_string(); let original_text_elements = self.textarea.text_elements(); + let original_mention_bindings = self.snapshot_mention_bindings(); let original_local_image_paths = self .attached_images .iter() @@ -2053,10 +2159,11 @@ impl ChatComposer { } } else { // Restore text if submission was suppressed. - self.set_text_content( + self.set_text_content_with_mention_bindings( original_input, original_text_elements, original_local_image_paths, + original_mention_bindings, ); self.pending_pastes = original_pending_pastes; (InputResult::None, true) @@ -2245,10 +2352,11 @@ impl ChatComposer { _ => unreachable!(), }; if let Some(entry) = replace_entry { - self.set_text_content( + self.set_text_content_with_mention_bindings( entry.text, entry.text_elements, entry.local_image_paths, + entry.mention_bindings, ); return (InputResult::None, true); } @@ -2930,6 +3038,8 @@ impl ChatComposer { insert_text: format!("${skill_name}"), search_terms, path: Some(skill.path.to_string_lossy().into_owned()), + category_tag: (skill.scope == codex_core::protocol::SkillScope::Repo) + .then(|| "[Repo]".to_string()), }); } } @@ -2952,23 +3062,26 @@ impl ChatComposer { insert_text: format!("${slug}"), search_terms, path: Some(format!("app://{connector_id}")), + category_tag: Some("[App]".to_string()), }); } } + let mut counts: HashMap = HashMap::new(); + for mention in &mentions { + *counts.entry(mention.insert_text.clone()).or_insert(0) += 1; + } + for mention in &mut mentions { + if counts.get(&mention.insert_text).copied().unwrap_or(0) <= 1 { + mention.category_tag = None; + } + } + mentions } fn connector_brief_description(connector: &AppInfo) -> String { - let status_label = if connector.is_accessible { - "Connected" - } else { - "Can be installed" - }; - match Self::connector_description(connector) { - Some(description) => format!("{status_label} - {description}"), - None => status_label.to_string(), - } + Self::connector_description(connector).unwrap_or_default() } fn connector_description(connector: &AppInfo) -> Option { @@ -3049,6 +3162,42 @@ fn is_mention_name_char(byte: u8) -> bool { matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-') } +fn find_next_mention_token_range(text: &str, token: &str, from: usize) -> Option> { + if token.is_empty() || from >= text.len() { + return None; + } + let bytes = text.as_bytes(); + let token_bytes = token.as_bytes(); + let mut index = from; + + while index < bytes.len() { + if bytes[index] != b'$' { + index += 1; + continue; + } + + let end = index.saturating_add(token_bytes.len()); + if end > bytes.len() { + return None; + } + if &bytes[index..end] != token_bytes { + index += 1; + continue; + } + + if bytes + .get(end) + .is_none_or(|byte| !is_mention_name_char(*byte)) + { + return Some(index..end); + } + + index = end; + } + + None +} + impl Renderable for ChatComposer { fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { if !self.input_enabled { @@ -5513,6 +5662,40 @@ mod tests { assert_eq!(vec![path], imgs); } + #[test] + fn submit_captures_recent_mention_bindings_before_clearing_textarea() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + composer.set_steer_enabled(true); + + let mention_bindings = vec![MentionBinding { + mention: "figma".to_string(), + path: "/tmp/user/figma/SKILL.md".to_string(), + }]; + composer.set_text_content_with_mention_bindings( + "$figma please".to_string(), + Vec::new(), + Vec::new(), + mention_bindings.clone(), + ); + + let (result, _) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(matches!(result, InputResult::Submitted { .. })); + assert_eq!( + composer.take_recent_submission_mention_bindings(), + mention_bindings + ); + assert!(composer.take_mention_bindings().is_empty()); + } + #[test] fn history_navigation_restores_image_attachments() { let (tx, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index da9f46ae4b..6d9322cd19 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -3,6 +3,8 @@ use std::path::PathBuf; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; +use crate::bottom_pane::MentionBinding; +use crate::mention_codec::decode_history_mentions; use codex_core::protocol::Op; use codex_protocol::user_input::TextElement; @@ -11,6 +13,7 @@ pub(crate) struct HistoryEntry { pub(crate) text: String, pub(crate) text_elements: Vec, pub(crate) local_image_paths: Vec, + pub(crate) mention_bindings: Vec, } impl HistoryEntry { @@ -19,14 +22,24 @@ impl HistoryEntry { text: String::new(), text_elements: Vec::new(), local_image_paths: Vec::new(), + mention_bindings: Vec::new(), } } pub(crate) fn from_text(text: String) -> Self { + let decoded = decode_history_mentions(&text); Self { - text, + text: decoded.text, text_elements: Vec::new(), local_image_paths: Vec::new(), + mention_bindings: decoded + .mentions + .into_iter() + .map(|mention| MentionBinding { + mention: mention.mention, + path: mention.path, + }) + .collect(), } } } diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index 928fb541b3..48cf255ee0 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -220,6 +220,7 @@ impl CommandPopup { match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), display_shortcut: None, description: Some(description), + category_tag: None, wrap_indent: None, is_disabled: false, disabled_reason: None, diff --git a/codex-rs/tui/src/bottom_pane/file_search_popup.rs b/codex-rs/tui/src/bottom_pane/file_search_popup.rs index 3e6ef814a2..9eb8121b69 100644 --- a/codex-rs/tui/src/bottom_pane/file_search_popup.rs +++ b/codex-rs/tui/src/bottom_pane/file_search_popup.rs @@ -125,6 +125,7 @@ impl WidgetRef for &FileSearchPopup { .map(|v| v.iter().map(|&i| i as usize).collect()), display_shortcut: None, description: None, + category_tag: None, wrap_indent: None, is_disabled: false, disabled_reason: None, diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 9bc046f010..3cf74e3a1a 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -258,6 +258,7 @@ impl ListSelectionView { display_shortcut: item.display_shortcut, match_indices: None, description, + category_tag: None, wrap_indent, is_disabled, disabled_reason: item.disabled_reason.clone(), diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index b9c77fc72d..960e67fed6 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -13,7 +13,6 @@ //! //! Some UI is time-based rather than input-based, such as the transient "press again to quit" //! hint. The pane schedules redraws so those hints can expire even when the UI is otherwise idle. -use std::collections::HashMap; use std::path::PathBuf; use crate::app_event::ConnectorsSnapshot; @@ -55,6 +54,14 @@ pub(crate) struct LocalImageAttachment { pub(crate) placeholder: String, pub(crate) path: PathBuf, } + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct MentionBinding { + /// Mention token text without the leading `$`. + pub(crate) mention: String, + /// Canonical mention target (for example `app://...` or absolute SKILL.md path). + pub(crate) path: String, +} mod chat_composer; mod chat_composer_history; mod command_popup; @@ -228,14 +235,19 @@ impl BottomPane { self.request_redraw(); } - pub fn take_mention_paths(&mut self) -> HashMap { - self.composer.take_mention_paths() + pub fn take_mention_bindings(&mut self) -> Vec { + self.composer.take_mention_bindings() } - /// Clear pending attachments and mention paths e.g. when a slash command doesn't submit text. + pub fn take_recent_submission_mention_bindings(&mut self) -> Vec { + self.composer.take_recent_submission_mention_bindings() + } + + /// Clear pending attachments and mention bindings e.g. when a slash command doesn't submit text. pub(crate) fn drain_pending_submission_state(&mut self) { let _ = self.take_recent_submission_images_with_placeholders(); - let _ = self.take_mention_paths(); + let _ = self.take_recent_submission_mention_bindings(); + let _ = self.take_mention_bindings(); } pub fn set_steer_enabled(&mut self, enabled: bool) { @@ -419,7 +431,7 @@ impl BottomPane { /// /// This is intended for fresh input where mention linkage does not need to /// survive; it routes to `ChatComposer::set_text_content`, which resets - /// `mention_paths`. + /// mention bindings. pub(crate) fn set_composer_text( &mut self, text: String, @@ -437,18 +449,18 @@ impl BottomPane { /// Use this when rehydrating a draft after a local validation/gating /// failure (for example unsupported image submit) so previously selected /// mention targets remain stable across retry. - pub(crate) fn set_composer_text_with_mention_paths( + pub(crate) fn set_composer_text_with_mention_bindings( &mut self, text: String, text_elements: Vec, local_image_paths: Vec, - mention_paths: HashMap, + mention_bindings: Vec, ) { - self.composer.set_text_content_with_mention_paths( + self.composer.set_text_content_with_mention_bindings( text, text_elements, local_image_paths, - mention_paths, + mention_bindings, ); self.request_redraw(); } @@ -481,6 +493,10 @@ impl BottomPane { self.composer.local_images() } + pub(crate) fn composer_mention_bindings(&self) -> Vec { + self.composer.mention_bindings() + } + #[cfg(test)] pub(crate) fn composer_local_image_paths(&self) -> Vec { self.composer.local_image_paths() diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 3f827dc41f..6377bedb95 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -30,6 +30,7 @@ pub(crate) struct GenericDisplayRow { pub display_shortcut: Option, pub match_indices: Option>, // indices to bold (char positions) pub description: Option, // optional grey text after the name + pub category_tag: Option, // optional right-side category label pub disabled_reason: Option, // optional disabled message pub is_disabled: bool, pub wrap_indent: Option, // optional indent for wrapped lines @@ -337,6 +338,10 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { } full_spans.push(desc.clone().dim()); } + if let Some(tag) = row.category_tag.as_deref().filter(|tag| !tag.is_empty()) { + full_spans.push(" ".into()); + full_spans.push(tag.to_string().dim()); + } Line::from(full_spans) } diff --git a/codex-rs/tui/src/bottom_pane/skill_popup.rs b/codex-rs/tui/src/bottom_pane/skill_popup.rs index 390b44295f..6ce8d4e373 100644 --- a/codex-rs/tui/src/bottom_pane/skill_popup.rs +++ b/codex-rs/tui/src/bottom_pane/skill_popup.rs @@ -24,8 +24,11 @@ pub(crate) struct MentionItem { pub(crate) insert_text: String, pub(crate) search_terms: Vec, pub(crate) path: Option, + pub(crate) category_tag: Option, } +const MENTION_NAME_TRUNCATE_LEN: usize = 24; + pub(crate) struct SkillPopup { query: String, mentions: Vec, @@ -94,13 +97,14 @@ impl SkillPopup { .into_iter() .map(|(idx, indices, _score)| { let mention = &self.mentions[idx]; - let name = truncate_text(&mention.display_name, 21); + let name = truncate_text(&mention.display_name, MENTION_NAME_TRUNCATE_LEN); let description = mention.description.clone().unwrap_or_default(); GenericDisplayRow { name, match_indices: indices, display_shortcut: None, description: Some(description).filter(|desc| !desc.is_empty()), + category_tag: mention.category_tag.clone(), is_disabled: false, disabled_reason: None, wrap_indent: None, diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index a90d5b87c6..2caa45602f 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -25,9 +25,17 @@ fn is_word_separator(ch: char) -> bool { #[derive(Debug, Clone)] struct TextElement { + id: u64, range: Range, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct TextElementSnapshot { + pub(crate) id: u64, + pub(crate) range: Range, + pub(crate) text: String, +} + #[derive(Debug)] pub(crate) struct TextArea { text: String, @@ -35,6 +43,7 @@ pub(crate) struct TextArea { wrap_cache: RefCell>, preferred_col: Option, elements: Vec, + next_element_id: u64, kill_buffer: String, } @@ -58,6 +67,7 @@ impl TextArea { wrap_cache: RefCell::new(None), preferred_col: None, elements: Vec::new(), + next_element_id: 1, kill_buffer: String::new(), } } @@ -87,7 +97,11 @@ impl TextArea { if start >= end { continue; } - self.elements.push(TextElement { range: start..end }); + let id = self.next_element_id(); + self.elements.push(TextElement { + id, + range: start..end, + }); } self.elements.sort_by_key(|e| e.range.start); } @@ -766,6 +780,28 @@ impl TextArea { .collect() } + pub(crate) fn text_element_snapshots(&self) -> Vec { + self.elements + .iter() + .filter_map(|element| { + self.text + .get(element.range.clone()) + .map(|text| TextElementSnapshot { + id: element.id, + range: element.range.clone(), + text: text.to_string(), + }) + }) + .collect() + } + + pub(crate) fn element_id_for_exact_range(&self, range: Range) -> Option { + self.elements + .iter() + .find(|element| element.range == range) + .map(|element| element.id) + } + /// Renames a single text element in-place, keeping it atomic. /// /// Use this when the element payload is an identifier (e.g. a placeholder) that must be @@ -835,41 +871,47 @@ impl TextArea { true } - pub fn insert_element(&mut self, text: &str) { + pub fn insert_element(&mut self, text: &str) -> u64 { let start = self.clamp_pos_for_insertion(self.cursor_pos); self.insert_str_at(start, text); let end = start + text.len(); - self.add_element(start..end); + let id = self.add_element(start..end); // Place cursor at end of inserted element self.set_cursor(end); + id } /// Mark an existing text range as an atomic element without changing the text. /// /// This is used to convert already-typed tokens (like `/plan`) into elements /// so they render and edit atomically. Overlapping or duplicate ranges are ignored. - pub fn add_element_range(&mut self, range: Range) { + pub fn add_element_range(&mut self, range: Range) -> Option { let start = self.clamp_pos_to_char_boundary(range.start.min(self.text.len())); let end = self.clamp_pos_to_char_boundary(range.end.min(self.text.len())); if start >= end { - return; + return None; } if self .elements .iter() .any(|e| e.range.start == start && e.range.end == end) { - return; + return None; } if self .elements .iter() .any(|e| start < e.range.end && end > e.range.start) { - return; + return None; } - self.elements.push(TextElement { range: start..end }); + let id = self.next_element_id(); + self.elements.push(TextElement { + id, + range: start..end, + }); self.elements.sort_by_key(|e| e.range.start); + Some(id) } pub fn remove_element_range(&mut self, range: Range) -> bool { @@ -884,10 +926,18 @@ impl TextArea { len_before != self.elements.len() } - fn add_element(&mut self, range: Range) { - let elem = TextElement { range }; + fn add_element(&mut self, range: Range) -> u64 { + let id = self.next_element_id(); + let elem = TextElement { id, range }; self.elements.push(elem); self.elements.sort_by_key(|e| e.range.start); + id + } + + fn next_element_id(&mut self) -> u64 { + let id = self.next_element_id; + self.next_element_id = self.next_element_id.saturating_add(1); + id } fn find_element_containing(&self, pos: usize) -> Option { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 43755c1f41..94fece3620 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -20,6 +20,11 @@ //! is in progress and while MCP server startup is in progress. Those lifecycles are tracked //! independently (`agent_turn_running` and `mcp_startup_status`) and synchronized via //! `update_task_running_state`. +//! +//! For preamble-capable models, assistant output may include commentary before +//! the final answer. During streaming we hide the status row to avoid duplicate +//! progress indicators; once commentary completes and stream queues drain, we +//! re-show it so users still see turn-in-progress state between output bursts. use std::collections::HashMap; use std::collections::HashSet; use std::collections::VecDeque; @@ -116,6 +121,8 @@ use codex_protocol::config_types::Personality; use codex_protocol::config_types::Settings; #[cfg(target_os = "windows")] use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::items::AgentMessageItem; +use codex_protocol::models::MessagePhase; use codex_protocol::models::local_image_label_text; use codex_protocol::parse_command::ParsedCommand; use codex_protocol::request_user_input::RequestUserInputEvent; @@ -164,6 +171,7 @@ use crate::bottom_pane::ExperimentalFeaturesView; use crate::bottom_pane::FeedbackAudience; use crate::bottom_pane::InputResult; use crate::bottom_pane::LocalImageAttachment; +use crate::bottom_pane::MentionBinding; use crate::bottom_pane::QUIT_SHORTCUT_TIMEOUT; use crate::bottom_pane::SelectionAction; use crate::bottom_pane::SelectionItem; @@ -210,6 +218,8 @@ mod skills; use self::skills::collect_tool_mentions; use self::skills::find_app_mentions; use self::skills::find_skill_mentions_with_tool_mentions; +use crate::mention_codec::LinkedMention; +use crate::mention_codec::encode_history_mentions; use crate::streaming::chunking::AdaptiveChunkingPolicy; use crate::streaming::commit_tick::CommitTickScope; use crate::streaming::commit_tick::run_commit_tick; @@ -539,6 +549,8 @@ pub(crate) struct ChatWidget { current_status_header: String, // Previous status header to restore after a transient stream retry. retry_status_header: Option, + // Set when commentary output completes; once stream queues go idle we restore the status row. + pending_status_indicator_restore: bool, thread_id: Option, thread_name: Option, forked_from: Option, @@ -639,7 +651,7 @@ pub(crate) struct UserMessage { text: String, local_images: Vec, text_elements: Vec, - mention_paths: HashMap, + mention_bindings: Vec, } impl From for UserMessage { @@ -649,7 +661,7 @@ impl From for UserMessage { local_images: Vec::new(), // Plain text conversion has no UI element ranges. text_elements: Vec::new(), - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), } } } @@ -661,7 +673,7 @@ impl From<&str> for UserMessage { local_images: Vec::new(), // Plain text conversion has no UI element ranges. text_elements: Vec::new(), - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), } } } @@ -687,7 +699,7 @@ pub(crate) fn create_initial_user_message( text, local_images, text_elements, - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }) } } @@ -701,14 +713,14 @@ fn remap_placeholders_for_message(message: UserMessage, next_label: &mut usize) text, text_elements, local_images, - mention_paths, + mention_bindings, } = message; if local_images.is_empty() { return UserMessage { text, text_elements, local_images, - mention_paths, + mention_bindings, }; } @@ -763,7 +775,7 @@ fn remap_placeholders_for_message(message: UserMessage, next_label: &mut usize) text: rebuilt, local_images: remapped_images, text_elements: rebuilt_elements, - mention_paths, + mention_bindings, } } @@ -805,6 +817,37 @@ impl ChatWidget { self.adaptive_chunking.reset(); } + fn stream_controllers_idle(&self) -> bool { + self.stream_controller + .as_ref() + .map(|controller| controller.queued_lines() == 0) + .unwrap_or(true) + && self + .plan_stream_controller + .as_ref() + .map(|controller| controller.queued_lines() == 0) + .unwrap_or(true) + } + + /// Restore the status indicator only after commentary completion is pending, + /// the turn is still running, and all stream queues have drained. + /// + /// This gate prevents flicker while normal output is still actively + /// streaming, but still restores a visible "working" affordance when a + /// commentary block ends before the turn itself has completed. + fn maybe_restore_status_indicator_after_stream_idle(&mut self) { + if !self.pending_status_indicator_restore + || !self.bottom_pane.is_task_running() + || !self.stream_controllers_idle() + { + return; + } + + self.bottom_pane.ensure_status_indicator(); + self.set_status_header(self.current_status_header.clone()); + self.pending_status_indicator_restore = false; + } + /// Update the status indicator header and details. /// /// Passing `None` clears any existing details. @@ -1178,21 +1221,29 @@ impl ChatWidget { } else { text }; + // Plan commit ticks can hide the status row; remember whether we streamed plan output so + // completion can restore it once stream queues are idle. + let should_restore_after_stream = self.plan_stream_controller.is_some(); self.plan_delta_buffer.clear(); self.plan_item_active = false; self.saw_plan_item_this_turn = true; - if let Some(mut controller) = self.plan_stream_controller.take() - && let Some(cell) = controller.finalize() - { + let finalized_streamed_cell = + if let Some(mut controller) = self.plan_stream_controller.take() { + controller.finalize() + } else { + None + }; + if let Some(cell) = finalized_streamed_cell { self.add_boxed_history(cell); // TODO: Replace streamed output with the final plan item text if plan streaming is // removed or if we need to reconcile mismatches between streamed and final content. - return; + } else if !plan_text.is_empty() { + self.add_to_history(history_cell::new_proposed_plan(plan_text)); } - if plan_text.is_empty() { - return; + if should_restore_after_stream { + self.pending_status_indicator_restore = true; + self.maybe_restore_status_indicator_after_stream_idle(); } - self.add_to_history(history_cell::new_proposed_plan(plan_text)); } fn on_agent_reasoning_delta(&mut self, delta: String) { @@ -1253,6 +1304,7 @@ impl ChatWidget { self.quit_shortcut_key = None; self.update_task_running_state(); self.retry_status_header = None; + self.pending_status_indicator_restore = false; self.bottom_pane.set_interrupt_hint_visible(true); self.set_status_header(String::from("Working")); self.full_reasoning_buffer.clear(); @@ -1294,6 +1346,7 @@ impl ChatWidget { self.request_status_line_branch_refresh(); } // Mark task stopped and request redraw now that all content is in history. + self.pending_status_indicator_restore = false; self.agent_turn_running = false; self.update_task_running_state(); self.running_commands.clear(); @@ -1525,6 +1578,7 @@ impl ChatWidget { self.adaptive_chunking.reset(); self.stream_controller = None; self.plan_stream_controller = None; + self.pending_status_indicator_restore = false; self.request_status_line_branch_refresh(); self.maybe_show_pending_rate_limit_prompt(); } @@ -1666,7 +1720,7 @@ impl ChatWidget { text: self.bottom_pane.composer_text(), text_elements: self.bottom_pane.composer_text_elements(), local_images: self.bottom_pane.composer_local_images(), - mention_paths: HashMap::new(), + mention_bindings: self.bottom_pane.composer_mention_bindings(), }; let mut to_merge: Vec = self.queued_user_messages.drain(..).collect(); @@ -1678,7 +1732,7 @@ impl ChatWidget { text: String::new(), text_elements: Vec::new(), local_images: Vec::new(), - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }; let mut combined_offset = 0usize; let mut next_image_label = 1usize; @@ -1700,7 +1754,7 @@ impl ChatWidget { elem })); combined.local_images.extend(message.local_images); - combined.mention_paths.extend(message.mention_paths); + combined.mention_bindings.extend(message.mention_bindings); } Some(combined) @@ -1711,11 +1765,15 @@ impl ChatWidget { text, local_images, text_elements, - mention_paths: _, + mention_bindings, } = user_message; let local_image_paths = local_images.into_iter().map(|img| img.path).collect(); - self.bottom_pane - .set_composer_text(text, text_elements, local_image_paths); + self.bottom_pane.set_composer_text_with_mention_bindings( + text, + text_elements, + local_image_paths, + mention_bindings, + ); } fn on_plan_update(&mut self, update: UpdatePlanArgs) { @@ -2080,9 +2138,24 @@ impl ChatWidget { if self.retry_status_header.is_none() { self.retry_status_header = Some(self.current_status_header.clone()); } + self.bottom_pane.ensure_status_indicator(); self.set_status(message, additional_details); } + /// Handle completion of an `AgentMessage` turn item. + /// + /// Commentary completion sets a deferred restore flag so the status row + /// returns once stream queues are idle. Final-answer completion (or absent + /// phase for legacy models) clears the flag to preserve historical behavior. + fn on_agent_message_item_completed(&mut self, item: AgentMessageItem) { + self.pending_status_indicator_restore = match item.phase { + // Models that don't support preambles only output AgentMessageItems on turn completion. + Some(MessagePhase::FinalAnswer) | None => false, + Some(MessagePhase::Commentary) => true, + }; + self.maybe_restore_status_indicator_after_stream_idle(); + } + /// Periodic tick for stream commits. In smooth mode this preserves one-line pacing, while /// catch-up mode drains larger batches to reduce queue lag. pub(crate) fn on_commit_tick(&mut self) { @@ -2103,9 +2176,8 @@ impl ChatWidget { /// /// `scope` controls whether this call may commit in smooth mode or only when catch-up /// is currently active. While lines are actively streaming we hide the status row to avoid - /// duplicate "in progress" affordances, but once all stream controllers go idle for this - /// turn we restore the status row if the task is still running so users keep a live - /// spinner/shimmer signal between preamble output and subsequent tool activity. + /// duplicate "in progress" affordances. Restoration is gated separately so we only re-show + /// the row after commentary completion once stream queues are idle. fn run_commit_tick_with_scope(&mut self, scope: CommitTickScope) { let now = Instant::now(); let outcome = run_commit_tick( @@ -2121,10 +2193,7 @@ impl ChatWidget { } if outcome.has_controller && outcome.all_idle { - if self.bottom_pane.is_task_running() { - self.bottom_pane.ensure_status_indicator(); - self.set_status_header(self.current_status_header.clone()); - } + self.maybe_restore_status_indicator_after_stream_idle(); self.app_event_tx.send(AppEvent::StopCommitAnimation); } @@ -2555,6 +2624,7 @@ impl ChatWidget { full_reasoning_buffer: String::new(), current_status_header: String::from("Working"), retry_status_header: None, + pending_status_indicator_restore: false, thread_id: None, thread_name: None, forked_from: None, @@ -2717,6 +2787,7 @@ impl ChatWidget { full_reasoning_buffer: String::new(), current_status_header: String::from("Working"), retry_status_header: None, + pending_status_indicator_restore: false, thread_id: None, thread_name: None, forked_from: None, @@ -2868,6 +2939,7 @@ impl ChatWidget { full_reasoning_buffer: String::new(), current_status_header: String::from("Working"), retry_status_header: None, + pending_status_indicator_restore: false, thread_id: None, thread_name: None, forked_from: None, @@ -3022,7 +3094,9 @@ impl ChatWidget { .bottom_pane .take_recent_submission_images_with_placeholders(), text_elements, - mention_paths: self.bottom_pane.take_mention_paths(), + mention_bindings: self + .bottom_pane + .take_recent_submission_mention_bindings(), }; if self.is_session_configured() && !self.is_plan_streaming_in_tui() { // Submitted is only emitted when steer is enabled. @@ -3045,7 +3119,9 @@ impl ChatWidget { .bottom_pane .take_recent_submission_images_with_placeholders(), text_elements, - mention_paths: self.bottom_pane.take_mention_paths(), + mention_bindings: self + .bottom_pane + .take_recent_submission_mention_bindings(), }; self.queue_user_message(user_message); } @@ -3414,7 +3490,7 @@ impl ChatWidget { .bottom_pane .take_recent_submission_images_with_placeholders(), text_elements: prepared_elements, - mention_paths: self.bottom_pane.take_mention_paths(), + mention_bindings: self.bottom_pane.take_recent_submission_mention_bindings(), }; if self.is_session_configured() { self.reasoning_buffer.clear(); @@ -3551,13 +3627,18 @@ impl ChatWidget { text, local_images, text_elements, - mention_paths, + mention_bindings, } = user_message; if text.is_empty() && local_images.is_empty() { return; } if !local_images.is_empty() && !self.current_model_supports_images() { - self.restore_blocked_image_submission(text, text_elements, local_images, mention_paths); + self.restore_blocked_image_submission( + text, + text_elements, + local_images, + mention_bindings, + ); return; } @@ -3594,16 +3675,43 @@ impl ChatWidget { }); } - let mentions = collect_tool_mentions(&text, &mention_paths); + let mentions = collect_tool_mentions(&text, &HashMap::new()); + let bound_names: HashSet = mention_bindings + .iter() + .map(|binding| binding.mention.clone()) + .collect(); let mut skill_names_lower: HashSet = HashSet::new(); + let mut selected_skill_paths: HashSet = HashSet::new(); if let Some(skills) = self.bottom_pane.skills() { skill_names_lower = skills .iter() .map(|skill| skill.name.to_ascii_lowercase()) .collect(); + + for binding in &mention_bindings { + let path = binding + .path + .strip_prefix("skill://") + .unwrap_or(binding.path.as_str()); + let path = Path::new(path); + if let Some(skill) = skills.iter().find(|skill| skill.path.as_path() == path) + && selected_skill_paths.insert(skill.path.clone()) + { + items.push(UserInput::Skill { + name: skill.name.clone(), + path: skill.path.clone(), + }); + } + } + let skill_mentions = find_skill_mentions_with_tool_mentions(&mentions, skills); for skill in skill_mentions { + if bound_names.contains(skill.name.as_str()) + || !selected_skill_paths.insert(skill.path.clone()) + { + continue; + } items.push(UserInput::Skill { name: skill.name.clone(), path: skill.path.clone(), @@ -3611,9 +3719,33 @@ impl ChatWidget { } } + let mut selected_app_ids: HashSet = HashSet::new(); if let Some(apps) = self.connectors_for_mentions() { + for binding in &mention_bindings { + let Some(app_id) = binding + .path + .strip_prefix("app://") + .filter(|id| !id.is_empty()) + else { + continue; + }; + if !selected_app_ids.insert(app_id.to_string()) { + continue; + } + if let Some(app) = apps.iter().find(|app| app.id == app_id) { + items.push(UserInput::Mention { + name: app.name.clone(), + path: binding.path.clone(), + }); + } + } + let app_mentions = find_app_mentions(&mentions, apps, &skill_names_lower); for app in app_mentions { + let slug = codex_core::connectors::connector_mention_slug(&app); + if bound_names.contains(&slug) || !selected_app_ids.insert(app.id.clone()) { + continue; + } let app_id = app.id.as_str(); items.push(UserInput::Mention { name: app.name.clone(), @@ -3654,8 +3786,16 @@ impl ChatWidget { // Persist the text to cross-session message history. if !text.is_empty() { + let encoded_mentions = mention_bindings + .iter() + .map(|binding| LinkedMention { + mention: binding.mention.clone(), + path: binding.path.clone(), + }) + .collect::>(); + let history_text = encode_history_mentions(&text, &encoded_mentions); self.codex_op_tx - .send(Op::AddToHistory { text: text.clone() }) + .send(Op::AddToHistory { text: history_text }) .unwrap_or_else(|e| { tracing::error!("failed to send AddHistory op: {e}"); }); @@ -3678,7 +3818,7 @@ impl ChatWidget { /// /// The blocked-image path intentionally keeps the draft in the composer so /// users can remove attachments and retry. We must restore - /// `mention_paths` alongside visible text; restoring only `$name` tokens + /// mention bindings alongside visible text; restoring only `$name` tokens /// makes the draft look correct while degrading mention resolution to /// name-only heuristics on retry. fn restore_blocked_image_submission( @@ -3686,15 +3826,15 @@ impl ChatWidget { text: String, text_elements: Vec, local_images: Vec, - mention_paths: HashMap, + mention_bindings: Vec, ) { // Preserve the user's composed payload so they can retry after changing models. let local_image_paths = local_images.iter().map(|img| img.path.clone()).collect(); - self.bottom_pane.set_composer_text_with_mention_paths( + self.bottom_pane.set_composer_text_with_mention_bindings( text, text_elements, local_image_paths, - mention_paths, + mention_bindings, ); self.add_to_history(history_cell::new_warning_event( self.image_inputs_not_supported_message(), @@ -3892,8 +4032,12 @@ impl ChatWidget { | EventMsg::ReasoningRawContentDelta(_) | EventMsg::DynamicToolCallRequest(_) => {} EventMsg::ItemCompleted(event) => { - if let codex_protocol::items::TurnItem::Plan(plan_item) = event.item { - self.on_plan_item_completed(plan_item.text); + let item = event.item; + if let codex_protocol::items::TurnItem::Plan(plan_item) = &item { + self.on_plan_item_completed(plan_item.text.clone()); + } + if let codex_protocol::items::TurnItem::AgentMessage(item) = item { + self.on_agent_message_item_completed(item); } } } @@ -6436,7 +6580,7 @@ impl ChatWidget { text, local_images: Vec::new(), text_elements: Vec::new(), - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }; if should_queue { self.queue_user_message(user_message); diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 6d7df44119..fcd2b84616 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -10,6 +10,7 @@ use crate::app_event::ExitMode; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::FeedbackAudience; use crate::bottom_pane::LocalImageAttachment; +use crate::bottom_pane::MentionBinding; use crate::history_cell::UserHistoryCell; use crate::test_backend::VT100Backend; use crate::tui::FrameRequester; @@ -40,6 +41,7 @@ use codex_core::protocol::ExecCommandSource; use codex_core::protocol::ExecPolicyAmendment; use codex_core::protocol::ExitedReviewModeEvent; use codex_core::protocol::FileChange; +use codex_core::protocol::ItemCompletedEvent; use codex_core::protocol::McpStartupCompleteEvent; use codex_core::protocol::McpStartupStatus; use codex_core::protocol::McpStartupUpdateEvent; @@ -61,6 +63,7 @@ use codex_core::protocol::UndoCompletedEvent; use codex_core::protocol::UndoStartedEvent; use codex_core::protocol::ViewImageToolCallEvent; use codex_core::protocol::WarningEvent; +use codex_core::skills::model::SkillMetadata; use codex_otel::OtelManager; use codex_otel::RuntimeMetricsSummary; use codex_protocol::ThreadId; @@ -69,6 +72,10 @@ use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Personality; use codex_protocol::config_types::Settings; +use codex_protocol::items::AgentMessageContent; +use codex_protocol::items::AgentMessageItem; +use codex_protocol::items::TurnItem; +use codex_protocol::models::MessagePhase; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::default_input_modalities; @@ -77,6 +84,7 @@ use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::CodexErrorInfo; +use codex_protocol::protocol::SkillScope; use codex_protocol::user_input::TextElement; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; @@ -391,7 +399,82 @@ async fn submission_preserves_text_elements_and_local_images() { } #[tokio::test] -async fn blocked_image_restore_preserves_mention_paths() { +async fn submission_prefers_selected_duplicate_skill_path() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let repo_skill_path = PathBuf::from("/tmp/repo/figma/SKILL.md"); + let user_skill_path = PathBuf::from("/tmp/user/figma/SKILL.md"); + chat.set_skills(Some(vec![ + SkillMetadata { + name: "figma".to_string(), + description: "Repo skill".to_string(), + short_description: None, + interface: None, + dependencies: None, + path: repo_skill_path, + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "figma".to_string(), + description: "User skill".to_string(), + short_description: None, + interface: None, + dependencies: None, + path: user_skill_path.clone(), + scope: SkillScope::User, + }, + ])); + + chat.bottom_pane.set_composer_text_with_mention_bindings( + "please use $figma now".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + mention: "figma".to_string(), + path: user_skill_path.to_string_lossy().into_owned(), + }], + ); + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + let selected_skill_paths = items + .iter() + .filter_map(|item| match item { + UserInput::Skill { path, .. } => Some(path.clone()), + _ => None, + }) + .collect::>(); + assert_eq!(selected_skill_paths, vec![user_skill_path]); +} + +#[tokio::test] +async fn blocked_image_restore_preserves_mention_bindings() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; let placeholder = "[Image #1]"; @@ -404,23 +487,33 @@ async fn blocked_image_restore_preserves_mention_paths() { placeholder: placeholder.to_string(), path: PathBuf::from("/tmp/blocked.png"), }]; - let mention_paths = - HashMap::from([("file".to_string(), "/tmp/skills/file/SKILL.md".to_string())]); + let mention_bindings = vec![MentionBinding { + mention: "file".to_string(), + path: "/tmp/skills/file/SKILL.md".to_string(), + }]; chat.restore_blocked_image_submission( text.clone(), - text_elements.clone(), + text_elements, local_images.clone(), - mention_paths.clone(), + mention_bindings.clone(), ); + let mention_start = text.find("$file").expect("mention token exists"); + let expected_elements = vec![ + TextElement::new((0..placeholder.len()).into(), Some(placeholder.to_string())), + TextElement::new( + (mention_start..mention_start + "$file".len()).into(), + Some("$file".to_string()), + ), + ]; assert_eq!(chat.bottom_pane.composer_text(), text); - assert_eq!(chat.bottom_pane.composer_text_elements(), text_elements); + assert_eq!(chat.bottom_pane.composer_text_elements(), expected_elements); assert_eq!( chat.bottom_pane.composer_local_image_paths(), vec![local_images[0].path.clone()], ); - assert_eq!(chat.bottom_pane.take_mention_paths(), mention_paths); + assert_eq!(chat.bottom_pane.take_mention_bindings(), mention_bindings); let cells = drain_insert_history(&mut rx); let warning = cells @@ -468,7 +561,7 @@ async fn interrupted_turn_restores_queued_messages_with_images_and_elements() { path: first_images[0].clone(), }], text_elements: first_elements, - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }); chat.queued_user_messages.push_back(UserMessage { text: second_text, @@ -477,7 +570,7 @@ async fn interrupted_turn_restores_queued_messages_with_images_and_elements() { path: second_images[0].clone(), }], text_elements: second_elements, - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }); chat.refresh_queued_user_messages(); @@ -545,7 +638,7 @@ async fn interrupted_turn_restore_keeps_active_mode_for_resubmission() { text: "Implement the plan.".to_string(), local_images: Vec::new(), text_elements: Vec::new(), - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }); chat.refresh_queued_user_messages(); @@ -605,7 +698,7 @@ async fn remap_placeholders_uses_attachment_labels() { text, text_elements: elements, local_images: attachments, - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }; let mut next_label = 3usize; let remapped = remap_placeholders_for_message(message, &mut next_label); @@ -666,7 +759,7 @@ async fn remap_placeholders_uses_byte_ranges_when_placeholder_missing() { text, text_elements: elements, local_images: attachments, - mention_paths: HashMap::new(), + mention_bindings: Vec::new(), }; let mut next_label = 3usize; let remapped = remap_placeholders_for_message(message, &mut next_label); @@ -896,6 +989,7 @@ fn test_otel_manager(config: &Config, model: &str) -> OtelManager { None, None, None, + "test_originator".to_string(), false, "test".to_string(), SessionSource::Cli, @@ -985,6 +1079,7 @@ async fn make_chatwidget_manual( full_reasoning_buffer: String::new(), current_status_header: String::from("Working"), retry_status_header: None, + pending_status_indicator_restore: false, thread_id: None, thread_name: None, forked_from: None, @@ -1869,6 +1964,28 @@ fn terminal_interaction(chat: &mut ChatWidget, call_id: &str, process_id: &str, }); } +fn complete_assistant_message( + chat: &mut ChatWidget, + item_id: &str, + text: &str, + phase: Option, +) { + chat.handle_codex_event(Event { + id: format!("raw-{item_id}"), + msg: EventMsg::ItemCompleted(ItemCompletedEvent { + thread_id: ThreadId::new(), + turn_id: "turn-1".to_string(), + item: TurnItem::AgentMessage(AgentMessageItem { + id: item_id.to_string(), + content: vec![AgentMessageContent::Text { + text: text.to_string(), + }], + phase, + }), + }), + }); +} + fn begin_exec(chat: &mut ChatWidget, call_id: &str, raw_cmd: &str) -> ExecCommandBeginEvent { begin_exec_with_source(chat, call_id, raw_cmd, ExecCommandSource::Agent) } @@ -2014,15 +2131,16 @@ async fn enqueueing_history_prompt_multiple_times_is_stable() { #[tokio::test] async fn streaming_final_answer_keeps_task_running_state() { - let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; chat.thread_id = Some(ThreadId::new()); chat.on_task_started(); chat.on_agent_message_delta("Final answer line\n".to_string()); chat.on_commit_tick(); + drain_insert_history(&mut rx); assert!(chat.bottom_pane.is_task_running()); - assert!(chat.bottom_pane.status_widget().is_some()); + assert!(!chat.bottom_pane.status_indicator_visible()); chat.bottom_pane .set_composer_text("queued submission".to_string(), Vec::new(), Vec::new()); @@ -2044,7 +2162,26 @@ async fn streaming_final_answer_keeps_task_running_state() { } #[tokio::test] -async fn preamble_keeps_status_indicator_visible_until_exec_begin() { +async fn idle_commit_ticks_do_not_restore_status_without_commentary_completion() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.on_task_started(); + assert_eq!(chat.bottom_pane.status_indicator_visible(), true); + + chat.on_agent_message_delta("Final answer line\n".to_string()); + chat.on_commit_tick(); + drain_insert_history(&mut rx); + + assert_eq!(chat.bottom_pane.status_indicator_visible(), false); + assert_eq!(chat.bottom_pane.is_task_running(), true); + + // A second idle tick should not toggle the row back on and cause jitter. + chat.on_commit_tick(); + assert_eq!(chat.bottom_pane.status_indicator_visible(), false); +} + +#[tokio::test] +async fn commentary_completion_restores_status_indicator_before_exec_begin() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; chat.on_task_started(); @@ -2054,12 +2191,45 @@ async fn preamble_keeps_status_indicator_visible_until_exec_begin() { chat.on_commit_tick(); drain_insert_history(&mut rx); + assert_eq!(chat.bottom_pane.status_indicator_visible(), false); + + complete_assistant_message( + &mut chat, + "msg-commentary", + "Preamble line\n", + Some(MessagePhase::Commentary), + ); + assert_eq!(chat.bottom_pane.status_indicator_visible(), true); assert_eq!(chat.bottom_pane.is_task_running(), true); begin_exec(&mut chat, "call-1", "echo hi"); + assert_eq!(chat.bottom_pane.status_indicator_visible(), true); +} + +#[tokio::test] +async fn plan_completion_restores_status_indicator_after_streaming_plan_output() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + chat.set_feature_enabled(Feature::CollaborationModes, true); + let plan_mask = + collaboration_modes::mask_for_kind(chat.models_manager.as_ref(), ModeKind::Plan) + .expect("expected plan collaboration mask"); + chat.set_collaboration_mask(plan_mask); + + chat.on_task_started(); + assert_eq!(chat.bottom_pane.status_indicator_visible(), true); + + chat.on_plan_delta("- Step 1\n".to_string()); + chat.on_commit_tick(); + drain_insert_history(&mut rx); + + assert_eq!(chat.bottom_pane.status_indicator_visible(), false); + assert_eq!(chat.bottom_pane.is_task_running(), true); + + chat.on_plan_item_completed("- Step 1\n".to_string()); assert_eq!(chat.bottom_pane.status_indicator_visible(), true); + assert_eq!(chat.bottom_pane.is_task_running(), true); } #[tokio::test] @@ -2068,11 +2238,17 @@ async fn preamble_keeps_working_status_snapshot() { chat.thread_id = Some(ThreadId::new()); // Regression sequence: a preamble line is committed to history before any exec/tool event. - // The status row must remain visible so the spinner/shimmer still communicates "working". + // After commentary completes, the status row should be restored before subsequent work. chat.on_task_started(); chat.on_agent_message_delta("Preamble line\n".to_string()); chat.on_commit_tick(); drain_insert_history(&mut rx); + complete_assistant_message( + &mut chat, + "msg-commentary-snapshot", + "Preamble line\n", + Some(MessagePhase::Commentary), + ); let height = chat.desired_height(80); let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height)) @@ -5087,6 +5263,34 @@ async fn stream_error_updates_status_indicator() { assert_eq!(status.details(), Some(details)); } +#[tokio::test] +async fn stream_error_restores_hidden_status_indicator() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + chat.on_task_started(); + chat.on_agent_message_delta("Preamble line\n".to_string()); + chat.on_commit_tick(); + drain_insert_history(&mut rx); + assert!(!chat.bottom_pane.status_indicator_visible()); + + let msg = "Reconnecting... 2/5"; + let details = "Idle timeout waiting for SSE"; + chat.handle_codex_event(Event { + id: "sub-1".into(), + msg: EventMsg::StreamError(StreamErrorEvent { + message: msg.to_string(), + codex_error_info: Some(CodexErrorInfo::Other), + additional_details: Some(details.to_string()), + }), + }); + + let status = chat + .bottom_pane + .status_widget() + .expect("status indicator should be visible"); + assert_eq!(status.header(), msg); + assert_eq!(status.details(), Some(details)); +} + #[tokio::test] async fn warning_event_adds_warning_history_cell() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index daabb60273..9cb7bee210 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -82,6 +82,7 @@ pub mod live_wrap; mod markdown; mod markdown_render; mod markdown_stream; +mod mention_codec; mod model_migration; mod notifications; pub mod onboarding; diff --git a/codex-rs/tui/src/mention_codec.rs b/codex-rs/tui/src/mention_codec.rs new file mode 100644 index 0000000000..5a7670e140 --- /dev/null +++ b/codex-rs/tui/src/mention_codec.rs @@ -0,0 +1,240 @@ +use std::collections::HashMap; +use std::collections::VecDeque; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct LinkedMention { + pub(crate) mention: String, + pub(crate) path: String, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct DecodedHistoryText { + pub(crate) text: String, + pub(crate) mentions: Vec, +} + +pub(crate) fn encode_history_mentions(text: &str, mentions: &[LinkedMention]) -> String { + if mentions.is_empty() || text.is_empty() { + return text.to_string(); + } + + let mut mentions_by_name: HashMap<&str, VecDeque<&str>> = HashMap::new(); + for mention in mentions { + mentions_by_name + .entry(mention.mention.as_str()) + .or_default() + .push_back(mention.path.as_str()); + } + + let bytes = text.as_bytes(); + let mut out = String::with_capacity(text.len()); + let mut index = 0usize; + + while index < bytes.len() { + if bytes[index] == b'$' { + let name_start = index + 1; + if let Some(first) = bytes.get(name_start) + && is_mention_name_char(*first) + { + let mut name_end = name_start + 1; + while let Some(next) = bytes.get(name_end) + && is_mention_name_char(*next) + { + name_end += 1; + } + + let name = &text[name_start..name_end]; + if let Some(path) = mentions_by_name.get_mut(name).and_then(VecDeque::pop_front) { + out.push('['); + out.push('$'); + out.push_str(name); + out.push_str("]("); + out.push_str(path); + out.push(')'); + index = name_end; + continue; + } + } + } + + let Some(ch) = text[index..].chars().next() else { + break; + }; + out.push(ch); + index += ch.len_utf8(); + } + + out +} + +pub(crate) fn decode_history_mentions(text: &str) -> DecodedHistoryText { + let bytes = text.as_bytes(); + let mut out = String::with_capacity(text.len()); + let mut mentions = Vec::new(); + let mut index = 0usize; + + while index < bytes.len() { + if bytes[index] == b'[' + && let Some((name, path, end_index)) = parse_linked_tool_mention(text, bytes, index) + && !is_common_env_var(name) + && is_tool_path(path) + { + out.push('$'); + out.push_str(name); + mentions.push(LinkedMention { + mention: name.to_string(), + path: path.to_string(), + }); + index = end_index; + continue; + } + + let Some(ch) = text[index..].chars().next() else { + break; + }; + out.push(ch); + index += ch.len_utf8(); + } + + DecodedHistoryText { + text: out, + mentions, + } +} + +fn parse_linked_tool_mention<'a>( + text: &'a str, + text_bytes: &[u8], + start: usize, +) -> Option<(&'a str, &'a str, usize)> { + let dollar_index = start + 1; + if text_bytes.get(dollar_index) != Some(&b'$') { + return None; + } + + let name_start = dollar_index + 1; + let first_name_byte = text_bytes.get(name_start)?; + if !is_mention_name_char(*first_name_byte) { + return None; + } + + let mut name_end = name_start + 1; + while let Some(next_byte) = text_bytes.get(name_end) + && is_mention_name_char(*next_byte) + { + name_end += 1; + } + + if text_bytes.get(name_end) != Some(&b']') { + return None; + } + + let mut path_start = name_end + 1; + while let Some(next_byte) = text_bytes.get(path_start) + && next_byte.is_ascii_whitespace() + { + path_start += 1; + } + if text_bytes.get(path_start) != Some(&b'(') { + return None; + } + + let mut path_end = path_start + 1; + while let Some(next_byte) = text_bytes.get(path_end) + && *next_byte != b')' + { + path_end += 1; + } + if text_bytes.get(path_end) != Some(&b')') { + return None; + } + + let path = text[path_start + 1..path_end].trim(); + if path.is_empty() { + return None; + } + + let name = &text[name_start..name_end]; + Some((name, path, path_end + 1)) +} + +fn is_mention_name_char(byte: u8) -> bool { + matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-') +} + +fn is_common_env_var(name: &str) -> bool { + let upper = name.to_ascii_uppercase(); + matches!( + upper.as_str(), + "PATH" + | "HOME" + | "USER" + | "SHELL" + | "PWD" + | "TMPDIR" + | "TEMP" + | "TMP" + | "LANG" + | "TERM" + | "XDG_CONFIG_HOME" + ) +} + +fn is_tool_path(path: &str) -> bool { + path.starts_with("app://") + || path.starts_with("mcp://") + || path.starts_with("skill://") + || path + .rsplit(['/', '\\']) + .next() + .is_some_and(|name| name.eq_ignore_ascii_case("SKILL.md")) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn decode_history_mentions_restores_visible_tokens() { + let decoded = decode_history_mentions( + "Use [$figma](app://figma-1) and [$figma](/tmp/figma/SKILL.md).", + ); + assert_eq!(decoded.text, "Use $figma and $figma."); + assert_eq!( + decoded.mentions, + vec![ + LinkedMention { + mention: "figma".to_string(), + path: "app://figma-1".to_string(), + }, + LinkedMention { + mention: "figma".to_string(), + path: "/tmp/figma/SKILL.md".to_string(), + }, + ] + ); + } + + #[test] + fn encode_history_mentions_links_bound_mentions_in_order() { + let text = "$figma then $figma then $other"; + let encoded = encode_history_mentions( + text, + &[ + LinkedMention { + mention: "figma".to_string(), + path: "app://figma-app".to_string(), + }, + LinkedMention { + mention: "figma".to_string(), + path: "/tmp/figma/SKILL.md".to_string(), + }, + ], + ); + assert_eq!( + encoded, + "[$figma](app://figma-app) then [$figma](/tmp/figma/SKILL.md) then $other" + ); + } +} diff --git a/codex-rs/utils/pty/src/process_group.rs b/codex-rs/utils/pty/src/process_group.rs index dadff29f9e..f8056acc66 100644 --- a/codex-rs/utils/pty/src/process_group.rs +++ b/codex-rs/utils/pty/src/process_group.rs @@ -117,6 +117,33 @@ pub fn kill_process_group_by_pid(_pid: u32) -> io::Result<()> { Ok(()) } +#[cfg(unix)] +/// Send SIGTERM to a specific process group ID (best-effort). +/// +/// Returns `Ok(true)` when SIGTERM was delivered to an existing group and +/// `Ok(false)` when the group no longer exists. +pub fn terminate_process_group(process_group_id: u32) -> io::Result { + use std::io::ErrorKind; + + let pgid = process_group_id as libc::pid_t; + let result = unsafe { libc::killpg(pgid, libc::SIGTERM) }; + if result == -1 { + let err = io::Error::last_os_error(); + if err.kind() == ErrorKind::NotFound { + return Ok(false); + } + return Err(err); + } + + Ok(true) +} + +#[cfg(not(unix))] +/// No-op on non-Unix platforms. +pub fn terminate_process_group(_process_group_id: u32) -> io::Result { + Ok(false) +} + #[cfg(unix)] /// Kill a specific process group ID (best-effort). pub fn kill_process_group(process_group_id: u32) -> io::Result<()> {