Skip to content

Generated SDK types export (Pydantic + Zod) from the proto; money as exact decimal - #10

Merged
legendko merged 43 commits into
mainfrom
feature/sdk-libraries
Jul 6, 2026
Merged

Generated SDK types export (Pydantic + Zod) from the proto; money as exact decimal#10
legendko merged 43 commits into
mainfrom
feature/sdk-libraries

Conversation

@KonstantinMirin

@KonstantinMirin KonstantinMirin commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Generated SDK types export (Pydantic + Zod) from the proto; money as exact decimal

Replace the protobuf-native TS/Python SDKs with a types export generated from the proto via JSON Schema — Pydantic v2 for Python, Zod for TypeScript — and make money + per-field validation generation-correct so they survive into both clients. Go keeps native protobuf (it is the server/runtime).

Why this exists / motivation

The contract is the proto, but our two real consumers can't speak protobuf natively. The MCP shim is FastMCP/FastAPI — a tool argument or a route body has to be a Pydantic model; a _pb2 object can't be either. The edge worker is Hono on Workers/Fastly — it needs Zod for request validation and a small bundle, and a generated descriptor runtime is neither. So both of them were hand-mirroring the proto's message shapes in their own idiom. That hand-mirroring is exactly the drift the contract was supposed to eliminate: a field renamed in the proto stays wrong in two places until someone notices at runtime.

The fix is to stop shipping a protobuf-native SDK that neither consumer can use, and instead generate the message types as the idiomatic type of each ecosystem — a Pydantic model, a Zod schema — from the one source. "Types export," not "full SDK": it is the message shapes plus per-field validation (presence, enum membership, string patterns, numeric ranges, item counts). Cross-field rules that only the server can adjudicate stay where they are — server-authoritative Go protovalidate CEL — and a cross-language parity harness proves the generated clients enforce every field-level rule exactly as the Go server does, so "generated from one source" is a checked property, not a hope.

Stacking / dependency note

This repo's PRs stack; review and merge bottom-up. #10 is the base of the SDK stack — it lands the generated types export and the money/validation proto corrections the SDK libraries build on:

#10  feature/sdk-libraries  → feature/protocol-unification   (THIS PR — types export + money/decimal + validation-as-standard-constraints)
 └─ #12  feature/ramp-sdk          (RAMP SDK: Go L1 protocol helpers)
     └─ #13  feature/stateless-offer-redemption  (full signed offer reflection)
         └─ #14  feature/ramp-102-relay-proto      (relay proto + well-known endpoint resolution)

#10 targets feature/protocol-unification (PR #8), not main; that base must merge first. Each downstream PR diffs against the one above it, so the money-as-decimal and closed-enum decisions here are load-bearing for all of #12#14.

What's in it — how the types are produced and consumed

Pipeline — scripts/gen-sdk-types.sh:

proto ──buf/bufbuild-protoschema──► JSON Schema per message (protovalidate-aware)
      ──merge_schema.py───────────► ONE $defs doc: clean message names, enums
                                     named & deduped from gen/descriptor.binpb
      ──datamodel-code-generator──► gen/python/wire/models.py  (Pydantic v2)
      ──json-schema-to-zod────────► gen/ts/wire/schemas.ts     (Zod)

Properties that make the output usable rather than a literal schema transliteration:

  • One base seam. Every Pydantic model extends the hand-written wire.base.WireModel (--base-class) — the single place to set model-wide behavior. It is extra="ignore" (forward-compatible: a field from a newer protocol version is dropped, not rejected; a consumer that wants strictness sets extra="forbid" in its own subclass) and overrides model_dump/model_dump_json to default exclude_none=True so parse→dump round-trips match proto-JSON's omit-unset behavior.
  • Authoritative names from the descriptor. Enums are hoisted into shared $defs and named from gen/descriptor.binpb (DenialReason, not Reason1); one model per entity, with the full nested hierarchy hydrating as typed models rather than dict[str, Any].
  • Money is an exact decimal, never a float. Python Decimal; TS a validated decimal string. (See below.)
  • Clean integers. int / coerced number, not int | strprotojson handles the int64 wire-string on the Go side, so the client type stays clean.

Consumers: the MCP shim imports the Pydantic models as FastMCP tool / FastAPI route types; the edge worker imports the Zod schemas to validate inbound bodies. Both now derive from the proto by regeneration, not by hand.

Money is exact decimal on the wire (the decision)

Money fields — Pricing.rate / Pricing.unit_cost, Cost.amount / Cost.unit_cost, TransactionItem.max_unit_cost — change from double to a decimal string with a decimal string.pattern (^([0-9]+([.][0-9]+)?)?$). Binary double cannot represent most decimal money values exactly, so it drifts and breaks settlement sums; a decimal string is exact and supports arbitrary sub-cent precision (e.g. "0.0001234" for per-token pricing). This is the value carried on the wire — a string, never a float — and the generated clients reflect it: Pydantic parses to Decimal('0.0001234') and re-emits as a string; Zod validates the decimal-string pattern. Go uses a decimal library at the app layer; there is no float anywhere in the money path. Acceptable as a pre-v1 breaking proto change.

Validation moved to standard constraints so it survives codegen

