feat(proto)!: Universal Licensing Core + proto-native vocabulary (protovalidate + codegen) - #6
Conversation
…s, quotas, obligations
Introduces a structured, cross-domain-portable licensing model that replaces
the flat AccessRestrictions structure. A resource now carries zero or more
LicenseTerm entries; each term is a complete access arrangement.
New messages: LicenseTerm, License, Restriction, Quota, Obligation (5 messages)
New enums: TermSemantics, RestrictionKind, QuotaWindow, ObligationKind,
ObligationTrigger, PricingMetering (6 enums)
Wire additions:
- Offer.terms (field 19) — repeated LicenseTerm for agents at discovery
- ResourceEntry.terms (field 13) — publisher-declared terms at ingest
- Pricing.metering (field 9) — ONLINE/NONE/OFFLINE_SELF_REPORTED
- PushResourcesResponse.warnings (field 3) — non-fatal ingest warnings
Breaking change: PRICING_MODEL_ATTRIBUTION (6) and PRICING_MODEL_CONTRIBUTION (7)
removed from PricingModel. Both were behavioral obligations, not payment models.
Migrate to LicenseTerm with Obligation.kind = ATTRIBUTION / CONTRIBUTION.
AccessRestrictions retained for backward compatibility.
Closes #5
…antics A license term with no Pricing is unactionable for an agent, so Pricing is now REQUIRED on every term — including REFERENCE_ONLY, whose License governs the human-readable terms but does not replace the machine-readable price. model=FREE must still be stated explicitly (absent Pricing is not free; a term may be FREE under an arbitrary license). Tightens the prior rule that exempted REFERENCE_ONLY from carrying Pricing. Comment-only contract change (proto3 has no required keyword); enforcement lives in the Exchange's licenseterm.Validate.
A REFERENCE_ONLY term references its governing terms in an external License document; one with no License.uri references nothing and is meaningless, so it is rejected at ingest. Comment-only contract change; enforcement lives in the Exchange's licenseterm.Validate.
…ired-on-every-term (RAMP-62)
…erm.scopes/part_label, Obligation.scope_license, UNSPECIFIED on required enums, drop revshare (RAMP-61)
These four registries (function, geography, user-type, quota-metrics) were committed to a stranded branch with no open PR. ADR-014 defines the vocab registry as part of the Universal Licensing Core, so they belong on this PR.
… = pricing-units vocabulary (RAMP-61)
PricingModel collapses to the closed charging structure {UNSPECIFIED, FREE,
PER_UNIT, FLAT}. The open-ended metering basis (per fetch/page/minute/record/
stream/...) moves out of the enum into Pricing.unit, governed by the new
vocab/pricing-units.json registry — same pattern as quota-metrics and the
restriction-value axes. New bases never touch the proto. No REVENUE_SHARE
(settlement is off-protocol).
RAMP-61. Introduce the (ramp.v1.vocab) custom field option (FieldOptions extension 50001, ramp/v1/vocab.proto) carrying the 15 registered metering tokens directly on Pricing.unit. Add protoc-gen-rampvocab, a buf plugin that reads the option structurally via a dynamicpb extension resolver built from the CodeGeneratorRequest descriptors and emits gen/go/vocab/pricingunits (typed constants, All, IsRegistered). Adopt protovalidate: structural field CEL on Pricing.unit (empty / lowercase-dashed / vendor:namespaced) plus message-level CEL on Pricing (PER_UNIT requires unit, FREE requires rate 0). Remove the obsolete side-car vocab/pricing-units.json — the token list now lives solely in the option. Additive, non-breaking (1.0.x): no message-shape change.
…b JSON (RAMP-61) Extend the buf-native vocabulary mechanism from the Pricing.unit pilot to every open vocabulary axis and remove all side-car JSON registries. - vocab.proto: add (ramp.v1.vocab_enum), an EnumValueOptions extension (50002), the enum-value twin of the FieldOptions (ramp.v1.vocab) (50001). An extend block targets one options message, so a second extension is required. - Quota.metric: annotate with the 8 metric tokens + a structural field CEL. - RestrictionKind: annotate RESTRICTION_KIND_FUNCTION (23), _GEOGRAPHY (the non-ISO specials *, EU, EEA only) and _USER_TYPE (6) with their token lists. - protoc-gen-rampvocab: read both options off field AND enum-value descriptors; emit one package per axis (quotametrics, functiontokens, geographytokens, usertypes), same shape as pricingunits. Special-case * -> Worldwide and all -> AllUses to avoid Go identifier collisions. - Delete vocab/ entirely (quota-metrics.json, restriction-values/*.json, README.md). The proto options are now the sole authored source. buf lint + buf generate clean; go build + go vet green.
legendko
left a comment
There was a problem hiding this comment.
Verdict
The core message model is sound (ingestion/emission symmetry, semantics split, dispute-chain integrity, comp.proto isolation, charging-structure-vs-metering-basis split all verified correct). But the branch ships an internally contradictory tree that does not match its own stated design, and it does not bring the documentation into conformance as the task requires. The single feature description (license-terms-impl.md) describes an idealized final state that the committed proto/ + cmd/ + gen/ + website/ do not collectively realize.
Two root causes explain ~70% of the findings:
- An incomplete/aborted "roll out vocabulary to all axes" step. The final commit
86c7962andlicense-terms-impl.mdclaim all five axes were converted to a proto-native vocabulary (with a(ramp.v1.vocab_enum)extension #50002 and an upgraded plugin). The committed source shows only the pilot (Pricing.unit).git show 86c7962onvocab.proto/ramp.proto/cmd/.../main.gois empty — only generated.gofiles and JSON deletions landed. Result: 4 of 5 generated vocab packages are orphans with no source in the tree, and the marquee deliverable is non-reproducible. - Documentation was not migrated to the final proto. Three website docs and the proto changelog describe earlier design states (retired pricing models, wrong field names, a deleted JSON registry, a removed
AccessRestrictionstype).
Does it break existing flow? The intended breaking changes (remove AccessRestrictions/Offer.restrictions, remove Pricing.revshare, collapse/renumber PricingModel) are documented and expected — acceptable for pre-v1. The unintended breakage is: (a) the regenerate workflow no longer reproduces gen/; (b) 6+ doc files instruct implementers to use a type that no longer exists; (c) a dual-pricing + signature-coverage ambiguity that could affect offer integrity. These are not flagged as expected anywhere and must be resolved.
Severity counts
| Severity | Count | IDs |
|---|---|---|
| Critical | 1 | C1 |
| High | 10 | H1–H10 |
| Medium | 13 | M1–M13 |
| Low | 7 | L1–L7 |
Two items (H2-signature, H3-mirror) escalate toward Critical under conditions noted inline.
CRITICAL
C1 — Vocabulary toolchain is non-reproducible; 4 of 5 generated packages are orphans
Confidence: high. Flagged by all six lenses. Resolution: DECIDE scope, then fix code (see Decisions).
The committed cmd/protoc-gen-rampvocab/main.go provably cannot produce any of the five committed gen/go/vocab/ packages. Three independent proofs:
- Header template mismatch (all 5). Plugin
emit()writes// Source vocabulary: (ramp.v1.vocab) on field <X>.(main.go:202); every committed file's header reads// Source vocabulary: on <X>.— including the pilotpricingunits.go:3. Re-runningbuf generaterewritespricingunits.goto the new header (verified) and leaves the other four untouched. constName("*")→* = "*", an invalid Go identifier, yetgeographytokens.goshipsWorldwide = "*"(main.go:250-262has no*/symbol special-casing).constName("all")→All, which collides with the emittedvar All(main.go:219), yetfunctiontokens.goshipsAllUses = "all"(special-case absent from the plugin).
Underlying source gaps: vocab.proto defines only extend FieldOptions { repeated string vocab = 50001; } — no vocab_enum/50002, despite license-terms-impl.md §"Proto-native vocabulary" claiming it (grep/git log -S across all history: not present). ramp.proto annotates only Pricing.unit (1026-1032); RestrictionKind (719-725) and Quota.metric (824-826) carry no annotations. The plugin's axisPackage map (main.go:39-41) is {"unit":"pricingunits"} with the comment "Pilot scope is Pricing.unit only."
Impact. gen/ is a primary consumer-facing deliverable pulled directly via go get / the TS path export. It cannot be regenerated from committed source (RULE: "Always commit regenerated SDKs alongside .proto changes"). The headline "single source of truth / cannot drift" guarantee is inverted for 80% of axes: their token lists exist only as generated output with no input. (Note: this is a build-integrity/maintainability Critical, not a runtime one — go build ./... and go vet ./... both pass; the orphans compile.)
HIGH
H1 — Offer JWS signature coverage of terms is unspecified; dual pricing compounds it
Confidence: high (gap is real); severity escalates to CRITICAL if signature is field-scoped. Resolution: confirm against reference-impl, then fix doc (or code).
Offer carries both top-level pricing (field 3) and terms (field 19, each LicenseTerm with its own pricing). The signature comment (ramp.proto:366-367) says the JWS is "over offer fields (offer_id, package.id, pricing, identity)" — it does not list terms(19), and it cites package.id (a CoMP concept that design-history.md says was moved to an extension), so the comment is demonstrably stale. If that 4-field enumeration is normative, the entire licensing+pricing payload is unprotected and broker-tamperable, defeating the stated anti-tamper purpose. If the JWS actually signs the whole message, it is "only" a doc bug. The JWS construction lives in the reference-implementation repo and cannot be confirmed here. This is the highest-priority open question.
H2 — protocol/licensing-terms.mdx (new spec page) describes a different protocol
Confidence: high. Resolution: rewrite doc.
The brand-new 243-line spec page (all-additions in this branch) documents an early design: retired model: PER_ACCESS / SUBSCRIPTION in every example (:142,159,178,199,224); Obligation.description (proto field is detail); license: { uri, title } (proto field is name); a "Vocabulary registry" section (:234-243) presenting the deleted vocab/*.json as the live mechanism with no mention of the proto-native vocab; user-type tokens non-commercial/government that don't exist (real set: non_profit/news_publisher/broadcaster). This is the primary human-facing page for the feature and would cause incorrect implementations.
H3 — reference/proto-ramp.mdx mirror is wrong at the wire level
Confidence: high (field numbers verified). Resolution: regenerate/rewrite the mirror. Escalates toward Critical because following it yields wire-incompatible encodings.
This hand-maintained mirror must track the proto. It does not:
LicenseTerm:semantics/licensefield numbers swapped (mirror: semantics=1, license=2; proto: license=1, semantics=2);scopes(7) andpart_label(8) missing.License: documentstitle(2)/spdx_expression(3) instead of protoid(2)/name(3)/immutable(4).Obligation: documentsdescription(3) instead ofscope_license(3)/detail(4).PricingModel: Frankenstein table —UNSPECIFIED=0added atop the oldPER_ACCESS/PER_TOKEN/PER_FETCH/SUBSCRIPTION/REVENUE_SHARE; proto isFREE/PER_UNIT/FLAT.Pricing: still listsrevshare(6) (:197).- All new enums shown off-by-one (
RestrictionKind FUNCTION=0,TermSemantics 0=ENUMERATED) — see also the security angle in H7/M-notes: teaching0=ENUMERATEDinvites reading an omitted/UNSPECIFIEDterm as authoritative. - Restriction/Quota vocabulary sources point to deleted
vocab/*.json. - Internal contradiction: "pricing MUST be present when ENUMERATED" vs the same table's "REQUIRED on every term regardless of semantics."
H4 — reference/changelog.mdx states wrong License fields and dead registry paths
Confidence: high. Resolution: fix doc.
Says License is (uri, title, spdx_expression) (proto: uri, id, name, immutable); cites vocab/restriction-values/ and vocab/quota-metrics.json (deleted). It also omits the PricingModel collapse and revshare/REVENUE_SHARE removal from the licensing entry.
H5 — proto/CHANGELOG.md mislabels the release and omits the headline feature
Confidence: high. Resolution: fix doc.
The "Unreleased" entry is headed "Additive, non-breaking (1.0.x)" while buf breaking reports four breaking removals/renumbers. It documents only the pilot Pricing.unit vocab mechanism and omits the entire Universal Licensing Core (LicenseTerm, License, Restriction, Quota, Obligation, six enums, Offer.terms, ResourceEntry.terms). It also references the never-on-main vocab/pricing-units.json. Notably, this changelog corroborates pilot scope — it disagrees with license-terms-impl.md's all-axes claim.
H6 — AccessRestrictions removal not propagated to docs (6+ files reference the deleted type)
Confidence: high. Resolution: update docs (migrate to LicenseTerm.restrictions).
Removal is intended (impl-doc). But the type still appears in: components/broker/selection-engine.mdx:154,233-261 (an entire "Stage 1: Filter by AccessRestrictions" section with Go code r *rampv1.AccessRestrictions), components/exchange/storage-model.mdx:68 (*rampv1.AccessRestrictions), components/agent-sdk/overview.mdx:339, components/exchange/request-flows.mdx:29, components/content-ingestion/catalog-compilation.mdx:59, protocol/extension-profiles.mdx:219. The branch's only edit to request-flows.mdx was unrelated (removed a "backwards compat" phrase). Implementers following these would reference a non-existent type.
H7 — Mandatory LicenseTerm invariants are prose-only; impl-doc overstates wire enforcement
Confidence: high. Resolution: add protovalidate CEL where expressible, or correct the doc.
The only buf.validate CEL in the proto is on Pricing (2 message rules + 1 field rule). Not wire-enforced: "Pricing required on every term," "REFERENCE_ONLY ⇒ License.uri non-empty," "SHARE_ALIKE ⇒ scope_license," "UNSPECIFIED enums rejected," unit-token membership, restriction/geography token validity. license-terms-impl.md frames validation as "Enforced at the RPC boundary by the Connect validate interceptor" — but the interceptor only runs the Pricing CEL; the rest is deferred to app ingest in the separate reference-impl. Any third-party implementation that trusts the impl-doc and skips app checks accepts malformed terms. protovalidate is already a dependency, so most of these are expressible on the wire.
H8 — Unvalidated, agent-fetched License.uri (SSRF / phishing surface)
Confidence: high. Resolution: add spec guidance + threat-model entry.
License.uri "MUST NOT be URL-validated" (ramp.proto:762-763) and REFERENCE_ONLY requires the agent to fetch and read it before use. No scheme allowlist, no "do not auto-fetch," no SSRF/metadata-endpoint guidance anywhere; website/.../security/threat-model.mdx received zero changes this branch. The non-validation is a deliberate accommodation for non-URL TDL schemes — the gap is the absence of consumer guidance, not the choice itself.
H9 — Restrictions fail open by default
Confidence: high. Resolution: document/decide safety posture.
Restriction.critical defaults false, making an unverifiable restriction advisory; an unknown restriction token is warned-and-ignored (robustness principle). A publisher's intended hard limit can therefore be silently downgraded — the opposite of the ext_critical (COSE crit) fail-closed posture used elsewhere. The asymmetry is undocumented.
H10 — gen/ SDK comments stale vs proto (regeneration contract violated)
Confidence: high. Resolution: regenerate + commit.
gen/go/ramp/v1/ramp.pb.go:476 and gen/ts/ramp/v1/ramp_pb.ts:4214 still cite (vocab/pricing-units.json) while the proto says (see vocab.proto). Shapes are in sync (the staleness is comment-only), but it proves committed gen/ ≠ a clean buf generate.
MEDIUM
- M1 — Dual pricing source of truth.
Offer.pricing(3) vsOffer.terms[].pricingwith no documented precedence (ramp.proto:350,451). Interacts with H1. Fix: document precedence / which binds the transaction & signature. - M2 —
optional Pricingcan't express "REQUIRED." Field isoptional Pricing pricing = 6with norequired/CEL; the schema contradicts its own "REQUIRED" comment (ramp.proto:908-914). Fix: add(buf.validate.field).requiredor message CEL. - M3 — Plugin
axisPackagehardcoded + silent skip. Axis identity authored in two layers;genMessagesilently skips any vocab-bearing field not in the map (main.go:120-133) — the direct mechanism by which the 4 axes orphaned. Bare-field-name keying is a latent collision hazard. Fix: derive package from the descriptor; error on unmapped vocab fields. - M4 —
constNamenot robust + untested. Invalid identifiers for*/empty/leading-digit; collisions foralland separator-variants; no_test.go, no diagnostic (main.go:250-262). Latent build break the moment any open axis is wired. Fix: sanitize + collision-guard + tests. - M5 — Tooling-only option in the wire package.
(ramp.v1.vocab)lives in packageramp.v1, sovocab.pb.go/E_Vocabandvocab_pb.tsship the extension descriptor into every consumer SDK (gen/go/ramp/v1/vocab.pb.go:37,58). buf convention (cf. protovalidate's own module) isolates annotations. Fix: move to a separate options package, or accept and document. - M6 — Licensing core is fully closed; asymmetric extensibility.
License/Restriction/Quota/Obligation/LicenseTermhave noext/ext_critical, while nestedPricingkeepsext(15)/ext_critical(90). Future licensing semantics are forced into core proto edits. Fix: decide deliberately and document; consider anextseam. - M7 —
cmd/outside the documented license boundary. README enumerate Apache-2.0 as "everything underproto/andgen/"; the new top-levelcmd/is under neither and not in README's structure list. Mitigated (rootLICENSEis Apache-2.0, so no legal hole) but the enumerated docs are now inaccurate, and build tooling + the protovalidate dep ship inside thego get-able module. Fix: update the license/structure docs to includecmd/. - M8 — Proto comments cite deleted
vocab/*.json.ramp.proto:721,723,795-797,821,825— the root source of the dangling references that propagate into both SDKs. Fix: code (rewrite comments to the proto-native mechanism, or to the chosen final scope). - M9 — Scope-gating semantics under-specified. Empty
scopes= public (fail-open on omission); hierarchicaldist:*matching defined only in a comment (ramp.proto:916-920). Cross-implementation scope-escalation risk. Fix: specify matching + default posture. - M10 — REFERENCE_ONLY document has no content-integrity binding. The authoritative external
uriis mutable and (per H1) likely outside the Offer signature;License.immutableis a bare bool with no hash, unlikeResourceIdentity.content_hash. Fix: add an optional content hash / digest for immutable references. - M11 — No CI gate. No
.github/workflows;amplify.ymlbuilds the website only. Nothing enforces lint/breaking/build or a "regenerate → assert clean tree" check that would have caught C1/H10. Fix: add a regenerate-and-diff CI job. - M12 — ADR-014 / RAMP-61 / RAMP-62 cited but absent;
design-history.mdnot updated. Commits and impl-doc reference an ADR that exists nowhere in the repo;design-history.md(90 lines; its stated purpose is recording wire-shaping decisions) has no licensing entry. "Faithful to ADR-014" is unverifiable and the core-vs-profile placement (M6) is undocumented. Fix: add the design-history/ADR record. - M13 — Threat model not updated.
security/threat-model.mdxgot no changes despite the new attack surface behind H1/H8/H9/M9/M10. Fix: add licensing threats.
LOW
- L1 — Generated headers misrepresent provenance (cite enum-value sources that cannot exist) and assert a "cannot drift" guarantee that is already false. (execution F9)
- L2 —
readVocabswallows marshal/unmarshal errors as "no vocab" — silent failure mode (main.go:149-158). (execution F10) - L3 —
unitregex rejects uppercase vendor namespaces (ACME:xfails;acme:Xpasses) — possibly unintended (ramp.proto:1036). (execution F6) - L4 — Cross-axis token overlap (
accesses/tokens/seats/units-manufacturedin bothpricingunitsandquotametrics) — intended; distinct billing-vs-cap axes; do not extract. (dry — verified non-issue) - L5 —
buf.yamlENUM_ZERO_VALUE_SUFFIXcomment doesn't notePricingMetering.ONLINE=0as the RAMP-native zero-value exception. (consistency F8) - L6 —
Offer.signaturecomment citespackage.id(a CoMP/extension concept) — stale wording independent of H1. - L7 — Codegen runs a local
go runplugin (executes code at generate time); protovalidate is pinned by digest inbuf.lock(good). Minor supply-chain note. (security SEC-10)
What passed (verified correct — credit)
- Message-model symmetry:
ResourceEntry.terms(13) andOffer.terms(19) are the samerepeated LicenseTerm. - Semantics split (ENUMERATED vs REFERENCE_ONLY) is coherent.
- Charging-structure vs metering-basis split is sound for the realized axis (
PricingModelenum +Pricing.unitvocabulary). - Dispute chain intact: Offer → Transaction.id → UsageReport → UsageReportResponse.report_id → DisputeRequest unaffected.
- comp.proto isolation intact:
ramp.protoimports only struct/timestamp/duration/buf.validate/vocab — no comp coupling. AccessRestrictionsfully removed fromproto/+gen/(the doc references in H6 are the only residue).- CEL is genuinely token-free — membership is delegated to generated
IsRegistered; the core DRY argument holds for the one live axis.FREE ⇒ rate==0andPER_UNIT ⇒ unit!=''are logically correct. - Generated vocab token lists match the (transient) deleted JSON exactly — zero token drift.
- Builds green:
buf build✓,buf lint✓,go build ./...✓,go vet ./...✓.buf breaking= exit 100, matching the documented intended pre-v1 break set. - protovalidate dependency is correctly used (blank import in
ramp.pb.go) and pinned inbuf.lock.
Decisions required from the maintainer (genuine ambiguities — not guessed)
- Vocabulary axis scope. Is the intended v1.0 state all five axes (then C1/H10/M3/M8 ⇒ finish the code: add
vocab_enum/50002, annotate the enum values +Quota.metric, extend the plugin withconstNamesanitizing/collision-guarding and per-axis CEL for underscore/uppercase tokens, regenerate) or pilotPricing.unitonly (then ⇒ delete the 4 orphan packages, correctlicense-terms-impl.md+ the86c7962narrative, fix proto comments)? The repo's own artifacts disagree: impl-doc + commit subject + 4 orphan packages say all-axes; proto + plugin +proto/CHANGELOG.mdsay pilot. - Offer signature coverage (H1). Does the Offer JWS sign the whole serialized message (incl.
terms(19)) or only the four enumerated fields? Determines whether H1 is a doc fix or a Critical integrity hole, and which pricing field binds the transaction (M1).
Items 1 and 2 are posed to the maintainer directly; the rest can proceed once the scope decision is made.
My thoughts on ADR-014 / RAMP-61 / RAMP-62 mentions in the commit messages: is not that critical because there are no mentions in the codebase, but for the future maybe it makes sense to omit such references to internal docs. A fix for current situation could be a rebase or a squash but that's your decision.
Regarding the proto/CHANGELOG.md - I think we should keep our idea of keeping everything under v1.0.0, besides the current changes are definitely not minor and not non-breaking (1.0.x) as currently stated.
…, doc sweep Completes the all-axes proto-native vocabulary (vocab_enum 50002; function/ geography/user-type annotated on RestrictionKind, Quota.metric and Pricing.unit on their fields) so buf generate reproduces gen/ byte-identically. Hardening: - Offer JWS documented as signing the whole canonical offer (terms + pricing) - Offer.pricing is the term's pricing (one offer per term) - Restriction.critical -> advisory (binding by default; fail closed) - License.uri_digest pins the referenced document (required when uri present) - regenerate-and-diff CI gate (.github/workflows/proto-ci.yml) Docs brought into conformance: AccessRestrictions removed, pricing models collapsed to FREE/PER_UNIT/FLAT, vocabulary now proto-native, License/Obligation field names corrected, off-by-one enums fixed in the proto mirror; threat-model gains a licensing section (T-LIC-1..4). CHANGELOG folded under v1.0.0.
|
Hi Yaroslav (@legendko) — thanks for the exceptionally thorough review. Pushed Your two maintainer decisions
Critical
High
Medium
Low
Build stays green: |
…_VALUE_SUFFIX exception
|
Follow-up on L5: addressed in |
…ment cmd/ (L3, M7) L3: the Pricing.unit and Quota.metric CEL accepted uppercase only AFTER the colon (acme:X passed, ACME:x failed). Widen the namespace class to [A-Za-z0-9._-] so vendor namespaces may be uppercase; bare registry tokens stay lowercase-dashed. M7: README now lists cmd/ in the repo structure and includes it in the Apache-2.0 code boundary.
|
Follow-up on L3 and M7 — both addressed:
That closes the review. Remaining items are the consciously-deferred maintainability nice-to-haves (M3 plugin error-on-unmapped-axis, M4 |
…ons, decode failures (M3, M4, L2) M3: key axis->package maps by full name (no short-name collisions) and ERROR when a vocab-bearing field/enum value is unmapped, instead of silently emitting nothing. M4: constName validates its output is a unique exported identifier; invalid idents (e.g. leading digit), reserved-name shadows (All/IsRegistered), and token->ident collisions now error with a clear message. Adds unit tests. L2: readVocab distinguishes 'no vocab option' from a decode failure and propagates the latter instead of masking it as an empty axis. Generated output is byte-identical for the current registries.
|
Follow-up on M3 / M4 / L2 — done in
That clears every actionable item in the review. Thanks again, @legendko. |
legendko
left a comment
There was a problem hiding this comment.
Thanks for addressing the above mentioned issues. We are almost there. The re-review found a few things that should be addressed before merge.
Verdict
The first review is substantially resolved. Of 31 original findings: 22 RESOLVED, 5 acceptably DEFERRED, 3 PARTIAL, 1 (H6) resolved-at-the-doc-layer-but-the-underlying-model-change-is-half-applied. No original finding is unaddressed.
However, the re-review surfaces two new HIGH issues that should be fixed before merge — one a regression introduced by the sweep, one a half-applied design change — plus six MEDIUM and several LOW items (a mix of newly-introduced and pre-existing-but-surfaced).
Does it break existing flow? (the primary-review bar)
- The intended breaks (remove
AccessRestrictions/Offer.restrictions/revshare; collapsePricingModel) remain documented and expected. ✓ - The
critical→advisoryinversion is a deliberate, well-documented in-branch behavioral change (fail-open → fail-closed). ✓ Acceptable. - The H6 discovery reframe ("Exchange no longer filters by requester attributes; the agent self-selects") is a real behavioral change that is only half-applied — it is explained in the new
Restrictioncomment, but the proto still ships the old filtering enums and two docs still describe the old model (N1). This is exactly the "breaks existing flow without fully reconciling it" case the primary review guards against. Must be completed or the residue reconciled.
NEW issues (introduced or surfaced by the re-review)
HIGH
-
N1 — H6 discovery reframe is half-applied (internal proto contradiction). [orchestrator-verified]
The newRestrictioncomment (ramp.proto:854-863) says "the Exchange does NOT filter terms by matching the requester's self-declared attributes (user_type / geography / intended_use); … the AGENT self-selects." But the same proto still ships the old filtering model:OfferAbsenceReason.{FUNCTION_PROHIBITED=3, GEO_RESTRICTED=4, USER_CATEGORY_PROHIBITED=5}(:256-261) andDenialReason.{FUNCTION_PROHIBITED=6, GEO_RESTRICTED=7}(:1664-1665), anddiscovery-paths.mdx:252("content blocked for your use case") +ext-c2pa.mdx:167("will not receive offers…") still describe attribute filtering. As written, the offer/denial enums and the Restriction model are mutually exclusive and nothing reconciles them. HIGH (not Critical: pre-v1, no consumers, dispute chain intact). Resolution: DECIDE — remove/repurpose those enums to reconcile with self-select, or narrow the Restriction claim. (Open question 1.) -
N2 — REFERENCE_ONLY semantics regressed in the canonical mirror. [orchestrator-verified]
The sweep rewroteproto-ramp.mdx:791from the correct "machine fields are informational; the Exchange MUST NOT auto-enforce them" to "machinerestrictions/quotas/obligationsMUST be absent" — which contradictsramp.proto:710-714andlicensing-terms.mdx:33(both say informational, present-but-not-auto-enforced). A right→wrong regression in the authoritative reference page, introduced by this branch (git diff 86c7962..HEADconfirms). Fix (doc): restore "informational; not auto-enforced."
MEDIUM
- N3 —
Requester.intended_useorphaned by the reframe. The new model namesintended_use(anduser_type/geography) as things the Exchange does NOT consume, butRequester.intended_use(ramp.proto:1256-1258) keeps its old comment with no statement of what (if anything) now reads it. Tied to N1. (Also a pre-existing underscore-vs-hyphen token-form mismatch vs FUNCTION tokens.) Fix: clarify role (agent self-selection input? advisory/telemetry? vestigial) or remove. - N4 —
uri_digesthas no hash-algorithm constraint. Free-formmethod:hexdigestwith no CEL acceptsmd5:/sha1:, making the new swap-protection collision-forgeable; and digest-present is prose/ingest-only though it underpins a new integrity guarantee. Fix: field-CEL strong-hash allowlist (sha256/sha384/sha512) + spec wording; consider wire-requiring it whenuriis set. - N5 — CI drift gate misses untracked files.
git diff --exit-code(.github/workflows/proto-ci.yml:44) does not see new generated files, so a future axis whosegen/go/vocab/<new>/is generated-but-not-committed passes green — the exact C1 class the gate exists to seal (the author's roadmap anticipates a "6th axis"). Fix:git add -A && git diff --cached --exit-code, ortest -z "$(git status --porcelain)". - N6 — Homepage code samples use retired
PER_ACCESS. [orchestrator-caught; missed by agents]website/src/pages/index.astro:1407,1707showmodel: PER_ACCESS(no longer exists). Likely missed because the sweep scoped tocontent/docs/, notpages/. Most-visible page. Fix (doc): PER_UNIT+unit or FLAT. - N7 — Offer replay:
expires_atis unsigned with no signed time anchor. [orchestrator-verified; PRE-EXISTING, not a regression]expires_atis excluded from the JWS (:375) and there is no signedissued_at/nonceinOffer, so a signed offer's validity window is not integrity-protected;DENIAL_REASON_OFFER_EXPIRED=9exists but enforcement lives in the reference-impl (not in this repo). The old signature comment also excludedexpires_at, so this branch did not introduce it — but H1's "whole offer is now signed" framing makesexpires_atthe conspicuous unsigned exception worth hardening. Severity MEDIUM (security agent rated HIGH; downgraded — pre-existing + enforcement out-of-repo). Fix: signedissued_at+ max-age check, or signexpires_at, or document the replay bound. (Open question 2.) - N8 — Open string axes still unbounded (original SEC-2, not addressed). Vendor-namespace CEL post-colon is
.+(whitespace/control chars), andRestriction.permitted/prohibited+Quota.metriccarry no CEL or length bound → injection / log-forging / oversize-payload surface into billing/logs/display. No T-LIC entry. Pre-existing; surfaced again. Fix: length caps + charset tightening.
LOW
- N9 —
ramp.proto:687-701worked-example comments use retiredPER_ACCESS(canonical proto contradicts its own enum; comment-only, not ingen/). - N10 —
ramp.proto:785RESTRICTION_KIND_OTHERcomment cites a non-existentRestriction.descriptionfield (Restriction has no free-text field; propagates to both SDKs). - N11 —
constNameSpecialspecial-case values bypassisExportedIdent/reservedIdentsvalidation; a future bad special-case would emit broken Go silently. - N12 — T-LIC-1 SSRF guidance is not cross-linked from the
License.uriproto comment orlicensing-terms.mdx; a proto-only implementer sees "MUST NOT URL-validate / must fetch" without the countermeasures. - N13 —
CLAUDE.md"Licensing split" still omitscmd/(README was fixed). - N14 — M6 closed-core rationale undocumented;
design-history.mdstill 90 lines with no licensing entry, yetproto/CHANGELOG.mdlinks to it "for the reasoning." One in-repo paragraph closes both M6 and M12. - N15 —
exchange-manifest.mdxpricing_models_supportedadvertises retired strings (revenue_share/subscription/per_token). Pre-existing (identical onorigin/main), not a regression, but now visibly stale. - N16 — "geography validated structurally (two-letter uppercase)" overstates the wire:
Restriction.permitted/prohibitedcarry no CEL (consistent with H7's prose-only posture).
What the orchestrator verified directly
buf lint=0,buf build=0;go build ./...=0,go vet ./...=0,go test ./cmd/...=ok.- C1 reproducibility: a clean
buf generateyields zero git diff —gen/(Go+TS SDKs + all 5 vocab packages) reproduces byte-identically. buf breakingvs main = the same documented pre-v1 set (thecritical→advisoryrename is invisible to main —Restrictionis branch-new).vocab.protodefines both extensions; all 5 axes annotated;License.uri_digest(5) andRestriction.advisory(4) present; Offer signature comment rewritten; CI workflow present.- Residual-token sweep across the live tree:
spdx_expression/non-commercial/Restriction-critical/vocab/*.json(in proto+gen) all gone;AccessRestrictionsonly in the 2 changelogs (correct). - N1 and N2 contradictions confirmed against
ramp.protoline-by-line; N7's unsigned-expires_at+ no-issued_atconfirmed.
Open questions / decisions for the maintainer
- (N1 — drives the only HIGH design item) Are
OfferAbsenceReason.{FUNCTION_PROHIBITED, GEO_RESTRICTED, USER_CATEGORY_PROHIBITED}andDenialReason.{FUNCTION_PROHIBITED, GEO_RESTRICTED}meant to be removed (full commit to "agent self-selects"), or kept as reconcile/accept-phase reasons? AGEO_RESTRICTEDdiscovery-phaseOfferAbsenceReasoncannot be salvaged by "reconcile-only" — it directly re-asserts Exchange-side discovery filtering. And what now consumesRequester.intended_use/user_type/geography(N3)? - (N7) Intended offer-replay bound — should
expires_atbe signed or anchored by a signedissued_at, and isDENIAL_REASON_OFFER_EXPIREDwired in the reference-impl? - (N4) Is a strong-hash (sha256+) mandatory for
uri_digest? - (M9) Multi-level scope-wildcard semantics (
dist:*vsdist:US:CA). - (N8) Is the open-axis injection/length surface (original SEC-2) accepted, or to be bounded?
Bottom line: the Critical is genuinely closed and the bulk of the review landed well. Before merge, address N1 (reconcile the half-applied discovery reframe) and N2 (REFERENCE_ONLY mirror regression); ideally also the MEDIUMs (N4 hash-algo, N5 CI gap, N6 homepage, plus the pre-existing N7/N8 hardening). The LOWs are cleanup.
…pires_at, bound open axes - Reframe restrictions as agent-self-selected with an OPTIONAL Exchange/Broker convenience pre-filter; keep OfferAbsenceReason/DenialReason attribute reasons but make them coherent (convenience signal, not enforcement). Clarify Requester.intended_use as an advisory filter hint, not an entitlement. - Correct REFERENCE_ONLY semantics: machine restrictions/quotas/obligations are optional, but when present must be accurate (no contradiction with the referenced document) and are enforced like ENUMERATED. Drops the wrong "must be absent" / "informational, not enforced" framings. - Sign Offer.expires_at so the validity window is integrity-protected against replay (only signature/signature_algorithm now excluded). - uri_digest: structural CEL allowlist — sha256/sha384/sha512 with matching hex length; reject forgeable md5/sha1. - Bound the open string axes: length caps + charset tightening on Restriction.permitted/prohibited, Quota.metric, and Pricing.unit (vendor-namespace post-colon no longer ".+"). - CI drift gate stages files first so NEW untracked generated output is caught. - Plugin: validate constNameSpecial values through the exported-ident/reserved guards so a bad special-case can't emit broken Go silently. - Docs: restore REFERENCE_ONLY mirror, fix homepage + worked-example PER_ACCESS, retire stale manifest pricing strings, soften geography "validated structurally", cross-link SSRF guidance, add the design-history licensing entry (closed-core rationale). - Regenerate gen/ (descriptor-embedded CEL + comments).
…ed licensing core Pricing inherited an open google.protobuf.Struct ext (15) + ext_critical (90) from the CoMP/COSE lineage. An untyped blob inside a signed, cross-exchange- comparable pricing object defeats the comparability unit_cost exists for and re-opens the unbounded-payload surface the vocabulary axes were closed to avoid. Removed both so the entire licensing core (License/Restriction/Quota/Obligation/ LicenseTerm/Pricing) is uniformly closed; new commercial dimensions arrive as typed fields or a vocab axis. The general ext/ext_critical mechanism stays on transport/discovery messages where contextual, ignorable metadata is fine. - proto: remove Pricing.ext / Pricing.ext_critical - regenerate gen/ (Go + TS) - docs: drop the Pricing ext row from the proto-ramp mirror; rewrite the design-history closed-core entry to "no ext anywhere in the licensing core" - homepage: correct stale "12 pricing models" -> per unit / flat / free
…as a current-state snapshot
Multi-agent read-through of the proto source for comments that narrate how the
schema got here rather than what it is now. Removed/reworded 14 spots:
- "Replaces CoMP AISystem" (Requester field + message), "Replaces ContentQuality"
(attestations), "(replaces eCPT)" (unit_cost)
- ATTRIBUTION/CONTRIBUTION "replace the retired PRICING_MODEL_*" narration
(ObligationKind + Obligation)
- Pricing field-6 "formerly revshare, retired / Pre-v1: not reserved" parenthetical
- PricingModel "Pre-v1: renumbered cleanly, nothing reserved"
- ResourceAttestation "claims-schema migrated to ext (see CHANGELOG)" — reworded
to present tense (and dropped the dangling CHANGELOG ref)
- DiscoveryMethod "v1 extension point for future..." framing
- "Relaxing the discovery anchor is intentionally deferred: revisit..." note
- vocab.proto protoc-evolution rationale ("modern protoc supports...") trimmed
The reasoning behind these choices lives in docs/design-history.md, which is the
file for it. Regenerated gen/ (comments propagate to the SDKs). No wire change.
…riction vocabulary across request/term/reason
Requester now carries identity + entitlements only (id, domain, type, name,
license_id, scopes, delegation). What is being asked for (uris) and the limits
the ask operates within (acceptable_restrictions) move to ResourceQuery and
RAMPRequest, where they belong. New AcceptableRestriction{axis, values} states
selection limits in the same open RestrictionKind vocabulary the terms use.
Reason enums unified onto the licensing core: the pre-licensing attribute fossils
(OfferAbsenceReason/DenialReason FUNCTION_PROHIBITED / GEO_RESTRICTED /
USER_CATEGORY_PROHIBITED) collapse to RESTRICTION_FILTERED and
RESTRICTION_NOT_SATISFIED, each carrying repeated RestrictionKind
(OfferGroup.restriction_filters, TransactionResult/Item.restriction_mismatches).
Request, term, and reason now all speak one vocabulary.
SCOPE_INSUFFICIENT reworded to cover subscription/scope-gated access generally,
not only enterprise deployments.
- proto: reshape Requester / ResourceQuery / RAMPRequest; add AcceptableRestriction;
collapse + renumber reason enums; add RestrictionKind carriers
- regenerate gen/ (Go + TS)
- docs: mirror + 14 walkthrough/narrative pages swept to the new shape; reason
tables and lists updated
Multi-agent consistency review of the Requester/ResourceQuery reshape surfaced 22 issues; all fixed: Reshape-introduced: - The doc sweep had placed uris/acceptable_restrictions into ExecuteTransaction (TransactionRequest) examples — discovery-phase fields that do not belong in a transaction commitment. Stripped from 8 transaction blocks across 6 walkthroughs (academic, due-diligence, medical-imaging, credit-report, eu-regulation, scenario) plus transaction-flow and walkthrough-v1. - Fixed AcceptableRestriction.values / Usage tokens that used enum-style FUNCTION_AI_INPUT / SUB_FUNCTION_RAG instead of the real vocabulary (ai-input / ai_input / rag). - Dropped the stale `uris` from the Requester signature tuple in 4 more docs (now (id, domain, scopes)). - proto: DenialReason comment referenced a non-existent "TransactionResult" — corrected to TransactionResponse/TransactionResultItem. Pre-existing mirror drift (proto-ramp.mdx) the review also caught: - Offer field 2 package->title, field 9 exchange_signature->signature, +ext_critical - Delegation: max_spend->max_spend_cents (int64), token string->bytes, +max_accesses/quota_period/issuer/ext_critical - Usage: function/subfn typed as Function/SubFunction -> repeated string Regenerated gen/; buf lint, go build/vet, website build all green.
…eaders (RFC 9421 multi-sig); drop IntermediaryHop Request authentication is RFC 9421 HTTP Message Signatures (headers), never message fields. Multi-hop forwarding is the same primitive: a stack of labeled RFC 9421 signatures, each covering the request plus the prior hop's signature, so the ordered set of signatures IS the forwarding chain (tamper-evident, order-bound). The in-message hop chain is therefore removed. Proto: - Remove message IntermediaryHop and ResourceQuery.intermediaries (field 5); document the header multi-sig model on ResourceQuery. - Keep RequestConstraints.max_hops and WellKnownManifest.max_intermediary_hops (now counted as signatures); reworded. - Regenerate gen/ (Go + TS). Docs (proto is source of truth): - Strip request-side signature *fields* from all JSON examples and prose: requester.signature/signature_algorithm, agent_signature(_algorithm), offer_signature_algorithm, broker_signature(_algorithm). Reframe to RFC 9421 HTTP Message Signatures in headers. Keep the real out-of-band artifacts: Offer JWS (signature/signature_algorithm), ResourceAttestation.signature, and the echoed offer_signature. - Replace IntermediaryHop tables / intermediaries[] examples with the header signature-stack model across walkthroughs, components, authentication, threat-model, production-architecture, changelog, mirror, design-history. - authentication.mdx "Signature Verification" rewritten to the RFC 9421 model. - Fixed a stale REFERENCE_ONLY changelog line (machine fields optional but, when present, accurate and enforced). buf lint, regen idempotent, go build/vet, website build all green.
…ation table + REFERENCE_ONLY changelog The core Offer has no `package` field (field 2 is `title`); IAB CoMP Package/Scope/Retrieval metadata lives as flat comp.* keys in Offer.ext per the ramp-comp-v1 profile. Replaced every top-level `package` object in Offer JSON examples across 11 walkthrough/profile docs with Offer.title + the documented comp.* ext keys (comp.package_id / seller / citation_required / content_types / retrieval_type), reframing prose that called Package a core Offer field. Also: - authentication.mdx Delegation table → max_spend_cents (int64), token (bytes), token_format "jwt" default, + issuer (match proto). - changelog REFERENCE_ONLY wording → machine fields optional but, when present, accurate and enforced. Website build green; docs-only (no proto/gen change).
…ing, unify scope matching, finish RFC 9421 auth model Resolve the three security-documentation contradictions the latest review surfaced (all docs/comments; no wire change): - Holder binding: add the mandatory, non-skippable verification step — the RFC 9421 request-signing key MUST equal the token's holder/sealed key — to authentication.mdx and for-exchange-operators.mdx, and de-bearer the proto Delegation comment + threat-model T-DEL-1 (theft neutralized by binding; scope/time/spend caps are defense-in-depth). Name JWT proof-of-possession (cnf); declare the full JWT verification path deferred past v1 with no degraded mode (Biscuit v3 remains the default and only fully specified path). - Scope matching: make the M9 segment-wise string rule the single normative algorithm protocol-wide; threat-model now cites it and frames Biscuit Datalog as a conformant implementation that must produce identical results. Propagate the rule to LicenseTerm.scopes and add the narrower-than-required case. - Request auth: rewrite the residual in-message tuple-signature wording in the operator/architecture/broker docs to RFC 9421 over @method/@target-uri/ content-digest. Value-level doc drift and hardening: - Complete the DenialReason table in event-types.mdx; broaden the DELEGATION_INVALID description everywhere. - Point the duplicated delegation-claim vocab in proto-ramp.mdx at the single source; make its Delegation intro token-format-agnostic. - Reserve the ramp_ claim-name prefix; bound scopes/uris with max_items. - Rename code-sample exchange_signature to the real wire fields (signature on an Offer, offer_signature when echoed), keeping the term as a documented alias in prose; strip the biscuit-v3: prefix from opaque token samples; fix the stale CoMP Go path in multi-tenant.mdx. - Record this round's decisions in design-history and the proto changelog. Doc-conformance gate: also scan proto/ramp and assert positive facts (every DenialReason value appears in the event-types table; registered ramp_ claims appear in the auth spec) — a denylist alone cannot catch dropped values. Stop tracking local review scratch (.claude/) and gitignore it.
… to the ext, not a pricing model PricingModel covers only the charging structures an Exchange can quote, sign, and compare at transaction time (FREE / PER_UNIT / FLAT). Revenue share has no transaction-time price — the rate and its settlement are an off-protocol agreement — so adding it as a pricing model would either be a rate-less label or would pull commercial terms into the signed, comparable Pricing. - design-history: record the decision and the reasoning. - licensing-terms: add "Revenue-share arrangements" — express it as a FREE term gated by an agreement scope plus a reporting Obligation (same shape as a subscription); agent self-selects between a public per-unit term and the scope-gated revshare term. - ext-comp: document comp.license[].revshare as carried verbatim in the ext and deliberately not mapped to a pricing model (parity by mapping, not duplication). - doc gate: stop denylisting the bare word "revshare" (it is now a live CoMP ext identifier and scope prefix); the retired pricing model stays guarded via the enum-constant patterns.
|
@legendko — round 4 addressed. You were right that this round's findings were a different animal from the earlier stale-example drift — these were the security invariants contradicting each other, and the doc gate sailing past all three is exactly the "necessary but not sufficient" proof you predicted. So I took your three gate recommendations as well, not just the findings. All on The three HIGH — the security-doc unitI treated
MEDIUM / value-level driftR4-4 LOW / hygieneR4-14 stripped the Your open questions
On your gate recommendationsAll three taken:
I left the compile-the-samples half out again — fragments don't compile standalone and the maintenance cost stays high — but the positive-fact assertions close part of the gap it would have covered, and a final grep sweep caught the last stray Go field ( One design call landed in the same branch — revenue shareNot from your review, but adjacent to the CoMP boundary and the So it's expressed with existing primitives instead: a Green
|
legendko
left a comment
There was a problem hiding this comment.
We did a great job and I think we are almost done.
Verdict
This is the cleanest round of the five. No new HIGH or CRITICAL. No new breaking changes (buf breaking identical to round 4; the +46 proto lines are non-breaking comments + max_items options). Builds green, buf generate reproduces zero-diff, the extended doc gate runs clean.
The remaining issues are 3 MEDIUM + 8 LOW, all the familiar pattern — the central fix is correct but a few peripheral docs lag. The most consequential, R5-1, is that the round's headline security step (mandatory holder-binding verification) is in authentication.mdx but missing from the ~5 walkthrough/component docs that depict the verify flow.
NEW issues
MEDIUM
- R5-1 — Holder-binding verification step not propagated to the verify-flow depictions. [orchestrator-verified] The R4-1 fix is airtight in
authentication.mdx, but the docs that show an Exchange verifying a delegation still teach chain-only verification with no holder-binding check:request-flows.mdx:104(step "2c… verifies the Biscuit token chain against the principal's published key" — not touched this round),exchange/overview.mdx:125("verifies the delegation chain"),walkthrough-v1.mdx:363,walkthrough-academic.mdx:193/833. An implementer following these omits exactly the step that makes the anti-theft guarantee real — fail-open-by-omission (the normative spec even says omitting it "reduces the token to a bearer credential"). Fix (doc): add the holder-binding step (or a one-line pointer toauthentication.mdx#verificationstep 3) wherever delegation verification is depicted. - R5-2 — Holder binding under-specified for the brokered multi-signature case. [security] With two RFC 9421 signatures (agent + broker) and the agent's Biscuit passing through, the spec doesn't state which signature the binding check uses; the hop-survival paragraph (
authentication.mdx:220) describes a re-attenuation/re-seal model that sits in mild tension with the default pass-through and withbroker/overview.mdx:108("never modifies delegation") vs:181("MAY attenuate"). Intent is inferable but not stated at the binding step. Fix (doc): state, at the binding step, which request signature must match the bound key in the brokered/pass-through vs re-attenuated cases. - R5-3 — R4-12 PARTIAL: two compile-broken CoMP stragglers. [orchestrator-found + consistency + security]
req.Aisystem.Aisysuse.Uri(removed CoMP path; should bereq.Uris/req.Msg.Uris) survives inbroker/overview.mdx:75andbroker/selection-engine.mdx:42— the prose/Go siblings in the same two files were fixed this round; these two were missed. Pre-existing, won't compile, the recurring half-applied miss. Tree-wide sweep confirms these are the only two. Fix (doc).
LOW
- R5-4 — Mirror scope-rule drift. [orchestrator-verified]
proto-ramp.mdx:714still callsLicenseTerm.scopes"hierarchical (dist:*coversdist:US)" — the terse framing the proto comment dropped this round; "hierarchical" is the wording T-LIC-3 warns against (the example is correct under the new rule; the word isn't). The mirror states the full segment-wise rule nowhere. Fix (doc): replace "hierarchical" with the segment-wise rule or a pointer. - R5-5 —
for-ai-agents.mdx:182DELEGATION_INVALIDdescription still narrower than the broadened normative tables (an R4-4 sibling the sweep missed). Fix (doc). - R5-6 — Revshare prose imprecision. The off-protocol-revshare note says "a reporting
Obligation," but there is no reportingObligationKind; reporting is the separate offer-levelReportingObligationmessage. Design is fine; the type name is imprecise. Fix (doc). - R5-7 —
revshare:scope not added to theauthentication.mdxscope-example table (parity with the documentedsubscription:example). Fix (doc). - R5-8 — Doc-gate
ramp_*positive-fact assertion is a hardcoded 3-item list (ramp_max_spend_cents/ramp_max_accesses/ramp_quota_period), not self-extending like theDenialReasonloop — a maintenance dependency that will silently miss a future registered claim. Fix (script): derive the list, or add a comment tying it to the registry. - R5-9 — Doc gate still can't catch the
Aisystemclass (R5-3). A wholesaleAisystemdenylist would false-positive on legitimate CoMP JSON keys, but a narrowreq\.Aisystem/\.Aisysuse\.Go-path pattern would catch both stragglers with zero false-positive risk. Fix (script, optional). - R5-10 —
enterprise.mdx:184frames caps as theft protection without mentioning holder binding; borderline (it's selling attenuation and references the "compromised key" residual case). Acceptable / optional. - R5-11 — Transaction-log
offer_signatureevent field is a mild misnomer (it signs the chain hash, not the offer); non-security. Optional.
Meta-finding (five rounds)
The proto / generated code / build tooling have been correct and reproducible throughout, and this round the documentation has substantially caught up: the security-doc contradictions are reconciled as a unit, the doc gate now covers proto/ramp + positive facts (both round-4 recommendations), and the AI-review scratch is gitignored. The only persistent residue is peripheral-doc lag — the round's central fix is right, but a handful of walkthrough/component/mirror files still depict the older flow (R5-1/R5-3/R5-4/R5-5). Two structural gaps remain in the gate-and-docs process: (a) it still doesn't compile the doc code samples (why the Aisystem Go stragglers and the chain-only verify depictions slip through), and (b) the positive-fact list is partly hardcoded. The recommendation stands: compile the doc code-samples against gen/ — it is the one check that would mechanically catch R5-1's chain-only verify code and R5-3's removed-field references.
Open questions for the maintainer
- (R5-2) In the brokered two-signature case, which RFC 9421 signature must match the token's bound key — and is the default broker behavior pass-through or re-attenuation? Reconcile
broker/overview.mdx:108vs:181. - (R5-3) Broker examples:
req.Uris(bare message) vsreq.Msg.Uris(Connect wrapper)? Either wayAisysuse.Uriis wrong. - (R5-4) Should
proto-ramp.mdxinline the scope rule or link toauthentication.mdx#scope-matching?
Bottom line: the branch is in strong shape — wire contract sound and reproducible, all prior findings resolved, the security-doc unit genuinely reconciled, the revshare decision well-reasoned and documented, the doc gate hardened per recommendation. No blocker remains. Before declaring R4-1 airtight end-to-end, propagate the holder-binding step to the verify-flow docs (R5-1) and disambiguate the brokered case (R5-2); fix the two Aisystem stragglers (R5-3); the rest are LOW polish.
…n; Biscuit optional Flip the delegation token model to a holder-bound JWT by default and rewrite the delegation story across the spec to match. The property RAMP depends on — a leaked token is not bearer-usable — is proof-of-possession, not anything specific to Biscuit, and a chain of cnf-bound JWTs delivers it with one fewer new technology for adopters. Model: - token_format defaults to "jwt". The grant is bound to a key via the RFC 7800 cnf claim (cnf.jkt = RFC 7638 thumbprint); the holder proves possession with the RFC 9421 request signature (verifier checks thumbprint(request key) == cnf.jkt). - Delegation is a chain of cnf-linked JWTs (owner -> principal -> agent): each child is signed by the key its parent named in cnf and narrows scope; the chain-linkage invariant rejects any token signed by a key not named upstream. Verified offline under the issuer's key alone; intermediate keys ride in the JOSE header jwk. - "biscuit-v3" stays a permitted OPTIONAL alternative for deep multi-hop offline attenuation; the Delegation message shape is unchanged. Updated: proto comments (Delegation, token_format, LicenseTerm scope-gating) and regenerated SDKs; authentication.mdx (full delegation section rewrite, JWT libraries, examples); threat-model (T-DEL theft/escalation, scope-gating); proto-ramp; licensing-terms; broker/exchange/architecture component docs; the walkthroughs and getting-started guides; design-history and changelogs. Pre-v1, no shipped consumers. Builds green: buf lint, regen idempotent, go build/vet/test, website build, doc-conformance gate.
… fix stragglers, harden gate - R5-1: add the holder-binding step (request key hashes to cnf.jkt) to the delegation verify-flow depictions that taught chain-only (exchange/overview, walkthrough-academic). - R5-2: state which signature binds in a brokered request — default broker pass-through, the AGENT remains the holder and the agent's signature must match cnf.jkt; reconcile broker/overview so narrowing is an opt-in only when the agent delegated to the broker (broker becomes the terminal holder). - R5-3: fix the two remaining removed-CoMP Go paths (req.Aisystem.Aisysuse.Uri -> req.Uris) in broker/overview and broker/selection-engine. - R5-4: proto-ramp scopes — drop "hierarchical", point to the segment-wise rule. - R5-5: broaden DELEGATION_INVALID description in for-ai-agents. - R5-6: revshare prose uses ReportingObligation (the offer-level message), not a reporting ObligationKind. - R5-7: add the revshare: scope to the authentication scope-example table. - R5-8: make the doc-gate ramp_ claim check self-extending — derive the registered claims from the auth registry and assert each maps to a Delegation proto field, no hardcoded list. - R5-9: add narrow req.Aisystem / .Aisysuse. Go-path patterns to the doc gate. Builds green: doc-conformance gate, website build.
…t it surfaces
Adds a Go conformance suite (conformance/) that performs the value- and
semantic-level checks the removed-identifier denylist structurally cannot:
- TestProtovalidateConstraints evaluates the embedded protovalidate CEL against
valid/invalid instances (uri_digest strong-hash, Pricing PER_UNIT/FREE,
unit format, charset + max_items). Until now nothing in the toolchain ever
ran the constraints, so a wrong CEL shipped green.
- Doc-example checks over website/src: every Pricing unit / consumed_unit is a
registered token, signature_algorithm is "EdDSA", and every LicenseTerm
example carries the required semantics discriminator. Wired into proto-ci via
`go test ./...`.
The harness surfaced the review findings (and two the manual pass missed) which
are fixed here:
- signature_algorithm "ed25519" -> "EdDSA" across the walkthroughs.
- unregistered Pricing units: articles->items, reports->records,
studies->records (matches the offer's unit), and seconds is now a registered
unit (added to the Pricing.unit vocab — per-second is the right media basis).
- ext-news max_display_words (deleted AccessRestrictions field) -> a Quota
{metric:"display-words"}.
- 22 LicenseTerm examples gained the semantics discriminator.
- ai_input/ai_train/ai_index -> dashed ai-input/ai-train/ai-index (the
registered vocabulary; CoMP's uppercase AI_INPUT is unaffected).
- DELEGATION_INVALID description unified across the tables; dropped the residual
"attenuation" Biscuit-ism.
Gate hardening: scope the ramp_<field> delegation-claim check to the Delegation
message body (was matching any field in the proto); denylist the deleted/renamed
identifiers above.
|
@legendko — ran a multi-agent internal review of the branch (consistency / DRY / layering / testing lenses) before handing back. The wire contract came out clean — The harness (the pattern-level fix)New
Patterns it surfaced (and fixes)PAT-01 — drift lives in the denylist's blind spot. Example payloads that would be rejected at ingest but pass the grep gate: PAT-02 — normative text copied, not single-sourced, already drifting. PAT-03 — new logic / guards unverified. The CEL constraints (now evaluated by the harness) and a self-check bug: the One decision worth flagging
All green: |
…d8y64, fc65j) Move cross-field presence rules into protovalidate CEL on the canonical proto so the shared SDK enforces them in every language (Go/TS/Python), not only in the Go Exchange's hand-rolled validator: - License: uri_digest is required whenever uri is set — any semantics. An undigested uri can be swapped after the offer is signed. (d8y64) - LicenseTerm: REFERENCE_ONLY terms must carry a license with a non-empty uri. (fc65j) - LicenseTerm: pricing is required on every term — (buf.validate.field).required on pricing. (fc65j) Conformance suite gains valid/invalid cases for all three; regenerated SDKs. Follow-up (agentic-content-access repo, separate): bump the proto go.mod pin, regenerate TS/Python, and slim internal/licenseterm.Validate to drop the now- duplicated presence checks (keep registry membership, canonicalization, and lint-warnings, which CEL cannot express).
|
@legendko — consolidating what's landed since your round-5 review, because one change is big enough that your prior mental model needs updating. The headline: the delegation token is now a JWT, not a BiscuitYou reviewed five rounds against a Biscuit-default model; we've since flipped it. The delegation token defaults to a holder-bound JWT ( Why: the property we actually depend on — a leaked token isn't bearer-usable — is proof-of-possession, not anything Biscuit-specific. A chain of (One consequence worth flagging since you've tracked the signature surface: there are now two algorithm names for the one Ed25519 key — Round 5 (R5-1 … R5-11): resolvedHolder-binding step propagated to every verify-flow doc (R5-1); brokered case disambiguated — default pass-through, the agent's signature binds (R5-2); the two Your standing recommendation, built: a doc-sample validation harnessThe thing you've asked for every round — compile/validate the doc samples against the contract — now exists as a Go conformance suite in CI (
It earned its keep immediately — caught a batch of example/vocab drift the denylist gate structurally can't see (unregistered units, missing New: presence invariants moved into protovalidate CELCross-field presence rules that were hand-rolled in the Go validator are now CEL on the proto, so the shared SDK enforces them in Go/TS/Python, not only at the Go Exchange:
All green: |
Port the remaining CEL-expressible coherence rules from the Go licenseterm validator into protovalidate CEL on the canonical proto, so the shared SDK enforces them in Go/TS/Python: - Restriction: permitted and prohibited must be disjoint (a token cannot be both permitted and prohibited on the same axis). - LicenseTerm: at most one Restriction per kind (same-kind restrictions are AND-combined, so duplicates are an authoring error). - Quota.limit >= 1 (a zero quota grants nothing). - Obligation: SHARE_ALIKE requires scope_license. Conformance suite gains valid/invalid cases for all four; regenerated SDKs. Follow-up (agentic-content-access): bump the proto go.mod pin, regenerate, and remove these now-duplicated checks from internal/licenseterm.Validate, leaving only vocab membership/warnings, canonicalization, and business rules.
…_digest)
A SHARE_ALIKE scope_license can be a URI, and a referenced-license URI needs the
same swap-protection digest as any other license reference. Rather than guess
URI-vs-SPDX from a bare string, model scope_license as a `License`: the SPDX
short-id goes in `id`, the URI in `uri`, and the existing d8y64 rule
(uri present ⇒ uri_digest present) applies to it automatically — a scope_license
URI without a digest is now rejected.
- Obligation.scope_license: string → License.
- SHARE_ALIKE CEL: requires scope_license to identify a license (id or uri).
- Conformance: SPDX-id ok, uri+digest ok, uri-without-digest rejected, absent
rejected. Docs updated (scope_license shown as { id: ... }).
Breaking (field type), pre-v1, no shipped consumers.
|
@legendko — update since the last note: the license-term coherence rules are now in protovalidate CEL as well, plus one refinement and an accuracy correction on the SDK-enforcement claim I made earlier. Coherence rules → CEL (
|
| Language | Generated | Validation wired + tested |
|---|---|---|
| Go | ✅ (messages + Connect + vocab) | ✅ protovalidate-go, run over valid/invalid instances in conformance/ |
| TypeScript | ✅ (protobuf-es) | protovalidate-es wired/tested here |
| Python | ❌ not generated | ❌ |
So "enforced everywhere" is true at the rule-definition level (any protovalidate runtime would apply them), but generated-and-proven only for Go today. Closing that — generating a Python SDK and wiring a TS validation harness that mirrors the Go conformance suite — is a tracked follow-up, not done here.
There was a problem hiding this comment.
Verdict
The substance of this round is excellent; the headline flip left a HIGH-severity straggler cluster in the docs.
Three significant moves all landed architecturally sound and wire-clean: (1) the JWT holder-of-key default (Biscuit→JWT) is cryptographically sound — cnf.jkt binds the grant to the request-signing key, the cnf-chain reproduces attenuation/hop-survival/leaked-token-useless, no security property is lost, and it resolves the standing R4-5/R5-2 JWT/brokered-binding findings; (2) Obligation.scope_license string→License is a clean composition (SHARE_ALIKE target now inherits uri⇒uri_digest tamper-evidence — and the author's rationale is sound: a single string couldn't tell an SPDX id from a URI, so a CEL couldn't know when to demand a digest; as a License, the existing uri ⇒ uri_digest rule applies automatically); (3) the 5-round-old "prose-only invariants" critique is finally addressed — d8y64/fc65j/6z1v3 move presence + coherence rules into protovalidate CEL. Those constraints are defined once in the proto and embedded in every descriptor, so they are enforceable by any protovalidate runtime — but, per the author's own correction, they are generated-and-validation-tested only in Go today (TS is generated but has no protovalidate-es wired/tested; Python is not generated — a tracked follow-up). And the author built the doc-sample validation harness recommended every round — conformance/ genuinely evaluates the CEL (30+ valid/invalid cases) and validates doc examples; it is real, sound, CI-wired, test-scoped (no consumer dep), and it caught real drift. All R5 findings (R5-1…R5-11) are resolved.
But the Biscuit→JWT flip + the new two-algorithm-name distinction reintroduced 2 HIGH and several MEDIUM issues — all in the docs / proto-comments / enforcement-story layer, none in the wire/gen. The failures cluster precisely in the blind spots of the (otherwise good) automated guards: the RFC 9421 alg= value (the harness only checks the signature_algorithm field), PascalCase Go (SignatureAlgorithm = "…", a regex gap), proto comments, prose Biscuit-isms, the gate-excluded design-history.md, and unlabeled pseudocode fences.
NEW issues
HIGH
- R6-1 — Algorithm-name swaps in a security primitive (both directions), incl. the canonical proto comment. [orchestrator-verified] The two-name convention (per the author:
ed25519= RFC 9421 request sig;EdDSA= JOSE / Offer JWS / delegation JWT / JWK) is violated in ~5 places:- RFC 9421 request-sig contexts wrongly using
alg=EdDSA:proto/ramp/v1/ramp.proto:1419(which contradictsramp.proto:51"Agent→Exchange: Signature header (alg=ed25519)" in the same file),broker/overview.mdx:106,for-exchange-operators.mdx:144. - JOSE/Offer-JWS contexts wrongly using
ed25519:request-flows.mdx:194(SignatureAlgorithm = "ed25519"),broker/selection-engine.mdx:436(alg := "ed25519" // default for offer signatures).
PAT-01's blanketed25519→EdDSAsweep mismatched the distinction in both directions. An operator followingfor-exchange-operators.mdx:144would verify RFC 9421 request signatures withalg=EdDSAand reject valided25519requests at the auth boundary. Severity HIGH (the operator-step instance is arguably CRITICAL). Fix (doc + proto comment):ed25519for RFC 9421,EdDSAfor JOSE, perramp.proto:51.
- RFC 9421 request-sig contexts wrongly using
- R6-2 — Phantom Biscuit URI-pattern delegation narrowing, claimed "cryptographically enforced." [security]
enterprise.mdx:178/182(+:188"cryptographically enforced"),proto-ramp.mdx:132, andfor-exchange-operators.mdx:157("evaluate all check conditions across all blocks") carry over Biscuit's per-resource/URI confinement, but the default JWT model has no wire field and no registered claim for URI/resource restriction — only scopes/caps/expiry. An operator who believes URI-confinement is enforced gets a fail-open gap. Severity HIGH. Open question 1 gates the fix: was URI-confinement intended to survive the flip (→ add a registered claim/field) or not (→ docs overclaim; remove/rewrite).
MEDIUM
- R6-3 — Required-enum discriminators are NOT wire-enforced. [orchestrator-verified] The proto has zero enum constraints (
grep -c "(buf.validate.field).enum"= 0; the only enum references in CEL are the conditional rules like PER_UNIT⇒unit, not an UNSPECIFIED-rejection), soprotovalidate.Validate()— even the Go runtime — acceptssemantics: UNSPECIFIED(andkind/model/trigger/axis= 0). Only the Go Exchange application code rejects it, not the shared SDK constraint, despite every "// unset — rejected at ingest" comment.conformance/docexamples_test.go:125still asserts UNSPECIFIED "would not validate," which is inaccurate at the protovalidate layer, and there is novalidate_test.gocase proving rejection (the harness gives false confidence here). Note: the author's "UPDATE 18-06-morning" correction proactively walked back the broader "enforced in Go/TS/Python" overstatement — verified accurate (gen/is go+ts only, no Python; noprotovalidate-eswired in TS) — which converts the cross-SDK half of this finding into an acknowledged, tracked limitation. The enum-has-no-CEL gap itself remains and is not addressed. Fix: add athis.<field> != …_UNSPECIFIEDCEL per discriminator + conformance cases, and correct the test comment. - R6-4 —
design-history.mdself-contradiction on the headline decision. [orchestrator-verified] §195 "## Biscuit v3; JWT verification deferred" asserts present-tense "Biscuit v3 is the [v1] format" / "JWT … deferred past v1," left in place beside the new §254 "## JWT … is the default; Biscuit is optional," with no supersede marker. Severity MEDIUM (re-graded down from the consistency/architecture agents' HIGH:design-history.mdis explicitly a non-normative history doc and the normative spec —authentication.mdx+ proto — is correctly JWT-default, so it misleads readers but won't drive a wrong implementation). Fix: mark §195 superseded by §254 (or rewrite past-tense). - R6-5 — Mirror self-contradiction on pricing-required. [orchestrator-verified]
proto-ramp.mdx:655-657says pricing "MUST be present" only "whensemantics = ENUMERATED," while:713says "REQUIRED on every term regardless of semantics." The fc65j CEL enforces every-term (conformance-tested). Fix (doc): drop the ENUMERATED-only wording. - R6-6 — Biscuit-isms remain in default-JWT canonical sources.
ramp.proto:1784DELEGATION_INVALIDstill says "…or its attenuation does not check out" (the phrase PAT-02/R5-5 claimed fully removed — missed in the source the mirror derives from);threat-model.mdx:220"sealed/attenuated to the holder's key" (default JWT is bound viacnf.jkt, not sealed);for-exchange-operators.mdx:157"across all blocks." Fix (proto comment + docs). - R6-7 — Conformance/doc-gate coverage gaps (the harness is good but these let R6-1 through green). (a)
TestDocSignatureAlgorithmkeys only on thesignature_algorithmfield — it cannot see the RFC 9421alg=value (R6-1's EdDSA-in-RFC9421) and its regex misses the PascalCase Go formSignatureAlgorithm = "…"(R6-1's ed25519-in-JOSE at request-flows.mdx:194). (b) The doc-example checks only parse```json-fenced double-quoted JSON, so walkthrough pseudocode/JS-style fences escape all three checks — confirmed live atscenario-walkthrough.mdx:144-150(aterms:[…]with nopricingand nosemantics, a double-miss). (c) Thequota.metric.formatandrestriction.prohibited.formatCELs are never exercised by the suite. Fix (test): check RFC 9421alg=context-aware, broaden example extraction beyondjsonfences, add the two missing CEL cases.
LOW
- R6-8 stale "Token Attenuation" H2 header over JWT body (
broker/overview.mdx:173). - R6-9 website changelog under-records the
scope_licensetype change + the conformance/CEL additions. - R6-10
threat-model.mdxT-DEL-1 leads with Biscuit "sealed/attenuated" phrasing before the correctcnfdescription. - R6-11
consumed_unitregistry membership is doc-test-only (no wire CEL) — a consistency nit vsPricing.unit. - R6-12 the self-deriving
ramp_*doc-gate check interpolates${field}unescaped into a regex — harmless for today's[a-z_]claim names; latent. - R6-13 "hierarchical scope" format wording lingers near the "no implicit hierarchy" matching rule (terminology).
Standing invariants (all intact)
comp.proto not imported by core; dispute chain (Offer→Transaction→UsageReport→UsageReportResponse→DisputeRequest) unbroken; closed licensing core uniform; protovalidate runtime is test-only (no new dep on gen/go consumers); buf.lock/go.mod pin the same protovalidate commit (no version skew); R3-8 (reserved) deferral unchanged and still acceptable (no consumers; the scope_license field-3 string→message reuse is the same sharp-edge class but inside the branch-new Obligation).
What the orchestrator verified directly
buf lint/build=0; go build/vet/test=ok incl. conformance pass; clean buf generate=zero diff; doc gate clean; buf breaking=49 (unchanged). Read validate_test.go (genuine, 30+ valid/invalid CEL cases). Confirmed: the algorithm-name swaps (proto:1419 vs proto:51; the doc sites), (buf.validate.field).enum count = 0 (R6-3), design-history.md §195/§254 contradiction (R6-4), proto-ramp.mdx :655/:713 pricing contradiction (R6-5), proto:1784 attenuation Biscuit-ism (R6-6), JWT-default + cnf model in proto, scope_license as License (1077), seconds registered. Verified the author's SDK-coverage correction: gen/ contains only go + ts (no Python); no protovalidate-es dependency in gen/ts/package.json (only a descriptive comment) — so the CEL is wired-and-tested in Go only today, as the author now states.
Open questions for the maintainer
- (R6-2 — gates the fix) Was per-URI/resource delegation confinement intended to survive the Biscuit→JWT flip? If yes, it needs a registered claim/wire field (it has neither today); if no,
enterprise.mdx/for-exchange-operators.mdxoverclaim "cryptographically enforced" and must be rewritten. - (R6-3 — partially answered) The author's UPDATE note clarified the cross-SDK reality (Go-only wired/tested; TS/Python a tracked follow-up), which settles the SDK-coverage half. Still open: was leaving the required-enum discriminators with no protovalidate CEL at all deliberate (so even Go's runtime accepts UNSPECIFIED and only the app rejects it), while the other presence invariants are CEL? If not, add the per-enum CEL.
- (R6-7) Is the harness intended to validate only
json-fenced wire examples (treating walkthrough pseudocode as illustrative), or should it cover those too?
Bottom line: the protocol is in strong shape — the wire contract is clean and reproducible, the JWT flip is sound and loses nothing, the deepest 5-round critique (prose-only invariants) is finally on the wire (enforceable by any protovalidate runtime; wired+tested in Go today, with TS/Python a documented follow-up), and the recommended validation harness now exists and works. The blockers before merge are R6-1 (algorithm-name swaps in a security primitive, incl. the proto comment) and R6-2 (phantom "cryptographically enforced" URI-confinement) — both doc/comment-level but security-relevant; then the MEDIUMs (R6-3 enum-CEL gap, R6-4 design-history supersede, R6-5 mirror, R6-6 Biscuit-isms, R6-7 harness gaps). None touches the wire format itself.
Add protovalidate CEL so the required discriminator enums are rejected at the wire/validation layer rather than only by application ingest: LicenseTerm.semantics, Pricing.model, Restriction.kind, Obligation.kind must not be *_UNSPECIFIED. PricingMetering.ONLINE=0 stays the deliberate safe-default exception. Add a format CEL to UsageReport.consumed_unit mirroring Pricing.unit (bare registered token or vendor:namespaced), so the metering unit is structurally validated on the wire instead of doc-test only. Regenerate Go + TS SDKs. Extend the conformance suite with the UNSPECIFIED-rejection cases and the previously-unexercised quota.metric.format and restriction.prohibited.format rules.
Strengthen the doc-conformance guards to catch the value-level and pseudocode-fence drift the prior denylist could not: - Signature-algorithm check is now context-aware. It enforces the two-name convention (ed25519 = RFC 9421 request signatures; EdDSA = JOSE/JWS for the Offer signature, delegation JWTs, JWKs) across the signature_algorithm JSON field, the PascalCase Go SignatureAlgorithm form, and bare alg=… tokens classified by same-line context. Ambiguous lines are skipped, not guessed (57 sites checked, was a handful). - LicenseTerm semantics check now scans all code fences (not just ```json) and matches quoted-or-unquoted keys, so term shapes in walkthrough pseudocode are covered (551 fences scanned). Harden the delegation-claim doc-gate: guard the derived field name against unexpected characters before interpolating it into the match pattern.
…lt precision - Fix algorithm-name swaps: ed25519 for RFC 9421 request signatures, EdDSA for JOSE/JWS (offer signatures). Touches broker/overview, broker/selection-engine, exchange/request-flows, for-exchange-operators. - Qualify the "cryptographically enforced" URI-confinement claim: the default JWT model has no URI/resource claim — URI gating is via scopes; per-URI confinement is a biscuit-v3 / vendor-claim capability, not a wire guarantee. (enterprise, for-exchange-operators) - Remove Biscuit-isms from JWT-default sources: holder binding is via cnf.jkt (threat-model T-DEL-1); drop "attenuation across blocks". - Resolve proto-ramp pricing-required self-contradiction: pricing is required on every term regardless of semantics; rewrite the validation-rules block to cite the enforcing CEL ids. - Fix walkthrough term examples missing semantics/pricing. - design-history: mark the superseded "Biscuit v3 default" section. - changelog: record Obligation.scope_license as a License and the wire-enforced CEL + conformance suite. - Replace lingering "hierarchical scope" wording with the segment-wise rule.
|
Thanks Yaroslav — round 6 was a good catch on the algorithm-name distinction; the Biscuit→JWT flip left more residue than the diff suggested. All of R6 is addressed on Your two open questions
HIGH
MEDIUM
LOW
Still deferred (your standing R3-8 note)No Build stays green: |
legendko
left a comment
There was a problem hiding this comment.
The PR can be approved, the following gaps are non-blocking, just nice-to-fix.
Tell me whether you want to address them or leave.
Residual findings (all NON-BLOCKING)
MEDIUM
- R7-MED-1 — R6-3 is incomplete:
Quota.windowandObligation.triggerstill acceptUNSPECIFIED. [orchestrator-verified + both agents] The!= …_UNSPECIFIEDrejection CEL was added to four discriminators but not toQuota.windoworObligation.trigger, both of which carry the identical// unset — rejected at ingestcontract (proto:836,:856). Proof it is real, not theoretical: the passing conformance case{"quota limit ok", Quota{Metric:"accesses", Limit:1}, true}(validate_test.go:87) haswindowunset → protovalidate provably accepts a quota with an ambiguous accumulation period; same for an obligation with an ambiguous firing trigger. Why non-blocking: the four highest-stakes discriminators (which select enforcement semantics and pricing structure) are wire-enforced, and a malformedwindow/triggeris still rejected by the Go Exchange at ingest — it is only the shared protovalidate constraint that is missing, and these are coherence fields, not a trust boundary. Fix: twothis.<field> != …_UNSPECIFIEDmessage CELs + two conformance cases (≈4 lines).AcceptableRestriction.axis(advisory request input) andPricingMetering.ONLINE=0(real safe default) are correctly left alone.
LOW
- R7-LOW-1 — Mirror not updated for
consumed_unit. [consistency]e961ec4added the format/length CEL toUsage.consumed_unitin the proto, butproto-ramp.mdx:353still says only "defaults to tokens," while the siblingPricing.unit/Quota.metricrows document their format in the same mirror. CLAUDE.md mandates the mirror track the proto. Fix: add the format/length constraint to that row. - R7-LOW-2 — Changelog over-states the enum fix. "required discriminator enums reject UNSPECIFIED" reads as complete, but two (
window/trigger) are not yet enforced (R7-MED-1). Fix: name the four, or complete R7-MED-1 first. - R7-LOW-3 — Mixed citation style (cosmetic). The mirror's validation block says "cite the enforcing CEL ids," but three of four citations (
fc65j/d8y64/6z1v3) are requirement tracking-codes (cross-referenced in proto comments + conformance tests), not CELid:s. They resolve correctly; only the style is mixed.
(standards-layering.mdx:36 "Ed25519 (EdDSA)" was assessed and is benign — a crypto-stack key-type mention, not an alg= assignment.)
Round 6 added the !=UNSPECIFIED CEL to four discriminators; the round-7 re-review found two more left out (Quota.window, Obligation.trigger), both marked "rejected at ingest" — the enumeration was done by hand, twice, and was incomplete both times. Add the two missing message CELs (quota.window_specified, obligation.trigger_specified) and, more importantly, stop enumerating by hand: TestRequiredEnumDiscriminatorsRejectZero reads the proto, collects every enum whose zero value is marked "unset — rejected at ingest", walks the descriptors, and fails unless each field of such an enum is either covered by a zero-rejection rule or explicitly allow-listed with a reason. A new discriminator now fails the build until enforced — the set can no longer drift. The guard immediately surfaced four advisory/diagnostic fields the manual reviews never flagged (OfferGroup.restriction_filters, the two restriction_mismatches, WellKnownManifest.pricing_models_supported); these are Exchange-produced output / capability advertisements, not enforced term discriminators, so they are allow-listed with reasons (same category as AcceptableRestriction.axis). Conformance fixtures set window/trigger so each case fails only for its named reason; added explicit window/trigger UNSPECIFIED-rejection cases.
…tor set - proto-ramp: document consumed_unit's wire-enforced token format (was only "defaults to tokens"); clarify that the validation-block parenthetical codes are requirement tracking-codes or CEL rule ids. - changelog: name the six required discriminators (was the vaguer "required discriminator enums") and note the derive-from-proto guard.
|
Addressed — all four, plus a process fix so this class can't recur. On The real question your R7-MED-1 raisedHow did R6 and R7 both miss So instead of point-fixing, I made the enumeration mechanical. New guard
A newly added discriminator now fails CI until it's enforced or consciously allow-listed — the set can't drift out of sync with the contract comment again. It immediately caught four more that both reviews missed
The four findings
Also confirmed your Green: |
One command runs the full CI gating sequence locally (buf lint, buf generate + drift check vs HEAD, go build/vet/test, doc-conformance), so running only a subset can't silently pass a check CI will fail. buf breaking is included as informational/non-blocking, matching the workflow. Non-destructive: does not touch the git index. (cherry picked from commit e1ebc84)
Summary
Introduces the Universal Licensing Core — a structured, cross-domain licensing model for any digital resource — and a proto-native vocabulary mechanism for the open string axes it relies on. One resource carries many
LicenseTerms (free for academic RAG, paid for commercial, reference-only for complex rights), the same shape at ingestion (ResourceEntry.terms) and emission (Offer.terms).The design evolved over the branch (see commit history); this description reflects the final state. Closes #5.
Message model
LicenseTermadded toResourceEntry(field 13) andOffer(field 19).Licenseis identity-shaped:uri(canonical, never URL-validated — data-labels TDLs use non-URL schemes),id(SPDX short-id / TollBit cuid),name,immutable.Obligationcarriesscope_license(required forSHARE_ALIKE) anddetail.Pricing: charging structure (enum) + metering basis (vocabulary)
PricingModelwas collapsed to the closed charging structure; the open-ended metering basis moved out of the enum intoPricing.unit(a vocabulary — see below).PER_UNIT⇒Pricing.unitrequired (a registered token or avendor:custom).FLAT= one-time fee.FREEmust be explicit.Pricing.metering(field 9) added.Pricing.revshare(field 6) andPRICING_MODEL_REVENUE_SHAREremoved — settlement is off-protocol.Proto-native vocabulary mechanism (the open axes)
The open string axes (
Pricing.unit,Quota.metric, and thefunction/geography/user-typerestriction axes) are defined in the proto and tooled by buf — no JSON registry, no enum-of-strings.vocab.proto— custom options(ramp.v1.vocab)onFieldOptions(ext50001) and(ramp.v1.vocab_enum)onEnumValueOptions(ext50002). The registered token list is authored once, as these options (on the field for single-axis fields; on theRestrictionKindenum values for the kind-discriminated restriction axes).protovalidate(adopted as abuf dep): a structural field CEL (empty / well-formed bare token /vendor:namespaced) plus message-level CEL for cross-field rules (PER_UNIT ⇒ unit != '',FREE ⇒ rate == 0). Enforced at the RPC boundary by the Connectvalidateinterceptor, in any language, no generated validation code.cmd/protoc-gen-rampvocab) reads(ramp.v1.vocab[_enum])structurally off the descriptor and emits, intobuf generate, a typed-constant package per axis undergen/go/vocab/:pricingunits,quotametrics,functiontokens,geographytokens,usertypes— each with token constants,All, andIsRegistered(). Application code branches on these; membership ("a bare token must be registered") is enforced from the same single source. Geography registers only the non-ISO specials (*,EU,EEA); ISO-3166 alpha-2 codes are structural.ns:anything, no registry change.This replaces the side-car
vocab/*.jsonfiles, which are deleted in this PR.Required enums carry
_UNSPECIFIED = 0TermSemantics,RestrictionKind,QuotaWindow,ObligationKind,ObligationTrigger,PricingModeleach start at_UNSPECIFIED = 0and are rejected if left unspecified (omission cannot silently default to a real value).PricingMeteringis the deliberate exception —ONLINE = 0is its real default.Breaking changes / removals
AccessRestrictionsremoved —LicenseTermis the sole restriction model (noOffer.restrictions).Pricing.revshare+PRICING_MODEL_REVENUE_SHAREremoved; the old per-unitPRICING_MODEL_PER_*collapsed intoPER_UNIT+unit.PRICING_MODEL_ATTRIBUTION/CONTRIBUTIONretired earlier in the branch (they areObligationKinds).Worked examples (final shapes)
News article — free for academic, paid for commercial
semanticsrestrictions[FUNCTION].permitted["ai-input"]["ai-input","ai-index"]restrictions[USER_TYPE].permitted["academic"]["commercial_entity"]pricing{model: FREE}{model: PER_UNIT, unit: "accesses", rate: 0.05, currency: USD}obligations[0]Stock photo — perpetual license, impressions cap
{semantics: ENUMERATED, restrictions[FUNCTION].permitted: ["display"], restrictions[GEOGRAPHY].permitted: ["*"], pricing: {model: FLAT, rate: 1.20, currency: USD}, quotas[0]: {metric: "impressions", limit: 500000, window: TOTAL}, obligations[0]: ATTRIBUTION}Patent/CAD — manufacture license, offline metering
{semantics: ENUMERATED, restrictions[FUNCTION].permitted: ["manufacture"], pricing: {model: PER_UNIT, unit: "units-manufactured", rate: 0.50, currency: USD, metering: OFFLINE_SELF_REPORTED}, quotas[0]: {metric: "units-manufactured", limit: 1000, window: TOTAL}, obligations[0]: CONTRIBUTION}Tooling added to this repo
proto/buf.yamldep onbuf.build/bufbuild/protovalidate;cmd/protoc-gen-rampvocabwired as a local plugin inproto/buf.gen.yaml.gen/go/vocab/*constant packages.vocab/*.jsonregistry files deleted (the vocabulary now lives in the proto).