Skip to content

Commit 9090dd7

Browse files
committed
codex_sdk: repair gaps from the typed plugin API review
Fix any remaining typed plugin API, documentation, or test issues and rerun the full Phase F quality floor.
1 parent 23b0985 commit 9090dd7

6 files changed

Lines changed: 230 additions & 48 deletions

File tree

README.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,30 @@ the under-development approval features only inside a temporary `CODEX_HOME`, so
370370
live command/file/permissions approval flows without mutating your real Codex settings or writing
371371
inside this repository.
372372

373+
Raw versus typed plugin calls:
374+
375+
```elixir
376+
alias Codex.AppServer
377+
alias Codex.Protocol.Plugin
378+
379+
{:ok, raw} =
380+
AppServer.plugin_read(conn, "/tmp/marketplace.json", "demo-plugin")
381+
382+
{:ok, %Plugin.ReadResponse{plugin: plugin}} =
383+
AppServer.request_typed(
384+
conn,
385+
"plugin/read",
386+
%Plugin.ReadParams{
387+
marketplace_path: "/tmp/marketplace.json",
388+
plugin_name: "demo-plugin"
389+
},
390+
Plugin.ReadResponse
391+
)
392+
393+
IO.inspect(raw["plugin"]["apps"], label: "raw apps")
394+
IO.inspect(plugin.apps, label: "typed apps")
395+
```
396+
373397
App-server v2 input blocks support `text`, `image`, `localImage`, `skill`, and `mention`.
374398
Legacy app-server v1 conversation flows are available via `Codex.AppServer.V1`.
375399