18 of 25 field-level rules moved from custom CEL to standard protovalidate constraints — 11 enum discriminators to enum.not_in:[0], 7 formats to string.pattern — because standard constraints flow through bufbuild/protoschema into the JSON Schema and therefore into the generated Pydantic/Zod, whereas custom CEL does not. The 7 genuine cross-field rules (a Restriction's permitted∩prohibited disjointness, SHARE_ALIKE ⇒ scope_license, etc.) stay CEL and stay server-authoritative.

The parity harness surfaced two places the generated clients silently under-enforced; both are fixed once at the JSON-Schema bridge (scripts/sdk-types/merge_schema.py), not patched per language:

  • Closed enums. protoschema emits each enum field as an open enum-name | integer union, which lets a raw int (0, 9999) slip past enum.not_in / defined_only. Collapsed to a closed, name-only enum — intentionally stricter than proto's open-enum forward-compat for the not_in:[0]-only discriminators.
  • Required presence. A field whose proto zero value is invalid (not_in:[0], min_len≥1, non-empty pattern, gte≥1, explicit required) is now marked required with its zero default dropped, so the client rejects omission the way the server does. The required set is derived authoritatively from protovalidate via conformance/requiredgen (Go) — the Python bridge consumes that list, it does not re-implement the rule semantics.

Concrete changes (reading the diff)

  • scripts/gen-sdk-types.sh — the regeneration entry point (4 stages above); provisions a throwaway venv + node_modules under .sdk-types-work/, pins datamodel-code-generator==0.64.0 and json-schema-to-zod@2.8.1 so the byte-compared output is deterministic in CI.
  • scripts/sdk-types/buf.jsonschema.yaml (protoschema template), merge_schema.py (descriptor-accurate names, enum hoisting, closed-enum + required-presence fixes), gen_zod.mjs (Zod emission), roundtrip_py.py / roundtrip_ts.ts (canonical interop drivers).
  • gen/python/wire/base.py — hand-written WireModel seam (the only non-generated file under wire/). gen/python/wire/models.py + gen/ts/wire/schemas.ts — the generated Pydantic / Zod types; money is Decimal / decimal-string, enums are closed, invalid-zero fields are required.
  • proto/ramp/v1/ramp.proto — money double → string + decimal pattern; discriminators → enum.not_in:[0]; formats → string.pattern. proto/ramp/v1/vocab.proto + cmd/protoc-gen-rampvocab/ — the vocab axis→package mapping moves into the proto (drops the hand-maintained Go plugin maps). Regenerated gen/go/**, gen/ts/**, gen/descriptor.binpb.
  • conformance/ — the parity harness. corpusgen builds a valid baseline per constrained message, mutates one field per constraint to a boundary-violating value, and emits corpus/cases.json labeled with Go protovalidate's verdict (the rule's only executable form is the oracle). requiredgen emits the authoritative required-field set. gen/python/tests/test_parity.py + gen/ts/tests/parity.test.ts assert Pydantic and Zod reach the same verdict on every case; vocab_parity_test.go asserts the Go/Python/TS vocab constant sets are identical per axis; canonical_test.go + scripts/check-canonical.sh round-trip every valid instance through Go protojson and require proto.Equal (round-trip against Go, not self-round-trip, so it tolerates the benign differences proto-JSON permits while catching anything Go can't ingest).
  • gen/{python,ts}/vocab/* — emitted vocab constant sets (function/geography/pricing/quota/user-type tokens) for both languages.
  • .github/workflows/sdk-types-ci.yml — regenerates the export, fails on drift (regenerate-and-diff, same contract as the existing gen/ gate), runs both parity suites; scripts/ci-local.sh is the single local mirror of the gating sequence.
  • website/src/content/docs/** — money examples updated to decimal strings; a how-money-flows page added; walkthroughs and the generated proto reference page reconciled with the new field types.

Out of scope / follow-ups

  • No higher SDK layers here. This is the types export only. The Go L1 protocol helpers (signing, offer construction) land in RAMP SDK: Go L1 protocol helpers (sdk/go/ramphelpers) on L0 #12; the Python/TS equivalents and the stateless-offer / relay protocol work in Stateless offer redemption: reflect the full signed Offer in execute #13Relay proto: well-known endpoint resolver + RAMP* prefix purge #14.
  • Cross-field validation stays server-side. The 7 CEL rules are deliberately not pushed into the clients; the clients enforce field-level rules and defer composite checks to the Exchange. This is by design, not a gap.
  • Closed enums are stricter than the wire. The clients reject unknown enum names for not_in:[0]-only discriminators, trading a sliver of forward-compat for catching raw-int bypass. Message-level extra="ignore" preserves forward-compat for new fields; only unknown enum values on closed discriminators are rejected.
  • Generator pins. The byte-compared output binds us to specific datamodel-code-generator / json-schema-to-zod / buf versions; bumping any of them is a deliberate regenerate-and-review step, not a transparent dependency update.

The repo is the SDK repo; the vocabulary/enum constants exist so SDK CONSUMERS use
typed constants instead of magic strings. Only Go had them, and Python was missing
entirely. This closes both gaps:

- Python base SDK: add protocolbuffers/python + pyi (type stubs) + connectrpc/python
  (Connect service stubs) to buf.gen.yaml → gen/python, with a pyproject.toml
  (messages are the only hard dep; connect + protovalidate are extras).
- Multi-language vocab: extend the single protoc-gen-rampvocab plugin to emit Go, TS,
  and Python from ONE pass over the (ramp.v1.vocab)/(ramp.v1.vocab_enum) options
  (out: ../gen → go/vocab, ts/vocab, python/vocab). The descriptor-reading core is
  unchanged; only per-language identifier casing + rendering differ. Because all
  three come from the same tokens in the same pass, they cannot drift from each other
  — no cross-language parity check is needed, and the existing regenerate-and-diff
  gate guards all of it for free.

Go vocab output is byte-identical (refactor preserved it). TS exposes vocab via a
wildcard subpath export so a new axis is published automatically. Python uses
UPPER_SNAKE constants + an ALL tuple + is_registered(); verified importable.
Add a Python SDK section (gen/python, pip install) and note that all three SDKs now
carry registered vocabulary constants emitted from one source, so consumers use typed
constants + a membership check instead of magic strings.
…the Go maps

Adding a vocab axis previously required editing two hardcoded maps in
protoc-gen-rampvocab (the one recurring manual touch). Move that mapping into the
proto, next to the tokens, so the generator is fully data-driven and adding an axis
touches only the .proto:

- New options (ramp/v1/vocab.proto): (ramp.v1.vocab_package) on FieldOptions (50003)
  and (ramp.v1.vocab_enum_package) on EnumValueOptions (50004), carrying the
  generated package/module name for the axis.
- Annotate the five axes in ramp.proto (Pricing.unit→pricingunits,
  Quota.metric→quotametrics, RestrictionKind values→functiontokens/geographytokens/
  usertypes).
- Plugin reads the package from the option (readVocabString) and errors loudly if a
  vocab-bearing descriptor lacks its package option; deleted fieldAxisPackage/
  enumAxisPackage.

The vocab constant outputs (gen/{go,ts,python}/vocab) are byte-identical — the
package names match the former maps. Only the descriptor-embedding message files
change (the new options are part of the proto descriptor).
…om CEL

The 11 "*_specified" discriminator rules were message-level CEL comparing a
fully-qualified enum (this.x != ramp.v1.Enum.UNSPECIFIED). That form has two
problems the SDK work surfaced: it does NOT survive proto→JSON Schema generation
(so it can't reach generated Pydantic/Zod), and protovalidate-python (celpy)
cannot even evaluate it ("undeclared reference to 'ramp'"). The standard
(buf.validate.field).enum.not_in:[0] expresses the same rule, and — measured —
it DOES survive into JSON Schema (UNSPECIFIED is dropped from the enum) and
evaluates in every protovalidate port.

Converted all 11 (Pricing.model, LicenseTerm.semantics, Restriction.kind,
Obligation.kind/trigger, Quota.window, AuthorizedExchange.relationship,
Requester.type, ResourceIdentity.resource_mutability, WellKnownManifest.role,
DisputeRequest.reason). Conformance wantRules updated to enum.not_in; INV-1 now
passes via the field rule; INV-5's declared-CEL set auto-shrinks to the 7
cross-field + 7 format rules (the genuinely-CEL ones). Doc citation updated.
Cross-field rules stay CEL (server-authoritative).
The 7 token/digest format rules were custom CEL (this.matches(...)), which — like
the discriminators — does not survive proto→JSON Schema generation. Expressed them
as standard protovalidate constraints instead:
- scalar fields (Pricing.unit, Quota.metric, Usage.consumed_unit, License.uri_digest)
  → (buf.validate.field).string.pattern with one combined regex (the bare|namespaced
  alternation, plus optional-empty where the field allowed empty);
- repeated token lists (Restriction.permitted/prohibited, AcceptableRestriction.values)
  → repeated.items.string.{min_len,max_len,pattern}.

These standard patterns DO ride into the generated JSON Schema / Pydantic / Zod.
Conformance wantRules updated to string.pattern; INV-3 (which kept the token-format
CELs canonical) is removed as obsolete — there are no token-format CELs left. The 7
genuine cross-field rules remain CEL (server-authoritative). Custom CEL ids now: 7.
Remove the protobuf-es (gen/ts) and Python _pb2/connect (gen/python) generation and
their buf.gen.yaml plugins. They are unusable in the target ecosystems — FastMCP needs
Pydantic, edge runtimes need Zod, and both consumers were hand-mirroring the proto
rather than importing these. Go keeps native protobuf (it is the server/runtime).

Kept: the standalone vocabulary constants (gen/{go,ts,python}/vocab) — usable by any
consumer. The TS/Python *types export* (Pydantic + Zod, generated from JSON Schema) is
added next; the manifests are trimmed to the vocab-only interim until then.
…JSON Schema

The replacement for the discarded protobuf TS/Python SDKs: a reproducible pipeline
(scripts/gen-sdk-types.sh) that generates the message types + per-field validation as
the idiomatic types of each ecosystem, so consumers (FastMCP/FastAPI in Python, edge
Zod stacks in TS) stop hand-mirroring the proto.

  proto --buf/bufbuild-protoschema--> JSON Schema (protovalidate-aware)
        --merge_schema.py-----------> one $defs doc, clean message names
        --datamodel-code-generator--> gen/python/ramp/models.py   (Pydantic v2)
        --json-schema-to-zod--------> gen/ts/ramp/schemas.ts       (Zod, refs inlined)

What rides through (measured): shape, enums (UNSPECIFIED excluded where it's a
required discriminator — the enum.not_in refactor pays off), string patterns, length
and item bounds. Verified the Pydantic actually enforces (a bad uri_digest raises).
Cross-field rules do NOT generate (no tool carries them) — they stay server-side in
Go protovalidate, the correct trust boundary.

Known v1 limitations: datamodel-codegen also emits a few helper RootModel aliases;
the Zod `ext` (google.protobuf.Struct) fields are z.any() (genuinely arbitrary JSON).
The pipeline uses external tools (datamodel-code-generator, json-schema-to-zod) so it
is run on demand via the script, not wired into the buf drift gate yet.
…es, CI drift gate

Rework the types export per the adcp postprocessing study + neutral-naming constraint:

- Single base seam: every model extends `wire.base.WireModel` (hand-written, neutral
  name — no protocol coupling, so a rename never touches consumers). One place for
  SDK-wide config: forward-compatible extra="ignore" + exclude_none on dump. Injected
  via datamodel-codegen --base-class.
- Authoritative names from the proto descriptor: enums are hoisted to shared $defs and
  named from gen/descriptor.binpb matched by value set (DenialReason, ObligationTrigger,
  C2PAStatus) — no Reason1/Kind1 dupes, no prefix-guessing. *_UNSPECIFIED sentinels
  dropped (never valid on the wire). One model per entity, referenced everywhere
  (LicenseTerm.license IS a License) — full hierarchy hydrates as typed models.
- No closed RootModel-for-leaf (--collapse-root-models); the stray rootless Model
  artifact is stripped in a postprocess pass.
- Neutral namespace: gen/{python,ts}/ramp → wire (models at wire.models / @sdk/wire/schemas).
- CI: .github/workflows/sdk-types-ci.yml regenerates and fails on drift (the full,
  scripted, gated process — not memory).

Verified: all 58 models extend WireModel, 0 RootModel/numbered-dup classes, nested
hierarchy intact, per-field validation enforces (bad uri_digest raises), exclude_none
seam works. 86 Zod exports (58 messages + 28 named enums).
… enums)

protoc-gen-jsonschema models a proto double as anyOf[number, "Infinity"/"-Infinity"/
"NaN", string] — proto-JSON's permissive float encoding — which datamodel-codegen
turned into field-named Enum classes (Rate, UnitCost) and Zod into "Infinity" literals.
RAMP money/quantity values are finite, so the merge now collapses any such field to a
plain number: rate/unit_cost are float / z.number(). int64-as-string anyOf is left
intact (that string form is the canonical proto-JSON int64 encoding the server emits).

(The other noted leftover — Zod ext → z.any() — was a miscount: ext already renders as
z.record(z.string(), z.any()), the correct arbitrary-Struct type; the z.any() was the
record's value type, not a bare any.)
Every proto `double` in RAMP is money (Pricing.rate/unit_cost, Cost.amount/unit_cost,
TransactionItem.max_unit_cost). float is wrong for money (0.1+0.2 != 0.3), so the merge
now marks these fields {type:number, format:decimal} → datamodel-codegen emits
`Decimal`. Verified: model_validate_json parses the JSON number 0.05 to an exact
Decimal('0.05') (pydantic-core reads the literal, no float intermediate), and dumps to
"0.05" — a JSON string, which proto-JSON accepts for a double on parse, so it stays
wire-compatible with the Go server (which emits the number form; both are valid
proto-JSON). Zod is unchanged (z.number()) — TS has no Decimal primitive.
Replace every money `double` (Pricing.rate/unit_cost, Cost.amount/unit_cost,
TransactionItem.max_unit_cost) with a `string` carrying a decimal-format pattern.
Binary float is wrong for money (rounding drift, broken sums at settlement scale); a
decimal string is exact and imposes no granularity floor — arbitrary sub-cent pricing
("0.0001234") is representable exactly, which is the opposite of what float allows.
Done now while there are zero users.

- proto: 5 fields → string with (buf.validate.field).string.pattern
  "^([0-9]+([.][0-9]+)?)?$"; pricing.free.zero_rate CEL updated for the string form.
- types export: the merge tags money strings format:decimal → Pydantic `Decimal`
  (model_validate_json parses the wire string to an exact Decimal and dumps it back as
  a string — wire-exact, no float intermediate); Zod is z.string().regex(...) (TS has
  no Decimal; money is a validated decimal string there). Verified "0.0001234"
  round-trips exactly.
- conformance: Pricing rate literals are decimal strings; freePricing() rate "0".

BREAKING wire change (number → string for money) — acceptable pre-v1, zero consumers.
Match the wire change (money is a decimal string, not a number): wrap every
rate/amount/unit_cost/max_unit_cost JSON example value in quotes (104 occurrences
across the walkthroughs, transaction-flow, content-mutability, and the academic/legal
ext profiles). comp.proto uses none of these field names, so all are ramp.v1 money.
protoc-gen-jsonschema models every integer as anyOf[{integer}, {string ^-?[0-9]+$}]
(proto-JSON accepts both, and emits int64 as a string for JS 2^53 safety), which gave
an ugly `int | str` union. The merge now collapses to the integer branch:
- Pydantic: clean `int`/`conint` (lax parsing still coerces the wire string "1000").
- Zod: gen_zod rewrites z.number() → z.coerce.number(), so the wire string is accepted
  (all numeric fields here are integers — money is a string — so global coerce is safe).
Verified limit parses from both 1000 and "1000".

Go is untouched and was already correct: native int64 with protojson handling the
wire-string conversion. Likewise Go money is a plain `string` (protobuf-go has no
Decimal; the Go server parses it with a decimal lib — no float in money anywhere).
CI was red on drift: gen/descriptor.binpb is byte-sensitive to the buf version, and
the types export is sensitive to datamodel-code-generator / json-schema-to-zod
versions — all were unpinned, so CI's newer versions diverged from the committed
output (the descriptor drift is pre-existing; it also fails on the base branch).

Pin to the versions the committed artifacts were generated with:
- buf 1.66.1 in proto-ci.yml and sdk-types-ci.yml (buf-setup-action `version:`)
- datamodel-code-generator==0.64.0 and json-schema-to-zod@2.8.1 in gen-sdk-types.sh
(bufbuild/protoschema was already pinned @v0.6.0; proto deps are pinned via buf.lock).
… stable

The merge built $defs from glob.glob(), whose order is filesystem-dependent (macOS
vs CI Linux), so datamodel-code-generator emitted classes in a different order on CI
than locally — the committed output and CI's regen diverged even though the source was
identical. Sort the input files and the $defs keys so class order is stable across
machines. Regenerated.
…to feature/sdk-libraries

# Conflicts:
#	.github/workflows/proto-ci.yml
#	gen/descriptor.binpb
#	gen/go/ramp/v1/ramp.pb.go
#	gen/ts/ramp/v1/ramp_pb.ts
#	proto/ramp/v1/ramp.proto
@KonstantinMirin
KonstantinMirin marked this pull request as ready for review June 19, 2026 18:34
…rcement gaps

Generate a validation corpus from the proto (Go protovalidate as the oracle) and
assert the generated Pydantic and Zod clients reach the same verdict on every
field-level rule. Building it surfaced two bridge gaps where the clients silently
under-enforced; both are fixed at the JSON-Schema bridge so the clients now match
the Go server exactly at field level.

Bridge fixes (scripts/sdk-types/merge_schema.py):
- close_enum_unions: collapse the open `enum-name | integer` union protoschema
  emits down to a closed name-only enum, so enum.not_in / defined_only can no
  longer be bypassed by a raw int (0, 9999) in Pydantic/Zod.
- mark_required: mark every field whose proto zero is invalid (not_in:[0],
  min_len>=1, non-empty pattern, gte>=1, explicit required) as required and drop
  the zero default, so clients reject omission like the server. The required set
  is derived authoritatively from protovalidate via conformance/requiredgen (Go),
  so the Python bridge does not re-implement the rule semantics.

Harness:
- conformance/corpusgen emits conformance/corpus/cases.json (proto-JSON instances
  + Go verdict); corpus_test.go re-validates it against protovalidate; ci-local
  regenerates and drift-gates it.
- gen/python/tests + gen/ts/tests assert Pydantic/Zod == the corpus verdict.
- vocab_parity_test asserts the Go/Python/TS vocab constant sets are identical.
- sdk-types-ci runs both parity suites after regeneration.

Result: 0 divergences across 79 cases — Pydantic == Zod == Go at field level.
Cross-field CEL stays server-authoritative; clients use closed name-only enums
(intentionally stricter than proto open-enum forward-compat for not_in-only
discriminators).
Assert the generated clients' wire output is loss-free and ingestible by the Go
server. Each client (Pydantic/Zod) parses every valid corpus instance and
re-serializes it; the re-emission, read back through Go protojson, must decode to
the same proto message as the original (proto.Equal). This is the round-trip-
against-Go obligation, not a self-round-trip: it tolerates the benign encoding
differences proto-JSON permits (int64 as number vs the canonical string, omitted
vs explicit zero fields — both decode identically) while catching anything Go
cannot ingest (e.g. money emitted as a number into a string field, or a malformed
timestamp).

- conformance/canonical_test.go: the proto.Equal assertion (CANONICAL_PY/_TS).
- scripts/sdk-types/roundtrip_{py.py,ts.ts}: client re-serialization emitters.
- scripts/check-canonical.sh: drives both clients + runs the Go assertion;
  sdk-types-ci runs it after the parity suites, reusing the gen work dir.
- corpusgen now populates Timestamp/Duration baseline fields (fixed values, kept
  deterministic) so the round-trip actually exercises those proto-JSON encodings
  — the forms most likely to diverge across languages.

Result: both clients round-trip every valid instance loss-free through Go,
including RFC 3339 Timestamp and Duration.
Cascade latest main (incl. CoMP V1 / PR #9) and the #8 protocol-unification work
down into the SDK-types branch. ramp.proto/comp.proto auto-merged cleanly
(unification + the SDK-types money/enum/required changes coexist). Resolved the
generated-artifact conflicts by regenerating, not by hand:
- gen/descriptor.binpb: regenerated (buf 1.66.1).
- gen/ts/comp/v1/comp_pb.ts: kept deleted — this branch dropped protobuf-es TS in
  favor of the Zod types export (buf.gen.yaml emits Go + connect-go + vocab only).
- gen/go, gen/*/vocab, gen/ts/wire (Zod), gen/python/wire (Pydantic), and the
  validation corpus all regenerated from the merged proto.
Base automatically changed from feature/protocol-unification to main July 1, 2026 13:25
# Conflicts:
#	gen/descriptor.binpb
#	gen/go/ramp/v1/ramp.pb.go
#	gen/ts/comp/v1/comp_pb.ts
#	gen/ts/ramp/v1/ramp_connect.ts
#	gen/ts/ramp/v1/ramp_pb.ts
#	gen/ts/ramp/v1/vocab_pb.ts
#	proto/buf.gen.yaml
#	website/src/content/docs/components/broker/selection-engine.mdx

@legendko legendko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall verdict

The engineering core is strong and mostly correct; the delivered guarantees are over-claimed. The proto refactor is faithful (all 7 CEL→pattern moves and all 11 enum→not_in:[0] moves are semantically equivalent; counts match the description exactly: 11 discriminators, 5 money fields, 7 formats, 7 cross-field CEL kept; dispute-chain and extension model intact; vocab single-source is a real win, machine-enforced, identical across 3 languages).

But three of the feature's flagship properties are not actually achieved in the committed output, and the parity harness — the thing meant to prove them — masks all three:

  1. Python money silently does not enforce the wire pattern (accepts -5, NaN, Infinity, 1E3).
  2. Forward-compat (extra="ignore") is defeated in both clients (Python extra='forbid', Zod .strict()).
  3. The determinism the drift-gate depends on is not pinned (black et al. float).

None is a live-production or security Critical (the Go server re-validates authoritatively, and the forbid/strict divergence fails closed), so the top tier is HIGH. There are no Critical issues. The documentation is materially out of conformance (README describes a deleted SDK; changelogs not updated).

Breaking-change check: the money double→string change and the TS protobuf-SDK deletion do break existing consumers, but both are intended and documented as such (a pre-v1 breaking proto change; the two real consumers couldn't use protobuf-native anyway) — so they satisfy "must not break existing flow unless explained + expected." The gap is that the breaking proto change is not recorded in either changelog (M1), which the repo's own documented process requires.


Findings (validated, deduplicated, most-severe first)

Legend: [fix: impl] implementation must change · [fix: doc] documentation must change · sources in brackets are the subagents that raised it + my own checks.

HIGH

H1 — Python money maps to Decimal, dropping the wire pattern → cross-language parity is violated. [fix: impl]
mark_money_decimal (scripts/sdk-types/merge_schema.py:68-83) tags money format: decimal, so datamodel-code-generator emits a bare Decimal and discards the sibling string.pattern. Verified: gen/python/wire/models.py:74 (amount: Decimal | None), :1077 (rate) carry no constr/regex — unlike Quota.metric/uri_digest, which keep theirs. Go keeps string.pattern; Zod keeps z.string().regex(...). Consequence (empirically confirmed by two agents; I confirmed statically): Pydantic accepts Decimal('-5'), 'NaN', 'Infinity', '1E3' and rejects valid empty "" — exactly the values Go and Zod reject/accept. This directly refutes the feature's central claim that the generated clients enforce every field-level rule exactly as the Go server, and its stated "no float anywhere in the money path" goal (NaN/Infinity are the float footguns re-entering). The MCP shim (Pydantic) is a trust boundary; only the server backstop keeps this out of Critical. The parity harness masks it — the only invalid money mutant in the corpus is "two words", which also fails Decimal parsing, so the verdicts coincide. Fix: keep money as a pattern-constrained str in Pydantic and convert to Decimal at the app layer (as Go does), or attach the pattern as a constr alongside a Decimal-coercing validator; add negative/NaN/Infinity/empty money mutants to the corpus. [security F1, arch H1, + my static check]

H2 — Forward-compatibility (extra="ignore") is defeated in both generated clients. [fix: impl]
gen/python/wire/base.py:16 hand-writes extra="ignore" and both the feature's design notes and gen/python/README.md promise "a field from a newer protocol version is dropped, not rejected." Reality (verified): all 59 generated models carry model_config = ConfigDict(extra='forbid') (gen/python/wire/models.py — 59 subclasses, 59 extra='forbid', 0 ignore), which in Pydantic v2 overrides the base — WireModel.extra is dead code. TS has no seam at all (gen/ts/wire/ holds only schemas.ts): json-schema-to-zod emits a per-schema mix of .strict() (reject unknown) and .catchall(z.union([<fieldtypes>, z.never()])) (accept an extra key only if its value happens to match an existing field type) — neither is proper passthrough. Root cause (verified by two agents independently regenerating protoschema, and provable by logic): the "non-strict" protoschema variant merge_schema.py selects still carries additionalProperties: false, so merge_schema.py:15-16's comment ("NON-strict variant (no additionalProperties:false) so extra policy is controlled once on the WireModel base") is factually wrong, and nothing strips it. Net effect: an older SDK breaks on a newer-protocol message — the exact drift the design exists to kill — and the single-seam premise is false. The ext map itself stays open, so extension profiles are safe; what breaks is forward-compat for new top-level fields + the documented client contract. Untested (corpus never feeds an extra field). Fix: strip/neutralize additionalProperties on message objects in merge_schema.py (let the base govern) and normalize the Zod extra-policy + add a gen/ts/wire/base.ts seam; add a negative parity case asserting an unknown field is accepted. (If forbid is actually desired, then all three doc sites must change instead — but the stated design intent is explicit that new-field forward-compat is a goal, so impl is the wrong side here.) [DRY H1, exec H1, security F5, arch H2, layering (partial) — 5 agents; I resolved the layering-vs-DRY contradiction against the file: seam is shadowed]

H3 — Codegen is not deterministic → the CI drift-gate (the feature's core guarantee) is a time-bomb. [fix: impl]
scripts/gen-sdk-types.sh pins datamodel-code-generator==0.64.0 and json-schema-to-zod@2.8.1, but the byte-compared models.py is formatted by black, pulled only transitively and unpinned (a fresh install resolved black 26.5.1); protobuf and isort (pip, line 33) and zod (npm, line 59) are likewise unpinned, and step 4 uses npm install (not npm ci) with no committed lockfile. The design claims the byte-compared output is deterministic in CI. A black/isort release reformats the output and the drift gate — the mechanism the whole "can't drift" story rests on — fails on an unrelated day. Fix: pin black, isort, protobuf in the venv install and zod in the npm step; commit a lockfile and use npm ci. [exec #2, + my prior; security F7 & DRY #4 touch the same pins]

H4 — Root README.md (edited in this PR) documents an SDK that does not exist. [fix: doc]
Still claims Python from ramp.v1 import ramp_pb2 (README.md:64), .pyi + Connect service stubs (:61), TS "Protobuf-ES + Connect-ES" (:57), and "All three SDKs are generated … by buf generate" (:38). All false after this PR: the protobuf-native TS/Python outputs were deleted (grep -r _pb2 gen/python = none), Pydantic/Zod come from scripts/gen-sdk-types.sh, and from ramp.v1 import ramp_pb2 raises ImportError for anyone who copies it. Contradicts the correct gen/python/README.md and buf.gen.yaml's own comment. Fix the README (impl matches sdk-lib intent). [consistency #1, layering #4, + my check]

MEDIUM

M1 — Neither changelog mirror was updated. [fix: doc]
The repo's documented process mandates updating both proto/CHANGELOG.md and website/src/content/docs/reference/changelog.mdx after meaningful proto changes; neither is in the diff, and neither records the breaking money double→string change or the validation refactor. A breaking pre-v1 change with no changelog entry is a process + conformance gap. [consistency #2, + my check]

M2 — Pydantic normalizes money on re-emit → byte-exact round-trip lost. [fix: impl]
Because money is Decimal, parse→dump rewrites pattern-valid strings: "007.50"→"7.50", "1E3"→"1E+3". If an MCP shim or Broker parses and re-serializes a JWS-signed offer (or anything under Content-Digest/RFC-9421 coverage), the bytes change and the signature/proto.Equal idempotency breaks. Zod preserves the string; Python doesn't. Untested (corpus money value is only "0"). Ties to H1 — fixing money-as-str resolves this too; add a money round-trip byte test. [security F2, arch M3]

M3 — Closed-enum collapse is incomplete; "each enum field is closed" is over-claimed. [fix: doc, optional impl]
close_enum_unions only collapses 2-branch anyOf (merge_schema.py:125 len(aof) == 2). Enum fields without not_in:[0] are emitted as 3-branch unions (UNSPECIFIED-name | enum | integer), so AcceptableRestriction.axis, delivery_method, status stay open: verified gen/python/wire/models.py:928axis: constr(...UNSPECIFIED) | RestrictionKind | conint(int32) | None = Field(0) accepts raw ints (incl. 0/9999) and defaults to the sentinel. Security impact is low (these aren't discriminators; open matches Go's forward-compat enum semantics), but the feature description implies universal closure. This is by-accident, not by-design — the 11 discriminators are closed only because they happen to be 2-branch. Fix: correct the claim to "closed for not_in:[0] discriminators only," and optionally collapse 3-branch unions too. (This corrects my own earlier assumption that all enums are closed — the security/arch agents were right.) [security F4, arch checked-ok note, + my confirmation]

M4 — Money field defaults to '' (str) under a Decimal | None type. [fix: impl]
gen/python/wire/models.py:74 amount: Decimal | None = Field('') — an empty-string default on a Decimal field (non-optional proto fields aren't in the required set, so mark_required never drops the proto3 default). Pydantic emits a serialization warning and it undermines the "exact Decimal" story. Fix: drop the default in mark_money_decimal. [exec #4, arch #6, + my confirmation]

M5 — The parity harness proves less than "every field-level rule." [fix: impl]
conformance/corpusgen/main.go generates no too_short/too_long mutants for repeated.items.string.min_len/max_len (enforced in clients, never exercised); its missing edge fires only for the one explicit required field, so pattern-derived required-presence (e.g. Quota.metric omission) is untested; money's divergent space (neg/NaN/empty) is untested (H1); and the raw-int defense for the 11 discriminators is never exercised (mutants need defined_only, which they lack). The harness is the feature's proof-of-correctness, so its blind spots are how H1/M3 shipped silently. Fix: add the missing mutant classes. [arch #4, exec #5/#6]

M6 — scripts/ci-local.sh is not the "single local mirror" it claims. [fix: impl or doc]
The feature notes and the script's own header call it the single local mirror of the gating sequence, but it mirrors proto-ci.yml only — it never runs gen-sdk-types.sh, the models/schemas drift check, the Pydantic/Zod parity suites, or check-canonical.sh. A developer running it locally gets zero coverage of this feature's guarantees. Fix: add the sdk-types steps, or correct the claim. [DRY #2, exec #3, arch #5 — 3 agents agree]

M7 — Money examples in docs only partially converted to decimal strings. [fix: doc]
Wire JSON in some pages was converted, but pseudocode/sibling examples were left as bare numbers, so a single doc now shows both forms: scenario-walkthrough.mdx:137-318 (pseudocode sketches), licensing-terms.mdx:156/173/192/213/226, standards-layering.mdx still show rate: 0.05/500.00. Fix: sweep and either convert to decimal strings or add a "money is a decimal string on the wire" note. (Prose like "rate=0 for subscriptions" is fine — describes a value, not a wire encoding.) [consistency #4]

LOW

L1 — gen/python/pyproject.toml:2-3 repeats the wrong "generated by buf generate" claim and omits the hand-written wire/base.py from its "only authored files" list. [fix: doc] [layering #2, consistency]

L2 — Component docs still describe protobuf-native wiringcomponents/mcp-server (protoc-gen-go-mcp), components/agent-sdk (@bufbuild/protobuf). Verify whether these describe this repo's output or the separate reference-implementation before editing. [fix: doc, verify scope] [consistency #5]

L3 — jsonl-ingestion.mdx:83 shows numeric rate:0.05. Ambiguous, flagged not asserted: this is the ramp-ingest publisher JSONL dialect (same line uses lowercase "model":"per_unit", "semantics":"enumerated", a functions[] field — none of which are proto-JSON), not proto wire JSON, so a numeric rate may be intended and correct there. Whether it must be a string depends on ramp-ingest, which lives in the separate reference-implementation repo. Open question — confirm against ramp-ingest rather than blindly converting. (I explicitly downgraded the consistency agent's "would fail protojson / MEDIUM" here.) [consistency #4, tempered by me]

L4 — Money pattern has no max_len. A multi-megabyte digit string is accepted; measured ~0.022s for 5M digits (Pydantic/RE2/RegExp are all linear), so this is a minor memory-amplification nit, not a real DoS. (Security agent rated MEDIUM; I downgrade to LOW on the measured evidence.) Optional: add string.max_len. [security F3 / arch #7]

L5 — MONEY_PATTERN is a 6th hardcoded copy of the proto's 5 money patterns (merge_schema.py:65), used for exact-equality detection. A proto pattern edit not mirrored here silently degrades Python money Decimal→str (fails open, untested). [fix: impl] [DRY #3]

L6 — corpusgen/requiredgen hardcode Int64/Int64Kind — a future int32/uint*/int64.gt rule silently gets no parity coverage and no required-marking. Latent (only Quota.limit int64.gte=1 exists today). [fix: impl, future] [exec #6, arch #4]

L7 — Supply-chain hardening gap. buf plugins + Go generators are hash-pinned via go.sum, but the pip/npm installs have no lockfile / --require-hashes, and npm install runs postinstall scripts with GITHUB_TOKEN in the CI env. The drift gate catches output tampering, not build-time code execution. [fix: impl] [security F7, exec, DRY]

L8 — Cosmetic/stale: proto comments ramp.proto:441 ("Pricing.rate = 0"), :1705/:1751 ("cost.amount=0") now describe string fields; gen/ts has no README (Python does) and package.json has no license field; requiredgen lives in conformance/ but its only consumer is gen-sdk-types.sh (mild scripts→conformance inversion; its docstring overstates a "shared artifact"); a dangling scripts/gen-corpus.sh reference. [various, LOW]


What I independently verified as CORRECT (so coverage is explicit)

  • Money conversion is complete and consistent: all 5 money fields (Pricing.rate/unit_cost, Cost.amount/unit_cost, RequestConstraints.max_unit_cost) carry the identical pattern; zero double fields remain; none missed; represented as Decimal/decimal-string in both clients. (The feature description's "TransactionItem.max_unit_cost" is a naming slip — the field is RequestConstraints.max_unit_cost; no TransactionItem money field exists.)
  • CEL→standard-constraint moves are faithful: all 7 format→string.pattern moves preserve anchoring + empty-string handling; all 11 enum→not_in:[0] moves are equivalent to the removed message-level _specified CEL (all 11 are non-optional scalars); the FREE-rate CEL rewrite covers every zero form and rejects non-zero; no cross-field CEL was collaterally dropped — exactly 7 remain, matching the docs.
  • Negatives are correctly disallowed on the wire — credits/refunds are implementation-specific (ramp.proto:95-96), DisputeResponse carries no Cost, and billing-adapter "negative amount" is internal. (So H1's problem is that Python wrongly accepts negatives, not that the proto forbids them.)
  • Counts match the description exactly: 11 discriminators, 5 money fields, 7 formats moved, 7 cross-field CEL kept.
  • Vocab single-source is real and enforced: the hand-maintained Go axis→package maps are gone (confirmed via git show origin/main), replaced by proto options vocab_package (50003) / vocab_enum_package (50004); cmd/protoc-gen-rampvocab emits Go+TS+Python in one pass and machine-enforces presence; the constant sets are identical across all three languages and gated by vocab_parity_test.go (5 axes, 3 languages).
  • merge_schema.py consumes requiredgen's output rather than re-deriving required-ness (DRY holds); it's deterministic (sorted glob, sorted $defs, sorted required); required-presence derivation is sound with no short-name collisions among the 59 messages.
  • Structural invariants intact: dispute chain unbroken; ext/ext_critical correctly modeled as explicit fields (no COSE-crit bypass); no message only Go can construct.
  • No ReDoS in any new regex (all linear under re/RegExp/RE2); the 11 discriminators genuinely reject raw 0/9999; proto-ramp.mdx field-type tables auto-render from the regenerated descriptor, so money types there update automatically (not stale).

Recommended resolution order

  1. H1 + M2 + M4 together — make Pydantic money a pattern-constrained str (convert to Decimal at the app layer), which fixes the parity violation, the re-emit byte drift, and the bad default in one move.
  2. H2 — strip additionalProperties in merge_schema.py, add a Zod seam, add a negative (unknown-field-accepted) parity case.
  3. H3 — pin black/isort/protobuf/zod, commit a lockfile, use npm ci.
  4. M5 — add the missing corpus mutant classes so H1/H2/M3-type regressions can't ship green again.
  5. H4 + M1 + M7 + L1/L2 — documentation conformance pass (README, both changelogs, money examples, pyproject, component docs).
  6. M3, M6, L4–L8 — claim corrections + hardening.

Money fields were tagged format:decimal in merge_schema.py, so
datamodel-code-generator emitted a bare Decimal and dropped the sibling
string.pattern. The generated Pydantic models then accepted -5, NaN,
Infinity and 1E3 and rejected a valid empty string — diverging from the Go
server and the Zod client, and reintroducing float values into the money
path.

Remove the format:decimal diversion so money regenerates as a
pattern-constrained string (constr) like the other decimal-string fields;
convert to a decimal at the application layer, as Go does. Zod is
byte-unchanged (it already emitted a regex string). Adds a behavioral test
pinning the pattern across all five money fields.
The non-strict JSON Schema variant still closed every message object: with
additionalProperties:false (baked into per-model extra='forbid' in Pydantic and
.strict() in Zod) and with patternProperties for the snake_case field-name
aliases (compiled by json-schema-to-zod into a catchall+superRefine that rejects
unknown keys). Both shadowed the WireModel base, so a message carrying an unknown
top-level field from a newer protocol version was rejected instead of accepted —
the exact drift the single-seam design exists to prevent.

Strip both closing constructs in merge_schema.py so the extra policy is governed
once: Pydantic models inherit the WireModel extra='ignore' base, and a new
gen/ts/wire/base.ts wire() seam applies the equivalent policy to every Zod schema
(default strip: an unknown field is accepted and dropped, matching Pydantic and
the Go typed-struct re-marshal). Dropping the snake_case aliases also tightens the
canonical form to camelCase. Adds forward-compat behavioral tests in both
languages.
Money is a decimal string on the wire; parse then dump must preserve the exact
bytes so a JWS-signed offer under RFC 9421 Content-Digest coverage keeps its
signature. Guards against a normalizing round-trip (leading/trailing zeros, high
precision, empty). Rides on the money-as-string fix.
…tput

The generated Pydantic/Zod output is byte-compared by the drift gate, but the
tools that shape it were only partly pinned: black (formats the models), isort,
protobuf, and zod floated, and the Zod step ran npm install without a lockfile. A
routine black/isort release would reflow the output and fail the gate on an
unrelated day. Pin black, isort and protobuf in the venv install and add a
committed manifest + lockfile so the Zod step runs npm ci against an exact tree.
Regenerating with the pins reproduces the committed output byte-for-byte.
The root README still described a protobuf-native stack that this repo no longer
produces: Python `ramp_pb2` + .pyi + Connect service stubs, TypeScript Protobuf-ES
+ Connect-ES, and "all three SDKs generated by buf generate". After the types
export, Go remains native protobuf + Connect (buf generate), while Python is
Pydantic models and TypeScript is Zod schemas, both from scripts/gen-sdk-types.sh.
Update the prose and the import examples so a copied snippet actually resolves.
Both changelog mirrors (proto/CHANGELOG.md and the website changelog) omitted the
breaking money field-type change and the validation-as-standard-constraints
refactor. Add an Unreleased entry to each: money moves from double to a decimal
string with a pattern, and 18 of 25 field-level rules move from custom CEL to
standard protovalidate constraints so they reach the generated clients.
wire/models.py and vocab/* come from scripts/gen-sdk-types.sh, not buf generate,
and the hand-written wire/base.py seam belongs in the authored-files list.
The wire-JSON blocks already used string money, but the pseudocode sketches
higher in the same page still showed bare numbers (rate: 0.05, unit_cost:
0.00001515, amount: 0), which reads as a float. Quote them so the sketch matches
the decimal-string wire form shown below it.
close_enum_unions collapses only the 2-branch anyOf that not_in:[0] discriminators
produce; enum fields without not_in:[0] keep their UNSPECIFIED member, emit a
3-branch union, and are deliberately left open (name-or-number, matching proto
forward-compat). Document that 'closed enums' means the discriminators, not every
enum field.
- proto: money-value comments show the decimal-string form (rate = "0",
  cost.amount = "0") now that money is a string; regenerated descriptor + gen.
- gen/ts: add the missing README and the package license field.
- corpusgen: fix a dangling regenerate reference to a nonexistent script (the real
  command is 'go run ./conformance/corpusgen').
- requiredgen: state its actual consumer (gen-sdk-types.sh) instead of implying a
  shared test artifact.
ci-local.sh mirrored only the proto CI, so a developer running it got zero coverage
of the generated types export. Add the sdk-types gate — regenerate gen-sdk-types,
assert no drift, run the Pydantic/Zod parity suites and the canonical round-trip —
so one local command covers everything. The two remain separate CI workflows:
proto-ci.yml sets RAMP_CI_SKIP_SDK_TYPES=1 (sdk-types-ci.yml owns it there), and the
block self-skips when python3/npm are absent.
The money decimal-string pattern had no length bound, so an arbitrarily long digit
string was accepted (a minor memory-amplification nit). Add string.max_len = 32 to
all five money fields — ample for real amounts with sub-cent precision. Regenerated
gen and the corpus, which now exercises a too-long money mutant across the three
clients.
requiredgen only evaluated int64 rules for the zero-is-rejected check, so a future
int32/uint*/float rule would fall through as 'zero is valid' — the field would not
be marked required and the generated clients would accept an omission the Go server
rejects. Panic instead of silently under-marking. corpusgen's validScalar already
dies on unhandled kinds, so the corpus side is covered.
The types-export generation runs npm ci without executing third-party lifecycle
scripts (--ignore-scripts) — json-schema-to-zod and zod are pure JS, and this step
produces the drift-gated schemas.ts, so no install-time code should run with the CI
token in env. Also pin pydantic to an exact version in the canonical + parity installs
instead of >=2.0. (The gen/ts test step keeps its scripts: vitest's esbuild needs its
postinstall.)
The corpus exercised only one pattern mutant per field and one valid baseline, so
whole classes went unchecked — most importantly money's decimal-string killers
(-5/NaN/Infinity/1E3, which a naive Decimal accepts) and the valid empty-string
boundary. Emit one pattern mutant per rejected bad-string (money killers appended,
index-keyed for stable ids), a positive empty-money case, pattern-derived
required-presence, and repeated-item min/max mutants. A coverage guard pins the four
classes. Corpus 86 -> 170; Pydantic and Zod match the Go oracle on all.
Delegation.token is a proto bytes field, which protoschema renders as a base64 string
with default null. json-schema-to-zod turned that into .default(null), and Zod
re-validates the default against z.string(), so a Delegation with token omitted failed
to parse. Normalize a null default to "" on string nodes in merge_schema (a string's
proto3 zero is the empty string, which the base64 pattern accepts). Drops the explicit
token workaround the forward-compat test needed.
The poc/ directory was removed from the repo, leaving dead links across the
component and getting-started docs. Drop them: point the SDK examples at the
generated gen/ outputs where relevant, and mark the unpublished packages as
planned. The PoC will be reworked separately.
The pip installs pinned exact top-level versions but left the transitive tree and
its integrity unpinned. Add pip-compile --generate-hashes lockfiles (requirements-gen
for the Pydantic toolchain, requirements-test for the parity/canonical runtime) and
install with --require-hashes, so a tampered or drifted dependency fails closed. Each
package carries all per-distribution hashes, so one file works on macOS and CI. Runtime
pydantic aligns to 2.13.4 (what the generator resolves) to keep one venv consistent;
generated output is byte-identical.
@KonstantinMirin

Copy link
Copy Markdown
Contributor Author

Thanks for the exceptionally thorough review — the triage was spot on, especially the observation that the three flagship properties weren't actually achieved and the parity harness was masking all three. That framing drove the fix order. All HIGH/MEDIUM/LOW are now addressed and pushed (18 commits, 10d707b..b437197); ./scripts/ci-local.sh is green end-to-end.

HIGH

  • H1 — Python money dropped the wire pattern. Root cause was mark_money_decimal tagging money format: decimal, so datamodel-code-generator emitted a bare Decimal and discarded the sibling string.pattern. Deleted that diversion; money now regenerates as constr(pattern=…) like every other decimal-string field, converted to a decimal at the app layer as Go does. Zod was byte-unchanged. Pydantic now rejects -5/NaN/Infinity/1E3 and accepts "", matching Go and Zod. This also resolved M2 (byte-exact round-trip) and M4 (the ''-under-Decimal default) at the source. 6fffae0, fe8bc39.
  • H2 — forward-compat defeated. You were right that the "non-strict" protoschema variant still carried additionalProperties:false; merge_schema never stripped it. Fixed there, and while implementing it I found a second closing mechanism you'd want to know about: the snake_case field-name aliases (patternProperties: {"^(unit_cost)$": …}) compile to a .catchall(...) + superRefine that also rejects unknown keys, so 48 multiword-field messages stayed closed even after the additionalProperties fix. Both are now stripped; models inherit the WireModel extra="ignore" base (0 per-model extra='forbid'), and a new gen/ts/wire/base.ts wire() seam governs the Zod side (default strip = accept-and-drop, matching Pydantic). Dropping the snake_case aliases also tightens the canonical form to camelCase. Behavioral forward-compat tests added in both languages. f754230.
  • H3 — non-deterministic codegen. Pinned black/isort/protobuf/zod, added a committed lockfile + npm ci, and (see L7) hash-locked the pip tree with --require-hashes. Regenerating reproduces the committed output byte-for-byte. f8219d1, b437197.
  • H4 — README documented the deleted SDK. Rewritten to the real pipeline (Go native protobuf; Python Pydantic + TS Zod from scripts/gen-sdk-types.sh); the import examples now resolve. 528b7a9.

MEDIUM

  • M1 — both changelog mirrors now record the breaking money double→string change + the validation refactor. 31d1538.
  • M3 — corrected the over-broad claim. "Closed enums" means the not_in:[0] discriminators only; enum fields without not_in:[0] stay open 3-branch by design (documented at close_enum_unions). 67c2016.
  • M5 — the harness gap. Added the missing mutant classes to corpusgen: money killers (-5/NaN/Infinity/1E3), the valid empty-money boundary (via a positive-case path — the exact blind spot), repeated-item min/max, and pattern-derived required-presence. Corpus grew 86 → 170; Pydantic and Zod match the Go oracle on every new case, and a coverage guard pins the four classes. 958b23f.
  • M6ci-local.sh now genuinely mirrors everything: it runs the sdk-types regenerate/drift/parity/canonical gate too (proto-ci keeps its scope via RAMP_CI_SKIP_SDK_TYPES=1). 5fbf5b3.
  • M7 — walkthrough pseudocode money values quoted to match the wire form. 9459f69.

LOW

  • L1 4656217 · L4 money max_len = 32 (which also gave money a too_long corpus mutant for free) 9a06b79 · L5 already resolved by H1 (MONEY_PATTERN deleted) · L6 requiredgen now fails loudly on an unhandled numeric kind 91cc051 · L7 --ignore-scripts on the generation step + hash-locked pip 6981115, b437197 · L8 cosmetic bundle (proto comments, gen/ts README + license, dangling script ref) c220de1.
  • L2 / L3 — verified rather than blindly edited, as you flagged: the component docs describe the reference stack (not this repo's types export), and jsonl-ingestion.mdx is the ingest publisher dialect (not proto wire JSON), so the numeric rate there is intended. No change needed to the wiring/rate; separately I did remove the dead poc/ links that surfaced during that check. 61315ff.

Two things worth flagging

  • A latent bug your review surfaced indirectly. Writing the H2 test exposed that Delegation.token (a proto bytes field) generated with .default(null), which Zod re-validates against z.string() — so a Delegation with token omitted failed to parse. Fixed by normalizing a null default to "" on string nodes. 89c98ca.
  • One finding I'm proposing not to chase: raw-int mutants for the closed discriminators. They can't be shared-verdict parity cases — Go accepts an undefined enum int (not_in:[0] only forbids 0; no defined_only, so open-enum forward-compat applies), while the closed-enum clients reject it (verified: Pricing.model=9999 → Go err=nil, clients reject). That divergence is the intended trade of closing the discriminators. Restoring parity via enum.defined_only would permanently forbid adding enum values without breaking old servers, which seems wrong for an evolving protocol; a clients-only "rejects 9999" test is redundant with the name-only enum shape. Happy to add the small guard if you'd prefer belt-and-suspenders, but my inclination is to leave it.

Ready for another pass whenever you are.

@legendko legendko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

One finding after the re-review:

N1 — Clients silently drop snake_case wire-JSON field names that the Go server accepts

Dropping the per-field snake_case aliases (part of the H2 fix) left the generated Pydantic/Zod clients camelCase-only. Proven empirically (both languages): {"unit_cost":"0.05"} → Go protojson populates unitCost, while Pydantic and Zod silently drop it (field stays default). Worse, it is not only data loss but a verdict divergence: {"unit_cost":"NOT_A_NUMBER"} → Go returns INVALID (the field's string.pattern), the clients return VALID — a direct violation of the "clients enforce exactly as the Go server" property, and the corpus/docexamples_test.go are camelCase-only so the harness is blind to it.

  • Breadth: ~36 .mdx files use snake_case keys in real json wire blocks — not just money: offer_id(124), offer_signature(64), estimated_quantity(61), unit_cost(44), delivery_method(43), transaction_id(36), billing_id(35), resource_mutability(33), incl. the canonical reference/ramp-json-example.mdx. No doc anywhere states a camelCase-vs-snake_case convention.
  • Why not HIGH: the canonical wire form the Go server actually emits is camelCase — proven here: the parity corpus payloads are camelCase ({"json":{"unitCost":"0"}}), and that corpus is generated from Go protojson. So real server→client traffic works; the harm is to a developer who hand-authors JSON from the docs against the generated SDK. Why not lower: it is pervasive, hits dispute-chain fields, is a genuine parity break, and is silent.
  • Provenance: camelCase-only field naming is pre-existing, but H2 changed the failure mode from loud reject (extra='forbid') to silent drop (.strip()/ignore) — strictly more dangerous.
  • Resolution (a decision the author should make — this is the one conflict to resolve):
    • Option A (impl, recommended for parity): make the clients accept snake_case like Go does (a clean camelCase↔snake alias, not the per-field patternProperties mess H2 removed). This restores "enforce exactly as Go", keeps all 36 doc files valid, and matches the proto-native field names developers read.
    • Option B (doc): commit to camelCase-canonical, recase every proto-JSON wire example to camelCase, and add an explicit "wire is camelCase proto-JSON" note. Larger doc churn; diverges from the .proto field names. Leave the ingest-dialect JSONL (snake_case) alone — it is not proto wire (round-1 L3).
    • Either way: add a snake_case corpus/doc-example case so the harness stops being blind to this class.
    • Assumption behind the grade: the reference server emits camelCase (strongly implied by the camelCase corpus). If it actually emits snake_case (protojson UseProtoNames), N1 becomes HIGH (real traffic breaks) — worth a one-line confirmation against the reference-implementation repo.

The generated clients were camelCase-only while Go protojson accepts both the
camelCase json_name and the original snake_case field name — so snake_case input
(which the proto, and ~all the docs, use) was silently dropped by the clients, and
an invalid snake_case value passed the clients while Go rejected it (a parity break).

Rather than alias both forms, standardize on snake_case everywhere — one name across
the proto, the docs, the wire, and both clients:
- corpusgen emits snake_case (protojson UseProtoNames=true);
- merge_schema consumes protoschema's proto-names (.schema.json) variant, so the
  Pydantic/Zod field names are snake_case (and idiomatic in Python);
- requiredgen keys the required-set by proto field name to match;
- a TestWireIsSnakeCase guard fails if a camelCase key ever re-enters the corpus.

The clients now enforce field rules exactly as Go on the canonical (snake_case) form.
The docs needed no changes — they were already snake_case.
@KonstantinMirin

Copy link
Copy Markdown
Contributor Author

Good catch on N1 — confirmed exactly as described. Verified empirically both directions: {"unit_cost":"0.05"} → Go populates the field, both clients silently drop it; and the verdict divergence {"unit_cost":"NOT_A_NUMBER"} → Go INVALID (does not match regex), clients VALID. Agreed it's a real parity break, and that H2 turned it from a loud reject into a silent drop.

Rather than either proposed resolution, we're closing the whole class: snake_case proto-JSON everywhere, no aliasing. cd6effe.

Reasoning:

  • The invariant is "clients enforce exactly as the Go server," and the divergence only exists because of the camelCase↔snake duality. Option A (accept both via aliases) keeps that duality alive as a permanent alias map; Option B (camelCase-canonical) doesn't actually fix the invariant — a developer who hand-authors snake_case (the natural choice, since the .proto is snake_case) still gets the silent-wrong result. Standardizing on snake_case deletes the duality instead of managing it.
  • One name across every artifact: the .proto field, the wire, the docs, the Pydantic attr (now idiomatic Python), the Zod key, the corpus, and protovalidate's error paths. No mental translation, greps consistently.

What changed:

  • corpusgen emits snake_case (protojson.MarshalOptions{UseProtoNames: true}).
  • merge_schema consumes protoschema's proto-names (.schema.json) variant, so the generated Pydantic/Zod field names are snake_case.
  • requiredgen keys the required-set by proto field name to match.
  • New TestWireIsSnakeCase guard fails if a camelCase key ever re-enters the corpus (protojson still accepts camelCase on input, so a stray UseProtoNames=false would silently split the naming again — the guard catches that).
  • The docs needed no changes — they were already snake_case across the board (incl. ramp-json-example.mdx). That's the tell that snake_case is the intuitive form here.

Net: the clients now enforce every field rule exactly as Go on the canonical (snake_case) form; the harness is no longer camelCase-blind (the whole corpus is snake_case and guarded). camelCase input is simply out of contract now. This is being adopted as the universal convention — the downstream Go-SDK work (which had pinned camelCase-JCS for signing) will flip its canonical form to snake_case-JCS to match, so all three languages and the wire stay identical. Standardized: snake_case everywhere.

@legendko legendko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N1's core — the verdict divergence and the silent-drop — is RESOLVED, and independently proven in all three languages. The author did not take either option the review offered (aliases, or camelCase-canonical); instead it standardized the wire on snake_case proto-JSON everywhere — proto, docs, corpus, and both clients — deleting the camelCase↔snake duality rather than managing it. That is a sound decision and it is cleanly implemented.

However, the fix is INCOMPLETE on documentation conformance, and the author's central claim is factually wrong. The commit message and main message both assert "The docs needed no changes — they were already snake_case." They were not. The documentation is mixed-case: while the majority of wire fields are snake_case, idempotencyKey appears camelCase in real wire-JSON blocks across 8 walkthrough files, and the ErrorDetail fields transactionDenial / usageReportRejection / restrictionMismatches are camelCase in error examples. Because idempotency_key is a required field, the now-snake-only clients hard-reject every documented transaction / usage-report / dispute example. This is the same class of defect as N1 — a doc↔client casing mismatch — in the opposite direction: the fix healed the snake-case majority and newly broke the camelCase minority.

It is trivially fixable (recase ~4 field names in the doc examples), but it is a real, unresolved conflict — so N1 is not fully closed: its verdict-divergence dimension is closed, its doc-conformance dimension is only partially closed.

Two smaller residuals: the breaking wire change is not recorded in either changelog (contra the round-1 M1 precedent), and the snake_case-canonical decision — now load-bearing for JCS signature bytes — is stated nowhere in the shipped spec, while the harness (docexamples_test.go) remains structurally blind to doc-example casing (which is exactly why idempotencyKey slipped through).

Nothing here is a code defect. The generated clients, corpus, guard, and determinism are all correct. The gap is documentation and spec-completeness.


Residual / new issues

RN1 — Documentation not fully recased; the "docs needed no changes" claim is false — MEDIUM

The snake-only clients now conflict with camelCase proto-field remnants that survive in the docs. These are real ```json wire blocks, not prose.

(a) idempotencyKey — HARD REJECT. idempotency_key is required (min_len:1, max_len:255) on all three state-mutating messages (TransactionRequest, UsageReport/ReportUsage, DisputeRequest). A doc example keying it idempotencyKey → the client strips the unknown camelCase key → the required idempotency_key is missing → ValidationError: field required. Present in the primary transaction walkthroughs:

File Example
protocol/transaction-flow.mdx TransactionRequest/UsageReport/DisputeRequest blocks (l.275, 335, 416, 538)
getting-started/poc-walkthrough.mdx curl bodies (l.149, 205, 228)
protocol/scenario-walkthrough.mdx l.620, 640, 773
protocol/walkthrough-v1.mdx l.184, 253
protocol/walkthrough-{credit-report,medical-imaging,eu-regulation,academic}.mdx txn + usage-report blocks

(≥21 occurrences over 8 files. components/exchange/scaling.mdx also matches but is a Go variable name, not a wire key — not a conflict.)

(b) ErrorDetail oneof — SILENT DROP. Proto fields are transaction_denial / usage_report_rejection / restriction_mismatches; docs write them transactionDenial / usageReportRejection / restrictionMismatches in error examples (poc-walkthrough.mdx:237,252, scenario-walkthrough.mdx:816, walkthrough-medical-imaging.mdx:257-259). The client strips the camelCase key → the oneof is empty → the denial/rejection reason is silently lost.

Provenance. camelCase idempotencyKey matched the pre-cd6effe camelCase clients, so cd6effe newly broke these examples — a genuine regression for them, even as it fixed the far larger snake-case set. The canonical reference example (reference/ramp-json-example.mdx) and the proto mirror (reference/proto-ramp.mdx) are already clean snake_case; it is the walkthroughs that are stale.

Why MEDIUM (not higher/lower). It hits the primary documented flow (transaction execution) across every walkthrough including getting-started, and contradicts an explicit author claim + the primary-review conformance bar — but it is loud (a required-field rejection the developer sees immediately, not silent data loss), touches only ~4 field names, is a pure doc edit, and the canonical reference doc is already correct.

Resolution (author's call; this is the conflict to resolve):

  • Recase the doc examplesidempotencyKeyidempotency_key, transactionDenialtransaction_denial, usageReportRejectionusage_report_rejection, restrictionMismatchesrestriction_mismatches — across the 8+1 files. This is what "docs needed no changes" should have covered.
  • And/or add a doc-example gate so this class cannot recur (see RN3): docexamples_test.go currently never parses the doc JSON through the clients/protojson.

RN2 — Breaking wire change absent from both changelogs — LOW

cd6effe is feat(sdk-types)! — a wire-observable breaking change (protojson output and the accepted client input both move camelCase→snake_case). Neither proto/CHANGELOG.md nor website/src/content/docs/reference/changelog.mdx records it (grep for snake/camel/proto-JSON/json_name naming = nothing). Round 1's M1 established that breaking wire changes must land in both changelog mirrors (the money double→string entry is there), and the website changelog must mirror the proto one. Fix: one "Unreleased / accepted pre-v1 breaking" entry in both, e.g. "Wire is snake_case proto-JSON (protojson UseProtoNames); camelCase json_name is out of contract for the generated clients."

RN3 — Canonical form undocumented in the spec; harness blind to doc casing — LOW–MEDIUM

The whole fix rests on "snake_case is the canonical wire form, camelCase is out of contract," yet no shipped doc states it — the decision lives only in the commit message and a test comment. This matters beyond ergonomics: the spec signs several messages with Ed25519 over JCS-canonicalized (RFC 8785) JSON (Offer/offer_signature, ResourceAttestation, content-attestation.mdx, for-verification-vendors.mdx, ramp.proto:722). JCS sorts keys lexicographically, so field-name casing determines the signed bytes — flipping camelCase→snake_case changes every signature's canonical input. The author's own response confirms this ("the downstream Go-SDK work, which had pinned camelCase-JCS for signing, will flip to snake_case-JCS"), which is precisely why the canonical form belongs in the protocol spec (this repo is the protocol definition), not just a commit. Fix: a one-paragraph canonical-form note in reference/proto-ramp.mdx (and/or the proto header), stating snake_case proto-JSON is the canonical wire and JCS-signing input.

Companion harness gap: docexamples_test.go has no protojson/Unmarshal — it does value/shape spot-checks, never round-trips the doc JSON through the clients. So CI cannot detect RN1. The new TestWireIsSnakeCase guards the corpus against camelCase regression but not the docs. A gate that parses each doc ```json block through the generated clients (or at least asserts no camelCase proto-field keys in doc wire blocks) would have caught idempotencyKey.

Recommended before merge: RN1 (recase the doc examples). Recommended, low-cost: RN2 + RN3. No code changes required — the generated code, corpus, guard, and determinism are correct.

…e wire naming

The snake_case standardization missed camelCase remnants in the walkthrough
examples: idempotency_key (required) and the ErrorDetail oneof fields
(transaction_denial / usage_report_rejection / restriction_mismatches) were still
keyed camelCase in real wire-JSON blocks across 8 files, so the snake-only clients
hard-rejected the documented transaction/usage-report/dispute examples. Recase them
(content payloads like mcpServers are not proto fields and are left as-is).

Also: record the breaking snake_case wire change in both changelog mirrors; state the
canonical form (snake_case proto-JSON, load-bearing for JCS signature bytes) in the
proto header and the proto reference doc; and add TestDocExamplesAreSnakeCase, a
descriptor-driven gate that fails if any doc code fence keys a real proto field in
camelCase — the harness blind spot that let this slip.
@KonstantinMirin

Copy link
Copy Markdown
Contributor Author

You're right on all three, and the "docs needed no changes" claim was wrong — my bad. I'd checked the docs with a hardcoded field list that didn't include idempotencyKey, so I missed the exact regression the snake-only clients introduced. Verified each point against the tree, then fixed all three. 84c750c.

RN1 — camelCase proto-field remnants (confirmed, fixed). Reproduced the hard reject: TransactionRequest.model_validate({"offer_id":…,"idempotencyKey":…})ValidationError: ('idempotency_key',): missing. A comprehensive scan (every camelCase-hump JSON key in the docs, checked against the descriptor) found exactly the four you named as real proto fields — idempotency_key, transaction_denial, usage_report_rejection, restriction_mismatches — and confirmed the rest (mcpServers, companyInfo, the D&B/credit-report payloads, …) are example content, not RAMP fields, so they're left as-is. Recased the four across the 8 walkthrough files; the recased examples now validate through the snake clients. You were also right that components/exchange/scaling.mdx's idempotencyKey is a Go variable, not a wire key — the gate below keys off "field": so it's not flagged.

RN2 — changelog (fixed). Added an "Unreleased / accepted pre-v1 breaking" entry to both mirrors (proto/CHANGELOG.md + the website changelog): wire is snake_case proto-JSON (protojson UseProtoNames), camelCase json_name out of contract for the generated clients.

RN3 — canonical form in the spec + harness gate (fixed).

  • Stated the canonical form in the shipped spec — a "Wire format is snake_case proto-JSON" note in reference/proto-ramp.mdx and the ramp.proto header — including the load-bearing bit you flagged: signatures are Ed25519 over JCS-canonicalized (RFC 8785) proto-JSON, JCS sorts keys, so field-name casing determines the signed bytes.
  • Closed the harness blind spot: docexamples_test.go never parsed doc JSON through the client naming, which is exactly why idempotencyKey slipped. New TestDocExamplesAreSnakeCase builds the set of camelCase json_names that differ from their snake proto name (from the descriptor) and fails if any doc code fence keys one of them — proven to bite on an injected idempotencyKey and green on the recased tree. It's descriptor-driven, so it can't go stale the way my manual list did.

ci-local.sh green end-to-end. Appreciate the persistence catching this — it was a genuine regression, loudly wrong, and now both the docs and the harness are fixed.

Direct named guards on the client layer to complete the snake_case invariant across
every layer: the proto source is already enforced by buf lint (FIELD_LOWER_SNAKE_CASE),
the corpus by TestWireIsSnakeCase, and the docs by TestDocExamplesAreSnakeCase, but the
generated Pydantic/Zod field names were only caught indirectly (a camelCase regression
surfaced as cryptic parity mismatches). Assert directly, by introspecting model_fields
and the Zod object shapes, that every generated field key is snake_case.
@KonstantinMirin

Copy link
Copy Markdown
Contributor Author

Follow-up on the harness gap. You flagged that docexamples_test.go was structurally blind to casing, and that's what let idempotencyKey slip. Rather than only patch the doc gate, I closed the snake_case invariant at every layer, so a non-snake proto field can't appear anywhere — not just in the examples. 608db17.

Guard coverage now:

  • Proto sourcebuf lint FIELD_LOWER_SNAKE_CASE (STANDARD, not in except) rejects a non-snake field declaration. Verified it bites: Field name "idempotencyKeyX" should be lower_snake_case. Since every artifact derives from the proto, this is the root guarantee — a non-snake RAMP field can't be declared in the first place.
  • Generated clients — new gen/python/tests/test_snake_fields.py (introspects Pydantic model_fields) and gen/ts/tests/snake_fields.test.ts (walks the Zod .shape) assert every generated field key is snake_case. Previously a client-casing regression only surfaced as cryptic parity mismatches; now it fails with Cost.unitCost directly (verified by injection).
  • CorpusTestWireIsSnakeCase (already present) fails on any camelCase key in the corpus JSON.
  • DocsTestDocExamplesAreSnakeCase (the gate from the last round) — descriptor-driven, fails if any doc code fence keys a real proto field's camelCase json_name.

Scoping note, since it's the subtle part: the guards are keyed to RAMP fields (via buf lint / the generated types / the descriptor), not "any camelCase key." The docs legitimately carry camelCase in content payloadsmcpServers, the D&B/credit-report data being licensed — that aren't RAMP wire fields, so a blanket casing ban would false-positive on them. The guards ask the proto "is this one of yours?" rather than pattern-matching blindly, so they neither miss a real field nor flag content.

Net: the source guard makes a non-snake RAMP field impossible to declare; the per-layer guards catch a pipeline regression (e.g. flipping back to the camelCase schema variant) with a clear, localized message. All four run in ci-local.sh and CI.

The canonical-form note referenced Offer.offer_signature, but Offer's signature
field is Offer.signature (offer_signature is a field on the transaction messages).
The docs remark-proto guard correctly rejected the unknown symbol; use the real one.

@legendko legendko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendko
legendko merged commit ef10a74 into main Jul 6, 2026
3 checks passed
@legendko
legendko deleted the feature/sdk-libraries branch July 6, 2026 14:09
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.

2 participants