[codex] Add external agent import result accounting#28008
[codex] Add external agent import result accounting#28008charlesgong-openai wants to merge 1 commit into
Conversation
charlesgong-openai
left a comment
charlesgong-openai
left a comment
There was a problem hiding this comment.
Added inline walkthrough notes for the main PR1 sections: protocol contract, delivery classification, service accounting, request processor aggregation, session behavior, and tests.
| #[ts(export_to = "v2/")] | ||
| pub struct ExternalAgentConfigImportResponse {} | ||
| pub struct ExternalAgentConfigImportResponse { | ||
| pub import_id: String, |
There was a problem hiding this comment.
Walkthrough: this is the correlation hook for the import RPC. The response now returns the generated importId so the app can bind later completed/progress notifications back to the request that started the import.
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct ExternalAgentConfigImportCompletedNotification {} | ||
| pub struct ExternalAgentConfigImportRawError { |
There was a problem hiding this comment.
Walkthrough: rawErrors are the per-failure details the UI can surface or log. They carry the item type, failure stage, message, optional cwd, and optional source path/name without making the whole import fail.
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct ExternalAgentConfigImportItemResult { |
There was a problem hiding this comment.
Walkthrough: itemResults at the progress layer are step-scoped. They keep the description/cwd so intermediate events can say which concrete migration item just ran.
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct ExternalAgentConfigImportTypeResult { |
There was a problem hiding this comment.
Walkthrough: completed itemResults are type-scoped. The final notification collapses duplicate migration steps by itemType and reports artifact-level success/error counts plus all raw errors.
| 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.
Walkthrough: this registers the progress notification method in the v2 protocol. PR1 defines the wire shape; PR2 is the slice that starts emitting these progress events.
| request_id: ConnectionRequestId, | ||
| params: ExternalAgentConfigImportParams, | ||
| ) -> Result<(), JSONRPCErrorError> { | ||
| let import_id = Uuid::new_v4().to_string(); |
There was a problem hiding this comment.
Walkthrough: the importId is generated before import work starts and returned in the immediate RPC response. Completed notifications reuse the same ID for request correlation.
| { | ||
| Ok(Some(canonical_path)) => canonical_path, | ||
| Ok(None) => { | ||
| record_import_error( |
There was a problem hiding this comment.
Walkthrough: selected session validation is no longer all-or-nothing. Undetected or unresolvable sessions become raw item errors, while valid sessions continue into the import path.
| } | ||
| } | ||
|
|
||
| fn completed_notification( |
There was a problem hiding this comment.
Walkthrough: this is the final API aggregation step. It groups item outcomes by itemType, adds success/error counts, concatenates raw errors, and sorts the result so clients and tests get stable ordering.
| } | ||
|
|
||
| pub(super) async fn import_sessions(&self, sessions: Vec<ExternalAgentSessionMigration>) { | ||
| pub(super) async fn import_sessions( |
There was a problem hiding this comment.
Walkthrough: session import now returns an item result instead of only side-effecting. That lets background session imports contribute success and error counts to ExternalAgentConfigImportCompleted.
| assert_eq!(completed.import_id, import_id); | ||
| let raw_errors = completed.item_results[0].raw_errors.clone(); | ||
| assert_eq!( | ||
| completed.item_results, |
There was a problem hiding this comment.
Walkthrough: this integration test locks the mixed-session behavior. One valid session is imported and one undetected session is reported as a raw error in the same completed payload.
1 participant
Summary
Adds the import response and completed notification contract needed to correlate external-agent config imports and report artifact-level import results.
This PR includes:
importIdinexternalAgentConfig/importresponsesitemResultsgrouped by migration item type0successesStack note: this is based on
codex/fix-image-generation-path-uribecause currentorigin/maindoes not include the image-generationPathUricompile fix and app-server tests fail before reaching this change without that base.Validation
just test -p codex-app-server-protocolenv -u CODEX_SQLITE_HOME just test -p codex-app-server external_agent_configjust test -p codex-app-server-client event_requires_deliveryjust fix -p codex-app-server -p codex-app-server-protocol -p codex-app-server-clientgit diff --check