From e546009190c34b2f5f46c8e64faea74c4b1916f6 Mon Sep 17 00:00:00 2001
From: Mohamed Boudra <boudra.moha@gmail.com>
Date: Thu, 25 Dec 2025 00:38:17 +0700
Subject: [PATCH] Review Codex MCP refactor issues

---
 REPORT-codex-mcp-refactor-review.md | 23 +++++++++++++++++++++++
 plan.md                             | 13 ++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)
 create mode 100644 REPORT-codex-mcp-refactor-review.md

diff --git a/REPORT-codex-mcp-refactor-review.md b/REPORT-codex-mcp-refactor-review.md
new file mode 100644
index 000000000..eb9b91033
--- /dev/null
+++ b/REPORT-codex-mcp-refactor-review.md
@@ -0,0 +1,23 @@
+# Codex MCP refactor review
+
+Scope:
+- packages/server/src/server/agent/providers/codex-mcp-agent.ts
+- packages/server/src/server/agent/providers/codex-mcp-agent.test.ts
+- packages/server/src/server/agent/agent-sdk-types.ts
+
+Findings
+
+1) Multi-key normalization remains in Codex MCP schemas (still “guessing” across multiple key names).
+- Evidence:
+  - Session identifiers normalize conversation id from multiple keys in `packages/server/src/server/agent/providers/codex-mcp-agent.ts:265-282`.
+  - Read file items accept `path`, `file_path`, and `filePath` and merge output content from multiple fields in `packages/server/src/server/agent/providers/codex-mcp-agent.ts:605-650`.
+  - MCP tool calls normalize `server`/`tool` across many alternative keys in `packages/server/src/server/agent/providers/codex-mcp-agent.ts:654-717`.
+  - Permission params normalize `call_id`/`codex_call_id`/`codex_event_id` and `command`/`codex_command`, `cwd`/`codex_cwd` in `packages/server/src/server/agent/providers/codex-mcp-agent.ts:939-980`.
+
+2) Related type definitions still use `Record<string, unknown>`.
+- Evidence:
+  - `packages/server/src/server/agent/agent-sdk-types.ts:17`, `:34`, `:94`, `:103`, `:105`, `:111`, `:132`, `:167`, `:170`.
+
+Notes
+- No `as` casts or `??` fallbacks were found in `packages/server/src/server/agent/providers/codex-mcp-agent.ts` or `packages/server/src/server/agent/providers/codex-mcp-agent.test.ts`.
+- No `Record<string, unknown>` usage remains in the Codex MCP provider or its tests.
diff --git a/plan.md b/plan.md
index 5e7728043..0f2fa4cae 100644
--- a/plan.md
+++ b/plan.md
@@ -518,11 +518,18 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov
 
   - **Done (2025-12-25 00:34)**: WHAT: replaced Codex MCP event parsing with Zod-normalized schemas, strict thread/item handling, and typed permission/config parsing in `packages/server/src/server/agent/providers/codex-mcp-agent.ts:856`, `packages/server/src/server/agent/providers/codex-mcp-agent.ts:904`, `packages/server/src/server/agent/providers/codex-mcp-agent.ts:1951`, `packages/server/src/server/agent/providers/codex-mcp-agent.ts:2447`; removed dynamic import/cast helpers and added Zod-based parsing utilities in `packages/server/src/server/agent/providers/codex-mcp-agent.test.ts:98`, `packages/server/src/server/agent/providers/codex-mcp-agent.test.ts:131`, `packages/server/src/server/agent/providers/codex-mcp-agent.test.ts:233`; added provider_event to stream event type in `packages/server/src/server/agent/agent-sdk-types.ts:77`. RESULT: Codex MCP provider/test utilities no longer use `as` casts, `Record<string, unknown>`, or `??` fallbacks; events are validated via Zod at the boundary with typed handling. EVIDENCE: `rg -n "as |Record<string, unknown>|\\?\\?" packages/server/src/server/agent/providers/codex-mcp-agent.ts packages/server/src/server/agent/providers/codex-mcp-agent.test.ts` (no matches).
 
-- [ ] **Review**: Verify CRITICAL REFACTOR removed all flagged issues.
+- [x] **Review**: Verify CRITICAL REFACTOR removed all flagged issues.
 
   - Check `codex-mcp-agent.ts`, `codex-mcp-agent.test.ts`, and related files for:
     - `as` casts (outside Zod parse outputs)
     - `Record<string, unknown>` or ad‑hoc casts
-    - `??` fallbacks on required fields
-    - multi‑key guessing for the same field
+  - `??` fallbacks on required fields
+  - multi‑key guessing for the same field
   - If any remain, add a follow‑up fix task immediately after this review.
+  - **Done (2025-12-25 00:37)**: WHAT: documented remaining multi-key normalization and lingering Record types in `REPORT-codex-mcp-refactor-review.md:1` and marked review complete in `plan.md:521`. RESULT: review confirms no `as`/`??`/Record usage in Codex MCP provider/test files but identifies remaining multi-key normalization and related Record types in `agent-sdk-types.ts`. EVIDENCE: `rg -n "\\bas\\b|Record<string, unknown>|\\?\\?" packages/server/src/server/agent/providers/codex-mcp-agent.ts packages/server/src/server/agent/providers/codex-mcp-agent.test.ts packages/server/src/server/agent/agent-sdk-types.ts` and report contents.
+
+- [ ] **Fix**: Eliminate remaining multi-key normalization in Codex MCP schemas.
+
+  - Replace `firstString`-based normalization with explicit Zod discriminated unions per event variant (single canonical key per variant) and fail fast on unknown shapes.
+  - Remove multi-key permission/call-id normalization by defining canonical permission event schemas and updating tests/emitters accordingly.
+  - Re-evaluate `Record<string, unknown>` usage in `agent-sdk-types.ts` and replace with explicit types where possible.