diff --git a/lib/trinity/mcp/server/plug.ex b/lib/trinity/mcp/server/plug.ex index e876872..150cfcd 100644 --- a/lib/trinity/mcp/server/plug.ex +++ b/lib/trinity/mcp/server/plug.ex @@ -32,10 +32,12 @@ defmodule Trinity.MCP.Server.Plug do catalog: Catalog, dispatch: nil, # Slice 062: the profile's authorization runs in `call/2` before the transport, which - # then finds the principal it left; the transport's own hook is that second look. - authorize: fn _conn -> - if Process.get(@principal_key), do: :ok, else: {:error, :unauthorized} - end, + # then finds the principal it left; the transport's own hook is that second look. A + # remote capture and never an anonymous function: `Plug.Builder` calls `init/1` at + # compile time in `:prod` and escapes what it returns into the endpoint, and a closure + # cannot be escaped (`fix(s062)`: the release build was broken and the gate, which runs + # in `:test` where `init/1` is called at runtime, could not see it). + authorize: &__MODULE__.authorized/1, allowed_origins: Keyword.get(config, :allowed_origins, loopback_origins()), server_name: Keyword.get(config, :server_name, "trinity"), tools_ttl_ms: Keyword.get(config, :tools_ttl_ms, 60_000), @@ -66,6 +68,16 @@ defmodule Trinity.MCP.Server.Plug do def call(conn, _opts), do: conn + @doc """ + The transport's `:authorize` hook: the second look at what `call/2` decided. `call/2` + authorizes the request and leaves the principal in this process before the transport runs, so + a request that reaches the transport without one never passed the profile's check. + """ + @spec authorized(Plug.Conn.t()) :: :ok | {:error, :unauthorized} + def authorized(_conn) do + if Process.get(@principal_key), do: :ok, else: {:error, :unauthorized} + end + @doc "The principal the plug left for the wrapper, in this process." @spec principal() :: Trinity.MCP.Auth.Principal.t() | nil def principal, do: Process.get(@principal_key) diff --git a/slices/062-mcp-server-auth/NOTES.md b/slices/062-mcp-server-auth/NOTES.md index 51b3f03..84531c6 100644 --- a/slices/062-mcp-server-auth/NOTES.md +++ b/slices/062-mcp-server-auth/NOTES.md @@ -241,3 +241,48 @@ name, so the older could stay the signer (`Keys.rotate!/1` now makes the new key - The `TrustedHeaders` profile, only if a deployment asks (deviation 2). - The `[::1]` origin in `Server.Plug.loopback_origins/0` is a browser `Origin` header (bracketed there, rightly); F2 is about `URI.host`, not that list. + +## Correction, 2026-09-22, after the tag: the release build was broken + +**F5, found by the `package` workflow at the `slice/062` tag** (run 35786247006, the first run of +that workflow over this slice: its push trigger is tags only). The linux and windows legs failed to +compile: + +``` +== Compilation error in file lib/trinity_web/endpoint.ex == +** (ArgumentError) cannot escape #Function<0.132933281/1 in Trinity.MCP.Server.Plug.init/1>. + The supported values are: lists, tuples, maps, atoms, numbers, bitstrings, PIDs and remote + functions in the format &Mod.fun/arity +``` + +`Plug.Builder` calls a plug's `init/1` **at compile time** when `MIX_ENV=prod` and escapes what it +returns into the compiled endpoint. Slice 062 replaced 061's `authorize: &Auth.Local.authorize/1` +(a remote capture, which escapes) with an anonymous function closing over the process-dictionary +key (which cannot be escaped). `mix gate` runs in `MIX_ENV=test`, where `init/1` is called per +request, so the local gate and all three CI legs were green over a tree whose release could not +compile. Reproduced on `main` at `9859fbe` in one command, before any fix: + +``` +$ MIX_ENV=prod mix compile +== Compilation error in file lib/trinity_web/endpoint.ex == +** (ArgumentError) cannot escape #Function<0.132933281/1 in Trinity.MCP.Server.Plug.init/1>. … +exit 1 +``` + +Fixed in `fix(s062)` on `fix/s062-prod-compile`: the hook is `&__MODULE__.authorized/1`, a public +function with the same body, as 061's was a remote capture. The regression test is +`test/trinity/mcp/server_test.exs`, "every option the plug's init returns can be escaped, as a +compile-time init must be": it runs `Macro.escape/1` over `Plug.init([])`, which is the compile-time +step that failed. Demonstrated red against the closure first, with the same message CI printed. + +The slice is not rewritten and the tag is not moved: this is a fix commit referencing the original, +as CLAUDE.md section 4 and docs/04 require. + +**Follow-up (new, and the reason this was missed): the gate never compiles `:prod`.** Nothing in +`mix gate` or in the push-triggered CI builds a release, and `package.yml` only triggers on tags, +so a compile-time defect in a plug's options, a release-only config error, or anything else that +differs between `:test` and `:prod` is invisible until a tag exists, which is after approval. Two +candidate fixes, for the owner: add a `MIX_ENV=prod mix compile` step to the gate (about a minute +per run, catches this whole class), or give `package.yml` a `branches:` push trigger beside its +tags one (slower, but proves the bundle too). Owner's call; not done here, since the gate is every +slice's contract and this slice does not own it. diff --git a/slices/062-mcp-server-auth/PROOF.md b/slices/062-mcp-server-auth/PROOF.md index f0e9105..e8c8b96 100644 --- a/slices/062-mcp-server-auth/PROOF.md +++ b/slices/062-mcp-server-auth/PROOF.md @@ -337,3 +337,18 @@ One of them is scoped `s061`, correctly: it amends 061's own test for the `401` **Not done, and deliberately.** No pull request is open, nothing is merged, and no tag exists: the slice's status is `done`, which is the agent's half, and `approved` is yours (G4). The `slice/062` tag and the three-OS `package` run that the tag triggers both come after that. + +## Correction, 2026-09-22, after the tag: the release build, and what this proof did not cover + +Appended, not rewritten. Everything above stands as measured, and it did not measure one thing: the +`package` workflow, which this file's "Gate" section correctly said would run only at the +`slice/062` tag. It ran (35786247006) and its linux and windows legs failed to compile the release, +because `Plug.Builder` escapes a plug's `init/1` result at compile time under `MIX_ENV=prod` and +this slice had put an anonymous function in those options. NOTES.md records it as **F5** with the +reproduction on `main` at `9859fbe`, the fix (`authorize: &__MODULE__.authorized/1`) and the +regression test, which was demonstrated red against the closure before the fix. + +What this says about the proof above: the AC evidence is unaffected (every criterion was measured +through the running server in `:test`, and the fix changes no behaviour, only how the same function +is passed). What it says about the gate is in NOTES.md as a follow-up: `mix gate` never compiles +`:prod`, so this class of defect cannot be caught before a tag exists under the current CI. diff --git a/test/trinity/mcp/server_test.exs b/test/trinity/mcp/server_test.exs index 09d283b..c76a6de 100644 --- a/test/trinity/mcp/server_test.exs +++ b/test/trinity/mcp/server_test.exs @@ -313,4 +313,19 @@ defmodule Trinity.MCP.ServerTest do assert %{"result" => %{"supportedVersions" => ["2026-07-28"], "resultType" => "complete"}} = Jason.decode!(answer) end + + # fix(s062). `Plug.Builder` calls a plug's `init/1` at compile time in `:prod` and escapes what + # it returns into the compiled endpoint, so an option that cannot be escaped breaks the release + # build and nothing else: the gate runs in `:test`, where `init/1` is called per request, and + # was green over it for the whole of slice 062. This is that compile-time step, in a test. + test "every option the plug's init returns can be escaped, as a compile-time init must be" do + opts = Trinity.MCP.Server.Plug.init([]) + assert is_list(opts) or is_map(opts) + assert Macro.escape(opts) + + # The authorize hook is the option that broke it: a remote capture escapes, a closure does not. + assert_raise ArgumentError, ~r/cannot escape/, fn -> + Macro.escape(authorize: fn _conn -> :ok end) + end + end end