Skip to content

fix(sdk): 0.3.4 must-fixes — encode module loading + raw response bodies (+5 hardening commits) - #18

Merged
rob-archastro merged 12 commits into
mainfrom
fix/sdk-0.3.4-encode-and-raw
Aug 16, 2026
Merged

fix(sdk): 0.3.4 must-fixes — encode module loading + raw response bodies (+5 hardening commits)#18
rob-archastro merged 12 commits into
mainfrom
fix/sdk-0.3.4-encode-and-raw

Conversation

@rob-archastro

@rob-archastro rob-archastro commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review on ArchCode

Problem and author intent

Remote-eval slice 4's strict typed proof and the follow-up adversarial sweep found two must-fix bugs in archastro 0.3.3, plus five hardening findings. A post-open adversarial audit of this branch then found four more defects — one of them introduced by the hardening in this PR — which are fixed in commits 9–12. Evidence and repro scripts live in the slice-4 consolidated ask (wt7 scratchpad, sdk-0.3.4-consolidated-ask.md).

@calvin: commits 3–7 are the hardening set; ride-0.3.4-or-fast-follow is your call. Note commits 9 and 12 fix defects in that hardening, so peeling 3–7 means peeling those too.

Must-fixes

1 — Codec.encode module-loading fallback (lib/archastro/codec.ex). encode(%module{}) gated the struct path on function_exported?(module, :to_map, 1), which never loads the module. An unloaded payload-struct module (normal for compile-time struct literals under mix test / iex — exactly where consumers' merge gates run) silently fell back to Map.from_struct, which also returns without recursing: :__archastro_unset__ sentinels hit the wire as literal strings (crashed the platform chat channel live), renamed fields kept struct keys (alias_ vs "alias"), keys stayed atoms, nested structs/DateTimes went un-encoded. Re-fires at every nesting level, so the fix is in the encode clause itself: Code.ensure_loaded?(module) and function_exported?(...), mirroring the in-file precedent on the decode-side {:ref, module} matcher. Unreachable in OTP embedded-mode releases; fully reachable everywhere else.

2 — raw responses arrive Req-decoded (lib/archastro/http.ex). Raw ops return the Req.Response, but Req's decode_body step ran first, so a raw blob served with Content-Type: application/json arrived as a decoded map. Raw requests now pass decode_body: false. The generator half is archastro-openapi PR 79.

Two production ops are affected, and both flip signature on regen (enumerated over the spec — these are its only format: binary occurrences):

  • GET /api/v1/trajectories/{trajectory}/contents
  • GET /api/v1/private_service_definitions/{app_id}/{private_service_id}

Both are emitted today as typed decode: :string ops that raise on every successful non-empty response, so no working consumer exists for either. On regen their signatures change in every SDK: Elixir {:ok, String.t()}{:ok, Req.Response.t()}, TypeScript Promise<string>Promise<{content, mimeType}>, and the analogous raw shapes in Python/Go/Swift/Rust. Nothing functioning breaks; the ops become usable for the first time.

Hardening (commits 3–7, Calvin's ruling)

  1. harden(socket): survive undecodable channel payloads — naked Codec.decode in the Socket GenServer meant one malformed broadcast, reply, or join response — or a payload-less {:reply, :error, socket} (Slipstream collapses it to the atom :error) — killed the whole socket: every channel, subscription, pending push, and the linked owner. Broadcasts drop with [:archastro, :channel, :decode_failure] telemetry; replies resolve as {:error, %Error{code: "decode_failure"}}; join failures fail the joiners and leave the topic.
  2. harden(codec): reject NaiveDateTime and Time inputs with a clear error — previously an opaque Jason tuple crash deep inside Req.
  3. harden(client): disable redirect-following on default requests — a 302 silently rewrote a token-refresh POST into a body-less GET.
  4. harden(query): drop nil params instead of sending the string null.
  5. harden(socket): sweep pushes and joins that are never acknowledged — pending entries had no expiry; a 60s sweep fails anything older than a full interval with code: "no_reply" (60–120s effective grace vs the 5s default call timeout). Most new behavior of the set — flag if you want it fast-followed or made configurable.

Audit round (commits 9–12)

An adversarial audit subagent over this branch found four defects, all fixed red-first:

  1. fix(channel): encode outbound payloads in the caller, not the socket — the highest-severity finding, and self-inflicted: Codec.encode ran inside the Socket GenServer for pushes and joins, so commit 4's new raise killed the shared socket and every channel on it. Ecto's timestamps() defaults to :naive_datetime, making that an easy value to hand it. Encoding moved to Channel.push/join so a bad payload fails only its own caller. Verified against the real channel harness (15/15).
  2. fix(http): keep structured API errors on raw requestsdecode_body: false also stripped the error envelope off failure responses (Error.from_response discards a non-map body), degrading a 404's {"error": {message, code}} to a generic message with no code. Binary bodies are now Jason-decoded before building the error, the same way the SSE path already does; non-JSON bodies are left alone.
  3. fix(query): keep dropping unset sentinels, not just nils — commit 6 removed the sentinel clause on the premise that Codec.encode always rewrites sentinels to nil. That holds only for structs exporting to_map/1; the Map.from_struct fallback leaves them intact, so a caller-built params struct crashed in scalar/1 where it used to work.
  4. fix(socket): leave a topic whose join response has no waiter — a join acked after its waiters were gone (swept, or callers gave up) left Slipstream's status at :joined with no channel in assigns; since join/3 no-ops unless the status is nil or :closed, every later join of that topic queued a pending entry that could never be satisfied. The topic is now left, returning it to a closed state; established topics rejoined after reconnect are kept.

Commit 8 (ci:) is unrelated to the SDK: PR runs built the contract harness from a long-dead openapi branch predating the harness's nullable normalization, so all 15 channel-contract tests failed at harness boot on any PR while main stayed green. The PR checkout ref is now main.

Scope

SDK-only, hand-written files (codec.ex, http.ex, socket.ex, channel.ex, client.ex, token_server/default.ex, query.ex) plus the CI workflow ref. No generated files touched — regen picks up both raw ops after the generator release. No spec or platform changes.

Risk assessment

Low-to-medium. Must-fix 1 makes the loaded and unloaded paths identical (the loaded path was always correct); previously-corrupt wire bodies become correct ones. Must-fix 2 changes raw-op bodies to always be the raw binary; the only JSON-content raw ops are the two that crash today. The hardening commits change failure modes (crash → typed error / drop-with-telemetry) rather than success paths; commit 7's sweep timer is the only genuinely new runtime behavior. Commit 9 moves work from the socket process to the caller process — verified against the real harness, which exercises join/push/broadcast over a real websocket.

User impact

Consumers on 0.3.4 stop hitting sentinel-corrupted request bodies under mix test / iex, can fetch trajectory contents and private service definitions, keep structured errors on raw endpoints, and (if the hardening rides) keep their sockets alive through malformed payloads and bad outbound values.

Testing

All red-first. Full suite 1345 passing, channel-contract suite 15/15 against the real harness subprocess, dialyzer clean, mix format --check-formatted clean.

  • Canonical proof, must-fix 1: test/codec_unloaded_modules_test.exs, "generated input/params structs encode identically when their modules are not loaded" — enumerates all 211 generated Input/Params modules from the app spec, encodes each with all-unset optionals, purges the module (:code.purge + :code.delete, asserting to_map/1 is no longer exported — the exact production precondition), re-encodes, and asserts byte-identical output plus no sentinel and no atom key anywhere in the tree. Watched fail on 0.3.3 with the exact live corruption.
  • Canonical proof, must-fix 2: test/http_test.exs, "raw requests deliver an application/json body as the raw binary" — drives the real ArchAstro.SDK.HTTP.request/4 stack (TokenServer → auth headers → full Req pipeline) against a Plug-served application/json body and asserts the raw string arrives; sibling tests pin structured errors on raw failures, non-JSON failure bodies, and that typed requests still decode.
  • Canonical proof, commit 9: test/socket_test.exs, "encode failures raise in the caller process, never inside the socket" — a stand-in socket process reports anything it receives; the test asserts the raise happens in the caller and the socket is never messaged. Backed by the real-harness channel suite.
  • Honest boundary: the HTTP tests use Req's plug adapter, so no network socket is crossed there; the channel-contract suite does cross a real websocket to a harness subprocess. The full network-crossing end-to-end for both must-fixes is remote-eval slice 4's strict typed proof against a real platform, which reruns at the 0.3.4 repin and is the gate this PR exists to clear.
  • Per-commit red-first tests for the rest: socket decode-failure quartet + telemetry, sweep aging, late-join-ack leave with an established-rejoin control, NaiveDateTime/Time raises, redirect option pins, query nil and sentinel omission.

Follow-ups and chain

  • Chain after merge: regenerate-sdk.yml (needs @archastro/sdk-generator 0.11.2 from openapi PR 79 first; regen PR gets no CI — main CI after merge is the gate) → release.yml patch → v0.3.4 → merge the tracking PR → hand version + Hex checksum to the slice-4 orchestrator.
  • If any of commits 3–7 are ruled fast-follow, commits 9 and 12 travel with them.

🤖 Generated with Claude Code

rob-archastro and others added 8 commits August 16, 2026 10:50
Codec.encode/1 gated the struct path on function_exported?(module,
:to_map, 1), which never loads the module. When a payload struct's
module was not yet loaded (normal for compile-time struct literals
under interactive-mode code loading, e.g. mix test / iex), encode fell
back to Map.from_struct and returned without recursing: the
:__archastro_unset__ sentinel reached the wire as a literal string,
renamed fields kept their struct keys (alias_ instead of "alias"),
keys stayed atoms, and nested structs and DateTimes went un-encoded.
The fallback re-fires independently at every nesting level, so the fix
belongs in the encode clause itself, mirroring the existing
Code.ensure_loaded?/1 guard on the decode-side {:ref, module} matcher.

Regression test encodes every generated Input/Params struct with
all-unset optionals under purged modules and asserts wire cleanliness,
plus a renamed-field wire-name check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Raw operations (raw: true) return the Req.Response so callers get the
body exactly as served, but Req's decode_body step ran first: a raw
blob served with Content-Type: application/json arrived as a decoded
map instead of the raw binary. Combined with the generator emitting
decode: :string for GET /trajectories/{trajectory}/contents, that op
raised on every successful non-empty fetch. Pass decode_body: false
for raw requests so the body arrives untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A naked Codec.decode in the Socket GenServer meant one malformed
broadcast, reply, or join response raised inside the socket process,
killing every channel, subscription, pending push, and the linked
owner. A payload-less {:reply, :error, socket} from the server
(collapsed to the bare atom :error by Slipstream) hit the same decode
path and had the same blast radius.

- broadcasts: drop the message for that subscriber and emit
  [:archastro, :channel, :decode_failure] telemetry
- replies: resolve the push with {:error, %Error{code: "decode_failure"}}
- join responses: fail the waiting joiners, leave the topic, keep the
  socket alive
- bare :error replies: resolve as a channel error instead of decoding

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A NaiveDateTime or Time in a request payload fell through encode's
struct fallback into Map.from_struct, surfacing later as an opaque
Jason tuple crash deep inside Req. Raise an ArgumentError at the
encode boundary that names the value and the conversion the caller
needs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The internal default Req.new(retry: false) in Client.build and
TokenServer.Default left Req's redirect-following on. A 302 from the
platform silently rewrote a token-refresh POST into a body-less GET
against the Location target. A 3xx is never a valid platform response;
surface it as the error it is.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A nil scalar in a plain-map query serialized as the literal string
"null" (scalar/1 had a nil clause), and the unset-sentinel filter was
dead code because Codec.encode rewrites sentinels to nil before the
filter runs. Treat nil — top-level or inside a repeated param — as
"not provided" and omit it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pending_pushes and pending_joins entries had no expiry: a push for a
fire-and-forget event (or a join the server never acks) parked its
entry forever on a long-lived socket. Entries now carry an insertion
timestamp and a periodic sweep fails anything older than a full
interval (60s, so an effective 60-120s grace — far above the 5s
default Channel call timeout) with {:error, %Error{code: "no_reply"}}.
Replies to callers that already timed out are no-ops.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pull-request runs pinned the harness checkout to a long-dead SDK
development branch that predates the harness's nullable-schema
normalization, so it exits at boot against the 0.3.3 spec and all 15
channel-contract tests fail on any PR (main pushes already use main
and are green). Verified locally: harness built from main passes all
15 against this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rob-archastro

Copy link
Copy Markdown
Contributor Author

Review — round 1 (head d97a362)

Verdict on the diff: no findings — all seven commits verified against the consolidated slice-4 ask. But CI is red for a reason outside this PR (root-caused below); one workflow-fix commit is required before merge.

Verified:

  • Must-fix 1: Code.ensure_loaded? added in the exact form of the in-file precedent (codec.ex:179). The regression test is the strong version of what was asked: enumerates all generated Input/Params modules from the app module list (>150 asserted, so the enumeration can't silently go empty), asserts loaded/unloaded encode equality plus wire-cleanliness (string keys, no sentinel atom or string) per module, and covers the renamed-field case (alias_"alias") under purge. async: false with rationale — correct, purging is VM-global.
  • Must-fix 2 companion: decode_body: false for raw: true requests; HTTP test serves a real application/json response through a plug and proves the body arrives as the raw binary, with a typed-decode control.
  • Hardening: socket safe_decode at all three decode sites (join failure replies error to every waiting from AND leaves the half-open topic; broadcast failure drops with telemetry; the Slipstream bare-:error collapse gets its own clause); pending sweep re-arms first, 1–2 interval grace, replies typed no_reply errors (replying to a dead from is a safe no-op); NaiveDateTime/Time raise with a conversion hint instead of guessing a timezone; both default Reqs get redirect: false; query drops nils (including inside lists) and scalar(nil) is gone.

CI red — root cause is a pre-existing workflow landmine, not this diff. All channel-contract failures are the harness subprocess exiting 1 at boot. ci.yml:35 pins the harness checkout to features/calvin-archastro-06-08-2026-elixir-sdk on pull_request events (main pushes use main). That branch's head is 2026-08-07 (af16aa01) — it predates archastro-openapi PR 75's ajv normalizeNullable fix, so the harness built from it dies on the spec's type-less nullables ("nullable" cannot be used without "type"), which the vendored spec has carried since the 0.3.3 regen. Reproduced locally both ways: a pre-75 harness crashes on this PR's spec with exactly that error; a harness built from openapi main boots cleanly against the same spec and serves its endpoints. This PR is simply the first human PR since the regen — automation PRs get no CI, so the mine sat unexposed.

Required change (new commit, no amend): ci.yml:35 — drop the event conditional, use ref: main unconditionally. The branch pin was a bootstrap-era artifact; main is where the harness actually lives now.

rob-archastro and others added 4 commits August 16, 2026 11:14
Codec.encode ran inside the Socket GenServer for pushes and joins, so
any unencodable payload raised there and killed the shared socket with
every other channel, subscription, and in-flight push on it. That was
survivable while encode was total, but this branch makes it raise on
NaiveDateTime/Time — an easy value to hand it, since Ecto timestamps()
defaults to :naive_datetime. Encode in Channel.push/join instead, so a
bad payload fails only its own caller.

Found by an adversarial audit subagent over this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Suppressing Req's body decoding for raw requests also stripped the
error envelope off failure responses: Error.from_response discards a
non-map body, so a 404 carrying {"error": {"message", "code"}}
degraded to the generic "ArchAstro API returned HTTP 404" with no
code or details. Decode a binary body before building the error, the
same way the SSE path already does; a non-JSON body is left alone.

Found by an adversarial audit subagent over this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit removed the sentinel clause on the premise that
Codec.encode always rewrites :__archastro_unset__ to nil first. That
holds only for structs exporting to_map/1; the Map.from_struct
fallback leaves sentinels intact, so a caller-built params struct
following the SDK's own sentinel convention crashed in scalar/1 where
it used to produce the correct URL. Drop both nil and the sentinel,
top-level and inside repeated params.

Found by an adversarial audit subagent over this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A join acked after its waiters are gone (swept, or callers that gave
up) left Slipstream's join status at :joined while no channel existed
in assigns. Slipstream's join/3 no-ops unless the status is nil or
:closed, so every later join of that topic queued a pending entry that
could never be satisfied — the topic stayed unjoinable until the socket
reconnected. Leave the topic instead, returning it to a closed state.
Established topics rejoined after a reconnect keep their channel and
are not left.

Found by an adversarial audit subagent over this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rob-archastro

Copy link
Copy Markdown
Contributor Author

Round 2 (head 3c7702f) — CI green, four self-found fixes verified

Verdict: meets bar. No findings. The workflow fix worked: both matrix legs pass, which confirms the root cause (harness built from the Aug-7 branch on PRs). Merge-ready from my side.

The four new fixes beyond the CI change are all real, and two of them close holes the round-1 hardening itself opened — good catches:

  • ee482c4 encode in the caller: this one matters most. Adding the NaiveDateTime/Time raises made Codec.encode inside the socket process a new crash vector — an unencodable payload from one caller would have killed the shared socket and every channel on it. Moving encoding into Channel.join/push makes it fail its own caller. Verified no Codec.encode remains in socket.ex, both queue_join clauses store the caller-encoded payload with no double-encode, and rejoin_established replays Slipstream's stored (already-encoded) params. The test asserts the raise happens in the caller and that nothing reaches the socket process.
  • 2edc8df structured errors on raw requests: decode_body: false would have stripped the error envelope off failure responses too. Fix decodes the body only on the error path; Error.from_response already guards non-map bodies, and the non-JSON 502 test pins the fallback.
  • bd6a693 query sentinels: corrects the round-1 comment's premise. Verified at source — encode(%module{})'s Map.from_struct fallback returns without recursing, so sentinels genuinely do survive to Query.encode. The test uses a struct with no to_map/1, which is the exact shape that reaches it.
  • 3c7702f unowned join responses: correct on both sides — an established channel being rejoined after reconnect is kept, a join whose waiters are gone is left rather than stranded joined-but-unowned. Both cases pinned.

Chain after merge: main CI → release.yml bump=patch → v0.3.4 → merge the release tracking PR → hand version + Hex checksum back for tag verification.

@rob-archastro
rob-archastro merged commit a36a529 into main Aug 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant