[codex] Record external agent import results#28396
Conversation
4918f16 to
5f4020e
Compare
| @@ -1602,6 +1602,7 @@ server_notification_definitions! { | |||
| AccountRateLimitsUpdated => "account/rateLimits/updated" (v2::AccountRateLimitsUpdatedNotification), | |||
| AppListUpdated => "app/list/updated" (v2::AppListUpdatedNotification), | |||
| RemoteControlStatusChanged => "remoteControl/status/changed" (v2::RemoteControlStatusChangedNotification), | |||
| ExternalAgentConfigImportProgress => "externalAgentConfig/import/progress" (v2::ExternalAgentConfigImportProgressNotification), | |||
There was a problem hiding this comment.
Registers the new best-effort import progress notification method in the server notification union so app-server clients can receive per-item import updates before the final completion notification.
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct ExternalAgentConfigImportProgressNotification { |
There was a problem hiding this comment.
Adds the progress notification payload. The itemTypeResults array is currently sent with one result per progress event, but leaves room for batching later without changing the wire shape.
| if let Some((source, target)) = | ||
| self.import_config(migration_item.cwd.as_deref())? | ||
| { | ||
| item_result.record_success(Some(source), Some(target)); |
There was a problem hiding this comment.
Records config imports as a concrete source -> target success when config.toml is created or merged, instead of only incrementing a count.
| ); | ||
| item_result.record_successes(skills_count); | ||
| for skill_name in imported_skills { |
There was a problem hiding this comment.
Keeps the existing metric as a count, but records each imported skill name individually so success details can show exactly which skills migrated.
| self.import_mcp_server_config(migration_item.cwd.as_deref())?; | ||
| emit_migration_metric( | ||
| EXTERNAL_AGENT_CONFIG_IMPORT_METRIC, | ||
| ExternalAgentConfigMigrationItemType::McpServerConfig, | ||
| /*skills_count*/ None, | ||
| ); | ||
| item_result.record_successes(migrated_count); | ||
| for server_name in migrated_server_names { |
There was a problem hiding this comment.
Records migrated MCP server names individually, which gives the completion payload and DB row the exact server ids instead of only the number of merged servers.
| emit_migration_metric( | ||
| EXTERNAL_AGENT_CONFIG_IMPORT_METRIC, | ||
| ExternalAgentConfigMigrationItemType::Hooks, | ||
| /*skills_count*/ None, | ||
| ); | ||
| item_result.record_successes(migrated_count); | ||
| for hook_name in migrated_hook_names { |
There was a problem hiding this comment.
Converts hook import success from a boolean/count into the concrete hook event names that were copied into hooks.json.
| ); | ||
| item_result.record_successes(commands_count); | ||
| for command_name in imported_commands { |
There was a problem hiding this comment.
Records each imported slash command name as an individual success after command files are converted into skill directories.
| ); | ||
| item_result.record_successes(subagents_count); | ||
| for subagent_name in imported_subagents { |
There was a problem hiding this comment.
Records each imported subagent name as a success, while still using the list length for telemetry count compatibility.
| } | ||
|
|
||
| let Some(target_parent) = target_config.parent() else { | ||
| return Err(invalid_data_error("config target path has no parent")); | ||
| }; | ||
| fs::create_dir_all(target_parent)?; | ||
| if !target_config.exists() { | ||
| let migrated_count = migrated_mcp_server_names(&migrated).len(); | ||
| let migrated_server_names = migrated_mcp_server_names(&migrated); |
There was a problem hiding this comment.
Captures the MCP server names before writing a new config file, so a fresh config migration reports the same concrete server ids as a merge migration.
| &source_external_agent_dir, | ||
| &target_hooks, | ||
| )?)) | ||
| let hook_names = hook_migration_event_names(&source_external_agent_dir, &target_hooks)?; |
There was a problem hiding this comment.
Reads the hook event names before importing hooks so the method can return the specific events copied when hooks.json is created.
| @@ -986,7 +999,10 @@ impl ExternalAgentConfigService { | |||
| fs::create_dir_all(target_parent)?; | |||
| if !target_config.exists() { | |||
| write_toml_file(&target_config, &migrated)?; | |||
| return Ok(1); | |||
| return Ok(Some(( | |||
There was a problem hiding this comment.
Returns the source settings path and target config path when config migration actually writes data; callers use this pair as the structured success detail.
| @@ -1150,20 +1171,20 @@ impl ExternalAgentConfigService { | |||
| } | |||
|
|
|||
| copy_dir_recursive(&entry.path(), &target)?; | |||
| copied_count += 1; | |||
| copied_names.push(entry.file_name().to_string_lossy().to_string()); | |||
There was a problem hiding this comment.
Collects copied skill directory names as success details; skipped existing directories remain omitted from the success list.
| state_db: Option<StateDbHandle>, | ||
| } | ||
|
|
||
| pub(crate) struct ExternalAgentConfigRequestProcessorArgs { |
There was a problem hiding this comment.
Collects constructor dependencies into a named args struct. That keeps the state_db addition readable at the callsite and avoids a long positional constructor.
| arg0_paths, | ||
| config.codex_home.to_path_buf(), | ||
| ); | ||
| let external_agent_config_processor = |
There was a problem hiding this comment.
Threads the state DB handle into the external-agent import processor so completed import notifications can be persisted locally by import id.
| @@ -207,23 +227,31 @@ impl ExternalAgentConfigRequestProcessor { | |||
|
|
|||
| let mut completed_item_results = Vec::new(); | |||
| if let Some(session_validation_result) = session_validation_result { | |||
| send_import_progress(&self.outgoing, &import_id, &session_validation_result).await; | |||
There was a problem hiding this comment.
Sends a best-effort progress event as each synchronous item finishes, before adding that item to the eventual completion payload.
| let session_imports = async move { | ||
| let session_import_result = session_import_result?; | ||
| let item_result = session_importer | ||
| .import_sessions(pending_session_imports, session_import_result) | ||
| .await; | ||
| send_import_progress(&session_progress_outgoing, &session_import_id, &item_result) |
There was a problem hiding this comment.
Background session imports also emit progress when they finish, so long-running async work is visible before the final must-deliver completion notification.
| import_id: String, | ||
| item_results: &[CoreImportItemResult], | ||
| ) { | ||
| let notification = completed_notification(import_id, item_results); | ||
| if let Some(state_db) = state_db |
There was a problem hiding this comment.
Persists the must-deliver completion notification before sending it. Persistence failures are logged and do not prevent the client notification from being sent.
| @@ -265,6 +299,12 @@ impl ExternalAgentConfigRequestProcessor { | |||
| ); | |||
| } | |||
| } | |||
| send_import_progress( | |||
There was a problem hiding this comment.
Background plugin imports emit their own progress item after each marketplace/plugin group finishes, then those results are folded into the final completion payload.
| .as_str() | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| Ok(ExternalAgentConfigImportResultRecord { |
There was a problem hiding this comment.
Converts the protocol completion payload into typed state DB records, preserving the full successes/failures arrays instead of derived success/failure counts.
| @@ -155,13 +155,13 @@ pub fn missing_subagent_names( | |||
| Ok(names) | |||
| } | |||
|
|
|||
| pub fn import_subagents(source_agents: &Path, target_agents: &Path) -> io::Result<usize> { | |||
| pub fn import_subagents(source_agents: &Path, target_agents: &Path) -> io::Result<Vec<String>> { | |||
There was a problem hiding this comment.
Changes subagent import to return the names it actually wrote, allowing the app-server import result to list migrated subagents rather than only a count.
| @@ -195,13 +195,13 @@ pub fn missing_command_names( | |||
| .collect()) | |||
| } | |||
|
|
|||
| pub fn import_commands(source_commands: &Path, target_skills: &Path) -> io::Result<usize> { | |||
| pub fn import_commands(source_commands: &Path, target_skills: &Path) -> io::Result<Vec<String>> { | |||
There was a problem hiding this comment.
Changes command import to return the skill names it created from command files, so command migration successes can be reported by name.
| @@ -0,0 +1,18 @@ | |||
| CREATE TABLE external_agent_config_imports ( | |||
There was a problem hiding this comment.
Creates the top-level import completion table keyed by import_id. This stores the full completion notification JSON for later lookup or diagnostics.
| use sqlx::Row; | ||
| use std::path::PathBuf; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] |
There was a problem hiding this comment.
Defines the DB-facing success object. It mirrors the notification success shape but uses plain strings for item_type so stored JSON is decoupled from protocol enum serialization.
| pub target: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] |
There was a problem hiding this comment.
Defines the DB-facing failure object with stage, message, cwd, and source so all failed migration items are stored with actionable context.
| } | ||
|
|
||
| impl StateRuntime { | ||
| pub async fn record_external_agent_config_import_completed( |
There was a problem hiding this comment.
Adds the state runtime write API for import completion. It stores the full completion notification plus structured per-type result records inside one transaction.
| completed_notification_json TEXT NOT NULL | ||
| ); | ||
|
|
||
| CREATE TABLE external_agent_config_import_results ( |
There was a problem hiding this comment.
Creates one ordered row per migration item type result. Successes and failures are JSON arrays of the full typed objects rather than separate count columns.
| .map_err(Into::into) | ||
| } | ||
|
|
||
| pub async fn external_agent_config_import_result_records( |
There was a problem hiding this comment.
Adds the read API for structured result rows, deserializing successes/failures back into typed arrays and preserving original result ordering.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn external_agent_config_import_completed_notification_json( |
There was a problem hiding this comment.
Adds a lookup for the exact stored completion notification JSON by import id. This gives callers a durable copy of the must-deliver completion event.
| @@ -161,6 +161,7 @@ pub(super) fn server_notification_thread_target( | |||
| | ServerNotification::AccountRateLimitsUpdated(_) | |||
| | ServerNotification::AppListUpdated(_) | |||
| | ServerNotification::RemoteControlStatusChanged(_) | |||
| | ServerNotification::ExternalAgentConfigImportProgress(_) | |||
There was a problem hiding this comment.
Treats import progress as a global/non-thread-targeted notification in the TUI routing layer, matching the completed import notification behavior.
| @@ -56,6 +56,9 @@ pub use model::ThreadGoalStatus; | |||
| pub use model::ThreadMetadata; | |||
| pub use model::ThreadMetadataBuilder; | |||
There was a problem hiding this comment.
Re-exports the new import-result record types from codex-state so app-server can persist typed successes and failures without depending on private runtime modules.
| @@ -205,6 +205,7 @@ impl ChatWidget { | |||
| | ServerNotification::McpServerOauthLoginCompleted(_) | |||
| | ServerNotification::AppListUpdated(_) | |||
| | ServerNotification::RemoteControlStatusChanged(_) | |||
| | ServerNotification::ExternalAgentConfigImportProgress(_) | |||
There was a problem hiding this comment.
Ignores import progress in the chat widget protocol handler because this notification is app-server state/progress metadata, not a thread item to render in the transcript.
5170105 to
09d54a0
Compare
alexsong-oai
left a comment
There was a problem hiding this comment.
Are we going to show more detailed import results in the client? including TUI, desktop app, etc?
yep |
1833bcf to
5b76cae
Compare
Summary
externalAgentConfig/import/progressnotifications while keepingexternalAgentConfig/import/completedas the must-deliver eventimportId, including concrete success/failure details for config, AGENTS.md, skills, plugins, MCP servers, subagents, hooks, commands, and sessionsexternalAgentConfig/import/readHistoriesso clients can recover persisted import results after missing the live completion notificationerrorTypeon import failures in protocol responses/notifications and persisted DB JSON so future code can classify failures without another wire/storage shape changeValidation
git diff --checkjust test -p codex-state external_agent_config_importsjust test -p codex-app-server-protocolCODEX_SQLITE_HOME=/private/tmp/codex-app-server-sqlite-read-details just test -p codex-app-server external_agent_config_import_sends_completion_notification_for_sync_only_importAlso ran earlier broader checks before publishing:
just test -p codex-stateCODEX_SQLITE_HOME=/private/tmp/codex-app-server-external-agent-test-sqlite just test -p codex-app-server external_agent_configjust test -p codex-external-agent-migration