guides/11-typed-plugin-api.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,22 @@ alias Codex.Protocol.Plugin
3737
)
3838
```
3939

40+
Raw and typed usage can live side by side:
41+
42+
```elixir
43+
alias Codex.AppServer
44+
alias Codex.Protocol.Plugin
45+
46+
{:ok, raw} =
47+
AppServer.plugin_read(conn, "/tmp/marketplace.json", "demo-plugin")
48+
49+
{:ok, %Plugin.ReadResponse{plugin: plugin}} =
50+
AppServer.plugin_read_typed(conn, "/tmp/marketplace.json", "demo-plugin")
51+
52+
IO.inspect(raw["plugin"]["apps"], label: "raw apps")
53+
IO.inspect(plugin.apps, label: "typed apps")
54+
```
55+
4056
## Typed Params
4157

4258
Typed plugin params own app-server wire encoding:
@@ -91,5 +107,10 @@ upstream JSON shape directly. Use the typed wrappers when you want:
91107
- preserved `extra` metadata
92108
- repo-local parse errors instead of raw validation internals
93109

110+
Nested response validation failures also stay on the outer response contract.
111+
For example, an invalid `plugin/list` payload still returns
112+
`{:error, {:invalid_plugin_list_response, details}}` instead of leaking nested
113+
schema exceptions.
114+
94115
The typed plugin models remain local to `codex_sdk`; they are not moved into the
95116
shared runtime-core repos.

lib/codex/app_server.ex

Lines changed: 67 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -208,9 +208,8 @@ defmodule Codex.AppServer do
208208
def request_typed(conn, method, params, response_module, opts \\ [])
209209
when is_pid(conn) and is_binary(method) and is_atom(response_module) and is_list(opts) do
210210
with {:ok, encoded_params} <- encode_typed_request_params(method, params),
211-
{:ok, result} <- Connection.request(conn, method, encoded_params, opts),
212-
{:ok, typed_response} <- parse_typed_response(response_module, result) do
213-
{:ok, typed_response}
211+
{:ok, result} <- Connection.request(conn, method, encoded_params, opts) do
212+
parse_typed_response(response_module, result)
214213
end
215214
end
216215

@@ -891,12 +890,14 @@ defmodule Codex.AppServer do
891890
"""
892891
@spec plugin_list(connection(), keyword()) :: {:ok, map()} | {:error, term()}
893892
def plugin_list(conn, opts \\ []) when is_pid(conn) and is_list(opts) do
894-
params = %Plugin.ListParams{
895-
cwds: Keyword.get(opts, :cwds),
896-
force_remote_sync: Plugin.Helpers.raw_true?(Keyword.get(opts, :force_remote_sync))
897-
}
898-
899-
Connection.request(conn, "plugin/list", Plugin.ListParams.to_map(params), timeout_ms: 30_000)
893+
with {:ok, params} <-
894+
encode_plugin_request_params(
895+
"plugin/list",
896+
cwds: Keyword.get(opts, :cwds),
897+
force_remote_sync: Keyword.get(opts, :force_remote_sync)
898+
) do
899+
Connection.request(conn, "plugin/list", params, timeout_ms: 30_000)
900+
end
900901
end
901902

902903
@doc """
@@ -922,18 +923,15 @@ defmodule Codex.AppServer do
922923
def plugin_install(conn, marketplace_path, plugin_name, opts \\ [])
923924
when is_pid(conn) and is_binary(marketplace_path) and is_binary(plugin_name) and
924925
is_list(opts) do
925-
params = %Plugin.InstallParams{
926-
marketplace_path: marketplace_path,
927-
plugin_name: plugin_name,
928-
force_remote_sync: Plugin.Helpers.raw_true?(Keyword.get(opts, :force_remote_sync))
929-
}
930-
931-
Connection.request(
932-
conn,
933-
"plugin/install",
934-
Plugin.InstallParams.to_map(params),
935-
timeout_ms: 30_000
936-
)
926+
with {:ok, params} <-
927+
encode_plugin_request_params(
928+
"plugin/install",
929+
marketplace_path: marketplace_path,
930+
plugin_name: plugin_name,
931+
force_remote_sync: Keyword.get(opts, :force_remote_sync)
932+
) do
933+
Connection.request(conn, "plugin/install", params, timeout_ms: 30_000)
934+
end
937935
end
938936

939937
@doc """
@@ -965,14 +963,14 @@ defmodule Codex.AppServer do
965963
@spec plugin_read(connection(), String.t(), String.t()) :: {:ok, map()} | {:error, term()}
966964
def plugin_read(conn, marketplace_path, plugin_name)
967965
when is_pid(conn) and is_binary(marketplace_path) and is_binary(plugin_name) do
968-
params = %Plugin.ReadParams{marketplace_path: marketplace_path, plugin_name: plugin_name}
969-
970-
Connection.request(
971-
conn,
972-
"plugin/read",
973-
Plugin.ReadParams.to_map(params),
974-
timeout_ms: 30_000
975-
)
966+
with {:ok, params} <-
967+
encode_plugin_request_params(
968+
"plugin/read",
969+
marketplace_path: marketplace_path,
970+
plugin_name: plugin_name
971+
) do
972+
Connection.request(conn, "plugin/read", params, timeout_ms: 30_000)
973+
end
976974
end
977975

978976
@doc """
@@ -1003,17 +1001,14 @@ defmodule Codex.AppServer do
10031001
@spec plugin_uninstall(connection(), String.t(), keyword()) :: {:ok, map()} | {:error, term()}
10041002
def plugin_uninstall(conn, plugin_id, opts \\ [])
10051003
when is_pid(conn) and is_binary(plugin_id) and is_list(opts) do
1006-
params = %Plugin.UninstallParams{
1007-
plugin_id: plugin_id,
1008-
force_remote_sync: Plugin.Helpers.raw_true?(Keyword.get(opts, :force_remote_sync))
1009-
}
1010-
1011-
Connection.request(
1012-
conn,
1013-
"plugin/uninstall",
1014-
Plugin.UninstallParams.to_map(params),
1015-
timeout_ms: 30_000
1016-
)
1004+
with {:ok, params} <-
1005+
encode_plugin_request_params(
1006+
"plugin/uninstall",
1007+
plugin_id: plugin_id,
1008+
force_remote_sync: Keyword.get(opts, :force_remote_sync)
1009+
) do
1010+
Connection.request(conn, "plugin/uninstall", params, timeout_ms: 30_000)
1011+
end
10171012
end
10181013

10191014
@doc """
@@ -1327,14 +1322,23 @@ defmodule Codex.AppServer do
13271322
defp encode_command_exec_delta(delta) when is_binary(delta), do: Base.encode64(delta)
13281323
defp encode_command_exec_delta(delta), do: delta
13291324

1325+
defp encode_plugin_request_params(method, params) when is_binary(method) and is_list(params) do
1326+
params
1327+
|> normalize_plugin_request_params(method)
1328+
|> then(&encode_typed_request_params(method, &1))
1329+
end
1330+
13301331
defp encode_typed_request_params(method, nil) do
13311332
case typed_params_module(method) do
13321333
nil ->
13331334
{:ok, %{}}
13341335

1335-
module when is_atom(module) ->
1336-
with {:ok, typed_params} <- apply(module, :parse, [%{}]) do
1337-
{:ok, apply(module, :to_map, [typed_params])}
1336+
module when is_atom(module) and not is_nil(module) ->
1337+
parser = :erlang.make_fun(module, :parse, 1)
1338+
encoder = :erlang.make_fun(module, :to_map, 1)
1339+
1340+
with {:ok, typed_params} <- parser.(%{}) do
1341+
{:ok, encoder.(typed_params)}
13381342
end
13391343
end
13401344
end
@@ -1352,9 +1356,12 @@ defmodule Codex.AppServer do
13521356
nil ->
13531357
{:ok, Params.normalize_map(params)}
13541358

1355-
module when is_atom(module) ->
1356-
with {:ok, typed_params} <- apply(module, :parse, [params]) do
1357-
{:ok, apply(module, :to_map, [typed_params])}
1359+
module when is_atom(module) and not is_nil(module) ->
1360+
parser = :erlang.make_fun(module, :parse, 1)
1361+
encoder = :erlang.make_fun(module, :to_map, 1)
1362+
1363+
with {:ok, typed_params} <- parser.(params) do
1364+
{:ok, encoder.(typed_params)}
13581365
end
13591366
end
13601367
end
@@ -1375,6 +1382,9 @@ defmodule Codex.AppServer do
13751382
{:error, {:invalid_typed_response_module, response_module}}
13761383
end
13771384
rescue
1385+
error in [CliSubprocessCore.Schema.Error] ->
1386+
{:error, {error.tag, error.details}}
1387+
13781388
error in [ArgumentError, RuntimeError] ->
13791389
{:error, {:invalid_typed_response, response_module, error}}
13801390
end
@@ -1385,6 +1395,17 @@ defmodule Codex.AppServer do
13851395
defp typed_params_module("plugin/uninstall"), do: Plugin.UninstallParams
13861396
defp typed_params_module(_method), do: nil
13871397

1398+
defp normalize_plugin_request_params(params, method)
1399+
when method in ["plugin/list", "plugin/install", "plugin/uninstall"] and is_list(params) do
1400+
if Keyword.has_key?(params, :force_remote_sync) do
1401+
Keyword.update!(params, :force_remote_sync, &Plugin.Helpers.raw_true?/1)
1402+
else
1403+
params
1404+
end
1405+
end
1406+
1407+
defp normalize_plugin_request_params(params, _method) when is_list(params), do: params
1408+
13881409
defp normalize_true(true), do: true
13891410
defp normalize_true("true"), do: true
13901411
defp normalize_true(_), do: nil

lib/codex/protocol/plugin/helpers.ex

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ defmodule Codex.Protocol.Plugin.Helpers do
22
@moduledoc false
33

44
alias CliSubprocessCore.Schema.Conventions
5+
alias CliSubprocessCore.Schema.Error, as: SchemaError
56
alias Codex.Schema
67

78
@spec required_string() :: Zoi.schema()
@@ -60,7 +61,12 @@ defmodule Codex.Protocol.Plugin.Helpers do
6061
def parse(schema, value, tag, key_mapping, projector) when is_function(projector, 1) do
6162
case Schema.parse(schema, normalize_input(value, key_mapping), tag) do
6263
{:ok, parsed} ->
63-
{:ok, projector.(parsed)}
64+
try do
65+
{:ok, projector.(parsed)}
66+
rescue
67+
error in [SchemaError] ->
68+
{:error, {tag, error.details}}
69+
end
6470

6571
{:error, {^tag, details}} ->
6672
{:error, {tag, details}}
@@ -72,6 +78,9 @@ defmodule Codex.Protocol.Plugin.Helpers do
7278
schema
7379
|> Schema.parse!(normalize_input(value, key_mapping), tag)
7480
|> projector.()
81+
rescue
82+
error in [SchemaError] ->
83+
reraise SchemaError, [tag: tag, details: error.details], __STACKTRACE__
7584
end
7685

7786
@spec split_extra(map(), [String.t()]) :: {map(), map()}

test/codex/app_server/api_test.exs

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@ defmodule Codex.AppServer.ApiTest do
44
alias Codex.AppServer
55
alias Codex.AppServer.Connection
66
alias Codex.AppServer.Protocol
7-
alias Codex.Protocol.Plugin
87
alias Codex.Options
8+
alias Codex.Protocol.Plugin
99
alias Codex.TestSupport.AppServerSubprocess
1010

1111
setup do
@@ -1102,6 +1102,35 @@ defmodule Codex.AppServer.ApiTest do
11021102
Task.await(task, 200)
11031103
end
11041104

1105+
test "request_typed/5 remains generic for non-plugin methods with plain params", %{
1106+
conn: conn,
1107+
os_pid: os_pid
1108+
} do
1109+
task =
1110+
Task.async(fn ->
1111+
AppServer.request_typed(
1112+
conn,
1113+
"experimentalFeature/list",
1114+
[cursor: "cur"],
1115+
Plugin.UninstallResponse,
1116+
timeout_ms: 30_000
1117+
)
1118+
end)
1119+
1120+
assert_receive {:app_server_subprocess_send, ^conn, request_line}
1121+
1122+
assert {:ok,
1123+
%{
1124+
"id" => req_id,
1125+
"method" => "experimentalFeature/list",
1126+
"params" => %{"cursor" => "cur"}
1127+
}} = Jason.decode(request_line)
1128+
1129+
send(conn, {:stdout, os_pid, Protocol.encode_response(req_id, %{"data" => []})})
1130+
1131+
assert {:ok, %Plugin.UninstallResponse{extra: %{"data" => []}}} = Task.await(task, 200)
1132+
end
1133+
11051134
test "request_typed/5 returns adapted parse errors for invalid typed payloads", %{
11061135
conn: conn,
11071136
os_pid: os_pid
@@ -1129,6 +1158,55 @@ defmodule Codex.AppServer.ApiTest do
11291158
assert is_list(details.issues)
11301159
end
11311160

1161+
test "request_typed/5 returns adapted parse errors for invalid nested typed payloads", %{
1162+
conn: conn,
1163+
os_pid: os_pid
1164+
} do
1165+
task =
1166+
Task.async(fn ->
1167+
AppServer.request_typed(
1168+
conn,
1169+
"plugin/list",
1170+
%Plugin.ListParams{},
1171+
Plugin.ListResponse,
1172+
timeout_ms: 30_000
1173+
)
1174+
end)
1175+
1176+
assert_receive {:app_server_subprocess_send, ^conn, request_line}
1177+
1178+
assert {:ok, %{"id" => req_id, "method" => "plugin/list"}} = Jason.decode(request_line)
1179+
1180+
send(
1181+
conn,
1182+
{:stdout, os_pid,
1183+
Protocol.encode_response(req_id, %{
1184+
"marketplaces" => [
1185+
%{
1186+
"name" => "codex-curated",
1187+
"path" => "/tmp/marketplace.json",
1188+
"plugins" => [
1189+
%{
1190+
"id" => "demo-plugin@codex-curated",
1191+
"name" => "demo-plugin",
1192+
"source" => %{"type" => "local", "path" => "/tmp/plugins/demo-plugin"},
1193+
"installed" => "yes",
1194+
"enabled" => true,
1195+
"installPolicy" => "AVAILABLE",
1196+
"authPolicy" => "ON_INSTALL"
1197+
}
1198+
]
1199+
}
1200+
]
1201+
})}
1202+
)
1203+
1204+
assert {:error, {:invalid_plugin_list_response, details}} = Task.await(task, 200)
1205+
assert is_binary(details.message)
1206+
assert is_map(details.errors)
1207+
assert is_list(details.issues)
1208+
end
1209+
11321210
test "typed plugin wrappers preserve wire parity and return typed structs", %{
11331211
conn: conn,
11341212
os_pid: os_pid

0 commit comments

Comments
 (0)