feat: add the OAN registry, JSONata mapper and weather provider plugins by ameersohel45 · Pull Request #2 · OpenAgriNet/network-adapter

ameersohel45 · 2026-08-31T12:59:44Z

What

Three plugins that let this adapter serve an OAN provider capability end to end, plus the core changes they need. Additive: nothing existing changes behaviour.

oanRegistry plugin — `pkg/plugin/implementation/oanregistry/`

Reads the OAN registry over its public /search endpoint. Serves two independent jobs: the sender's public key for validateSign (RegistryLookup), and a capability binding resolved into a call plan (ProviderRecordLookup).

Two caches under one operator-set TTL, off by default — the TTL is exactly how long a suspended participant keeps verifying.

It tracks the registry's current contract: bare base64 key material with no encoding label, keys identified by the osid the registry assigns rather than a friendly id, and consumer/provider/network roles. A response captured from a live registry is pinned as a test, alongside an older capture, because the registry is append-only — a row written under the previous contract keeps its old shape forever and both must keep reading.

Mapper plugin — `pkg/plugin/implementation/jsonmapper/`

Generic JSONata mapper: fetches a mapping at runtime, compiles, caches, runs. Knows nothing about any provider.

One file per binding-action carries both directions under request: and response:, plus a required: block of preconditions. Everything compiles on one fetch, and each part owns its own failure, so a typo in one half does not disable the other.

required: is what lets a capability state what it cannot serve. The schema packs are deliberately permissive — a spec-valid payload can still be unanswerable by a given upstream — so the mapping refuses those with its own message rather than sending a broken request and reporting whatever the provider says.

Provider step: machinery and domain

Split, because none of the calling is domain-specific:

pkg/plugin/implementation/internal/upstream/ — the machinery. Recognise the capability, resolve the call plan, translate out, call, translate back, with auth, retries and a response size cap. Holds nothing about any provider or domain.
pkg/plugin/implementation/weather/ — the domain. 32 lines: its name, and the prerequisite work a mapping cannot express.

Dispatch is by binding key (<participantId>|<capabilityCode>) read from the payload, with pass-through when it is not this step's capability, so several provider steps share one pipeline and adding a provider touches no routing table. A step serves a list of capabilities, and where the two halves of a binding key sit in a payload is overridable for a spec change, defaulting to the Beckn v2 convention.

Auth covers none, basic, header and query. The query scheme exists because some upstreams take their credential as a query parameter; the config holds the parameter's name and the name of the environment variable carrying the value, never the value. It also redacts: Go quotes the full URL in a transport error, so without that one unreachable host writes the credential into the log.

Every attempt logs what was asked and what came back, at info, with the credential removed:

upstream: GET http://host/v1/x?statecode=CG&token=REDACTED -> 200 OK, 492 bytes

Core changes

StepContext.ResponseBody lets a step return its own answer instead of the generated ACK. Empty means "generate the ACK", so existing modules are unaffected, and it is only consulted on the no-route path.
sendResponse writes that answer, and ackSigner signs the same bytes — signing the ACK while sending an answer would put a valid signature over the wrong body.
A module that answers capabilities itself refuses an unanswered request rather than ACKing it. An ACK with no callback tells the caller "accepted, answer follows" and leaves it waiting, which is how a stale binding key hides as a healthy response.
New mapper and providerSteps plugin kinds. providerSteps is separate from steps because they are handed a registry and a mapper, which the plain StepProvider contract cannot do.
ProviderRecordLookup and ErrProviderRecordNotFound in definition/registry.go, obtained by type-asserting a RegistryLookup — the same pattern already used for RegistryMetadataLookup.

The shipped mapping

config/mappings/mausamgram/weather-observation.select.yaml follows the openagrinet:WeatherObservation v0.1 pack: an OnDemand request, a Direct answer with one resource per forecast day, ids derived from each date, and the offer's resourceIds rewritten to the days actually returned rather than echoed from the request.

It answers however many days the provider sent, sorted on the numeric suffix rather than the key — fcstday1, fcstday10, fcstday2 sorts wrongly as text, and a ten-day forecast delivered in that order would be wrong in a way nothing downstream could detect.

Two details are spec conformance rather than taste. status.descriptor.code is DRAFT, because the Beckn v2 enum is DRAFT/ACTIVE/CLOSED and a quote is a draft commitment. And each resource carries a quantity, which Commitment.resources requires while the spec defines no quantity property and carries no Quantity schema at all — a defect upstream, but an answer without it fails validation for any consumer who validates.

The response context carries only correlation ids. It does not echo bapId, bapUri, bppId or bppUri: a mapping transforms a payload and has no business asserting network identity, and the two Uri fields were whatever the caller sent — a container-internal address, in a deployed stack, republished as though it were ours. Identity on an answer is the signature over it.

Why

An OAN provider is now a plugin plus a registry row, rather than another per-usecase backend service. The registry says which provider to call, how, and which mapping translates it; the plugin does only the work a mapping cannot express. Adding a provider touches no routing table and no shared code.

The machinery and domain split is what makes that claim testable: a second domain package should need no change to internal/upstream. It didn't — see the Mandi plugin on feat/8-mandi-plugin, which is 58 lines and touches none of it.

Testing

go build ./..., go vet ./... and go test ./... clean across 63 packages; race detector clean on the new packages.

The shipped mapping is run through the real mapper and the real provider step, against a captured provider response, rather than asserted about. Payloads in both directions are validated against the pinned Beckn v2 LTS spec and against openagrinet:WeatherObservation v0.1 with jsonschema — zero errors.

End to end on a local stack (registry, discovery, three adapters, mock provider): a signed /select returns an on_select carrying one resource per forecast day; /discover still answers; /publish reaches the discovery service through the network layer. A payload naming a provider this module is not configured for is refused rather than silently ACKed, and one whose registry row is missing reports the binding key that has no call plan.

Notes for review

config/local-beckn-one-bap.yaml and -bpp.yaml gained OAN plugin wiring. If the shipped samples should stay untouched, that is a clean revert — the OAN config also stands alone in config/oan-provider-adapter.yaml.
The registry's auth block is deliberately not read: an upstream's credential is the provider plugin's own configuration, and reading both would create two places that can disagree about how to authenticate a call.
A provider step matches the whole binding key, so it serves only the providers named in its config. Matching on the capability alone — which would let a provider be onboarded by registry writes alone — was built, tested and reverted: it silently skips the pre-call work of a provider nobody has written code for, and that defect exists today rather than being introduced by the change. Parked for discussion.

Story: OpenAgriNet/engineering-tracker#41

Closes #1
Closes OpenAgriNet/engineering-tracker#46
Closes OpenAgriNet/engineering-tracker#63
Closes OpenAgriNet/engineering-tracker#66

Resolves two different things from the OAN Registry, a SunbirdRC deployment, and keeps them apart because they answer different questions about different parties. RegistryLookup answers "who sent this": given the subscriber and key named in an inbound Authorization header, it returns that sender's signing key so the signature can be verified. This runs inside signature validation on every inbound message, so its timeout and retry budget are deliberately tighter than the sibling registry plugins' -- timeout x (retry_max + 1) is time a request spends waiting before it can even be rejected. ProviderRecordLookup answers "who do I call next" [beckn#63]: given a capability binding taken from a request body, it reads the binding and the participant that owns it, and joins them into one call plan -- where the provider is, and per Beckn action, how to reach it. Every way of saying "this capability cannot be served" returns one sentinel, because a caller does the same thing with all of them; a registry that could not be CONSULTED returns its own error, since an outage is not an answer. Several decisions here were forced by the deployed registry rather than chosen: - records are read from the nested shape the registry actually serves, with keys under node.keys[] rather than flat on the record - a key is matched by its osid, which is what an Authorization header carries; the friendly keyId identifies nothing the registry indexes - the "base64:" label is stripped from key material, because model.Subscription carries the bare value signvalidator feeds straight to base64.StdEncoding.DecodeString - status is checked at both levels, since a participant stays active while one of its keys is retired - actions are read as an array: the registry treats every nested object as an entity and injects osid into it, which a map cannot carry Status is an allow-list throughout, not a deny-list. model.IsKeyStatusUsable treats anything it does not recognise as usable, so passing the registry's own vocabulary through unchanged would let a suspended participant's signature verify. Verified against a live registry and the recorded response it serves.

Providers do not speak Beckn. The old provider backend answered that with one hand-written service per provider -- around 6,900 lines across eight of them, most of it building catalog JSON field by field. This makes the translation configuration instead: a new provider ships mapping files, not another transformation routine. The plugin is domain-free by design. It knows nothing about who is calling, nothing about what a mapping says, and nothing about the payloads passing through: it is handed a reference and an input, and it fetches, compiles, caches and runs whatever is there. Anything specific to a network or a provider belongs in the caller, which is what lets one mapper serve all of them. A mapping file carries every action one capability serves, keyed by action name. Request files are keyed by the action they translate, response files by the one they produce -- so a select mapping sits under "select" and its answer under "on_select", and each file names the Beckn actions it actually deals in. One file per direction rather than per action means a transaction walking select then confirm pays one fetch, not one per step. An action may be declared with an empty value. That is a statement rather than an omission: this action needs no document built, because the caller supplies the request itself -- a provider taking two query parameters is the ordinary case, and passing already-resolved values through a fetch and a compile to arrive at the same two fields buys nothing. Declared-but-empty and absent are deliberately different, and reported differently: the first says "I serve this, build it yourself", the second says "I do not serve this at all". Collapsing them would send an empty request where a refusal was owed, answered with a 200 and the wrong data. Two things the race detector settled rather than the design: - jsonata.Expression.Evaluate MUTATES the expression it is called on, binding into its own frame, so a cached compiled expression cannot serve two requests at once. Evaluation takes a per-mapping lock rather than recompiling: measured, evaluation is ~22us against ~184us to compile, and both are dwarfed by the upstream call that follows - a compile failure is held against its own action, so a typo in confirm is no reason for select to stop being served References arrive from the registry, which makes them external input: anything that is not an http(s) URL with a host is refused, and reads are capped in both time and size.

The last piece: a step that recognises its own capability, resolves what the provider needs beyond the Beckn payload, calls it, and lets the mapper translate both ways. With the registry supplying the call plan and the mapper the translation, adding a provider is now a plugin with two short methods, two mapping files and a registry row. Dispatch turned out to need no mechanism at all. A provider step handed a request for a capability it does not serve does nothing and returns nil, so several sit in one pipeline and each recognises its own work. There is no routing table to keep in step with the registry, and no filename convention -- which matters because a binding key contains | and :, and a plugin id is its .so basename. Keying on the binding key rather than the participant is deliberate: one provider can serve several capabilities with different logic, as gfr-crop-registry and gfr-crop-recommendation did in the old backend. Three supporting pieces: - definition.ProviderStepProvider, because a provider step needs a registry and a mapper handed to it, which the plain StepProvider contract cannot do. Same shape as PolicyCheckerProvider taking a ManifestLoader - internal/oanbinding derives the binding from a payload. Shared, because the binding is a property of OAN's payloads and not of any provider. A payload naming more than one distinct provider or type is refused rather than resolved to its first: one binding key describes one upstream call, so guessing would silently serve part of the request - model.StepContext.ResponseBody, so a step that has already obtained an answer has somewhere to put it. Without it the no-route path writes a fixed ACK and ignores the body entirely, so the answer was discarded and the caller got an ACK for data it asked for synchronously That last one has four call sites, every one gated on the field being non-empty, so no existing module changes behaviour by a byte. The gate that matters least visibly is in the step instrumentor: it shallow-copies the context in but copies only named fields out, so without one line there an answer written by an instrumented step vanishes -- and instrumentation is the default path, meaning it would work unwrapped and fail wrapped. signAck signs whichever body will actually be written. Signing the generated ACK while sending an answer would put a valid signature over the wrong bytes, which is the one failure here that looks fine in testing and is rejected by every peer. Mausamgram itself is small: its prerequisite reads a point from the request, and coordinates are GeoJSON order -- [lon, lat] -- which read the other way round yields a valid request for the wrong hemisphere, so there is a test for exactly that. Auth is configured by scheme naming the ENVIRONMENT VARIABLE to read, never the credential: the secret reaches the process through its environment and nothing else, and never through the registry. A configured credential that is absent fails the request rather than calling the provider unauthenticated. Verified end to end against a live registry, mappings served over HTTP and a mock provider: a signed select in, a valid on_select out.

…ng-tracker#41] The registry schemas at OpenAgriNet/discovery-service docs/registry/schemas.md changed shape. This reads the new one. Participant is flat. type -- node or upstream -- decides which fields apply, where the old shape wrapped them in a "node" or "upstream" object. baseUrl is one field for both, and role (BAP/BPP/NETWORK) is separate from type. A binding's actions carry their own mappings and their own status. Retiring one action is now one field on one entry, leaving the capability and every other action live; an inactive entry is skipped exactly as an absent one is. One mapping file per binding-action, holding both directions, replacing the per-direction pair. The halves are not independent -- the response mapping reads what the request mapping resolved into _local -- and two references hid that. A half that is absent or empty reports ErrNoTransform, which is a statement rather than an omission; a half that will not compile is an error, and the two must not collapse or an unmapped upstream answer would go out as a Beckn response. mappings stays a fully-qualified URL, carried verbatim. The documented contract specifies a repo-relative path resolved against an operator-configured root, and that is the safer shape -- it stops a registry row choosing which host this adapter fetches, compiles and runs a mapping from. It is deliberately not adopted yet: the network has not settled on a fixed location for published mappings, so the URL stays in the record and the local ProviderSchema pattern is relaxed to match. Who may write a registry row is therefore part of the mapper's threat model, and that is written down where the check lives. Also the contract's action budget defaults: timeoutMs 15000 and retryMax 0, and retryMax now counts retries rather than total attempts -- so an action that does not ask for retries is called exactly once. A retry on a non-idempotent action is a second booking. The registry's auth block is still not read, deliberately: an upstream's credential is the provider plugin's own configuration, and reading both would create two places that can disagree about how to authenticate a call. The captured-registry test is re-captured from the live registry in the new shape, and the end-to-end select through the local stack returns three mapped forecast resources.

…acker#66] Two things a mapping did not need. _local is gone. A mapping is handed the inbound payload, and on the way back the provider's answer, and nothing else. The values a provider plugin resolves before a call were also being passed in, which was a detour: the plugin holds them and used them to make the call, so a mapping reading them back was a second name for the same data. Where an answer needs one, it takes it from what the provider echoed -- the shipped mapping now reads response.location for the coordinates it was reading out of _local. ErrNoTransform is gone. A half that is absent or empty produces nothing, with no error, and what nothing means belongs to the caller rather than to a sentinel the mapper invents. On the request leg it means there is no document to send: for a method with no body the plugin sends the values it resolved, because it knows its provider and does not need the mapping's permission to call it; for a method with a body it sends no body, which is what an empty mapping says. A half that will not compile still reports an error, and that distinction is now carried by a test of its own: reading a broken half as "nothing" would send an unmapped upstream answer out as a Beckn response. One failure path is new. A response half that produces nothing leaves no Beckn answer, so the step fails rather than returning the provider's own shape under a valid signature. Its message says what was observed rather than guessing whether the transform was absent or simply matched nothing. Verified end to end: a signed /select returns three mapped forecast resources with the coordinates intact, which is what proves the mapping no longer needs _local to produce them.

A module that serves capabilities itself has no proxy behind it. When no step answered and no route was set, nothing ever will: there is no route to forward the request and nobody to send a callback. ONIX answered that with an ACK, which tells the caller "accepted, answer follows" and leaves it waiting for a message that is never coming. That is not a theoretical case. It is what a stale binding key looks like: the provider step reads a binding from the payload, does not recognise it, passes through -- which is the dispatch mechanism working correctly -- and the request falls out of the pipeline unanswered. The adapter then reports success. It cost two rounds of confusion during this work before the ACK was read as a symptom rather than as expected behaviour. So a module with provider steps now refuses an unanswered no-route request with 404 NET_ENTITY_NOT_FOUND. Modules without provider steps are untouched: an unanswered request there is the publisher or proxy path doing exactly what it should. 404 rather than AckNoCallbackErr, which exists for this shape and was the obvious pick. It maps to 202 Accepted, and a 2xx is what let this hide in the first place; it is also for a business outcome -- no inventory, provider closed -- where this is "nothing here serves that", which is what a 404 says. The check sits before the response steps, not after: ackSigner signs the body it expects to be written, so NACKing later would ship a signature over the ACK with a NACK body. No single provider step could make this decision. Several sit in one pipeline and each passes through what is not its own, so a step seeing a foreign binding cannot know whether a later step will serve it. Only the handler knows, once every step has run, that nobody did. Verified end to end: a select naming an unserved provider now returns 404 with NET_ENTITY_NOT_FOUND where it previously returned 200 ACK, and a select for the served capability still returns on_select with three mapped resources.

on_select minted a resource per forecast day, with ids derived from the date. The offer, echoed from the request, still referenced the id the consumer selected -- so offer.resourceIds pointed at something that appeared nowhere in the answer. The spec says resourceIds are "references to resources covered by this offer", and ours resolved against nothing. The answer now quotes ONE resource, carrying the id the request selected, with the forecast days under resourceAttributes.observations. That is the better model independently of the bug: the consumer asked for a quote on one resource, so that is what is quoted, and the days are content of it rather than resources of their own. It also fixes the reference by construction rather than by patching resourceIds to match invented ids. Fields that are the same for every day -- the point, the source, the observation type -- now appear once at the top instead of being repeated per resource. Only what varies per day repeats. Mapping file and its test only; no code. Both ends of the translation are data, which is what makes a response-shape change configuration. Left alone deliberately: Commitment.resources requires "quantity", but the spec defines no quantity property on Resource and no Quantity schema, so any value would satisfy it. Inventing one would commit us to a shape the spec has not chosen. Verified end to end against the published mapping: one resource carrying the requested id, three observations inside it, offer.resourceIds resolving, no errors.

@type

The mapping now produces what openagrinet:WeatherObservation v0.1 requires in Direct mode. The pack lives in OpenAgriNet/network-specs and had not been read when the mapping was first written; four things were missing or wrong. generatedAt was absent, and Direct mode requires it. So was a resource-level validity, which now spans the first forecast day to the last. A warning was a field of its own called "advisory". The pack has no such property, but its parameter enum carries Alert -- so a warning is now a parameter like any other reading, with unit "1", which is what the pack prescribes for a value that has no unit. It inherits the same $exists guard, so a day the provider gave no warning for carries no Alert entry rather than an empty one. The published catalog resource, which carried no informationMode at all, becomes OnDemand: supportedObservationTypes, supportedParameters, forecastHorizon, updateFrequency and geographicGranularities, and deliberately no parameters, which that mode forbids. One @type therefore serves both the catalog and the answer, and informationMode selects which half of the contract applies -- so a discover filtering on the outcome type finds the catalog. TWO FIELDS REMAIN OUTSIDE THE PACK, both deliberate, both recorded in the mapping's header. The pack sets no additionalProperties, so they validate; they are simply not governed. observations The pack carries one validity and one flat parameters array per resource, and every one of its examples is a single period. It cannot express a five-day forecast in the one resource the request selected. Splitting into five resources would return ids the consumer never selected and break the correlation the Contract model rests on, so the days stay inside. aggregation The pack's parameter entry is parameter/value/unit only. This provider reports a minimum AND a maximum for temperature and humidity, indistinguishable without it. @context stays the canonical schemas.openagrinet.global identifier. In JSON-LD that is a name, not a fetch target: it need not resolve today, and substituting a raw git URL that does would put an implementation detail on the wire and break every consumer when the branch is renamed. Mapping file and its test only; no code. Verified end to end against the published mapping: one resource carrying the requested id, three observations, Direct's required fields present, and the warning carried as an Alert parameter.

The step used to read the coordinates out of the payload itself and send them. That put the choice of which payload fields reach the provider in Go, so a provider wanting one more query parameter meant editing a struct, rebuilding and redeploying. It is the mapping's decision now: whatever the request half produces IS the request. resolvePoint, the point struct and flatPair are gone. What replaced them reads one field: if location.Type == geometryPoint { return nil } verifyGeometry exists only because a mapping cannot refuse. A Polygon's coordinates are nested, so JSONata would build a query parameter that is not a scalar and fail with an error naming neither the geometry nor the reason. The guard turns that into a 400 that says "request carries Polygon; this capability needs a Point". It reads the geometry's type and nothing else, so which fields reach the provider stays entirely configuration. One behaviour change: an empty request half now means an empty request -- no query parameters, no body. Nothing is substituted, because the step no longer holds anything to substitute. TestRunGuardsGeometryWithoutReadingCoordinates pins the boundary. It sends a Point whose coordinates are [1.0, 2.0] and a mapping producing {"station":"NASHIK-1"}, then asserts the query carries station and NOT lat or lon. Reintroducing extraction in Go fails that test. Verified end to end. Adding a date range to the published mapping -- two lines, no rebuild, no restart, no registry change -- reached the provider as ?from=2026-08-30&lat=19.9975&lon=73.7898&to=2026-09-03, and was reverted after. The geometry guard still answers 400 for Polygon, LineString, MultiPoint and an absent location, and 200 for a Point.

A mapping decides what a provider is asked for, but it could not decide that a request cannot be served at all. That judgement stayed in Go, so a capability with its own rule needed its own build -- and the rule sat in a different file from the extraction it guarded. Mappings now carry an optional block, checked before either half runs: required: - check: | ( $ra := beckn.message...resourceAttributes; $exists($ra.location) and $ra.location.type = "Point" ) message: "this capability needs a Point location" Named check/message so each field says what it is. "test" says nothing about which way the predicate must answer, and "otherwise" reads like an alternative value rather than an error. Verify is a method of its own rather than folded into Transform. Transform returns early for a half with no transform, so a mapping with an empty request half would have skipped its own preconditions -- a trap that cannot arise when asking is a separate call. Four ways this refuses rather than passing quietly: a false predicate returns its message as a 400; a predicate answering anything but true or false is a mapping fault, NOT permission, because a typo yielding nothing would otherwise wave through every request the check existed to stop; a predicate with no message is a fault, since refusing without saying why is what this avoids; and a predicate that will not compile reports itself without taking the halves down. verifyGeometry and geometryPoint are gone from the provider step, which now asks and propagates. The consequence is worth stating: nothing in the adapter enforces a payload rule any more. Whatever a mapping does not require, it accepts. That is the point, and it is why the responsibility sits in the published file. The shipped mapping also stops naming five forecast days. The provider answers fcstday1..fcstdayN and N is whatever the forecast ran to, so five truncated a ten-day answer. Two things that testing caught and reasoning would not have: the keys sort lexically as fcstday1, fcstday10, fcstday2, so the days are sorted on the numeric suffix; and JSONata collapses a one-element sequence to a bare value, so the list is forced to an array or a single-day forecast answers with an object where every other N answers with a list. That second bug was present in the hardcoded version too, masked because the mock always sent three. Verified end to end. A Point is served; a Polygon, a LineString, a MultiPoint and an absent location are each refused with the mapping's own message. With the provider sending one, three and five days the answer carries one, three and five observations, and the resource validity window follows.

A provider can serve more than one capability. The registry contract says so outright: a provider serving two capabilities is one Participant and two ProviderSchema rows. The adapter could not. bindingKey was a single string, so a second capability meant a second providerSteps entry with the same plugin id -- and those collide in the handler's id-keyed step map. The second silently overwrote the first, the step list could only name it once, and a capability disappeared with no error at startup or at request time. bindingKeys is a list now, and the step checks membership. Comma-separated, because a plugin config value is a string: the convention reqpreprocessor and schemav2validator already use, and unambiguous here because a binding key separates its own halves with a pipe. Nothing else was needed -- what differs per capability is the endpoint, the mapping and the budget, and all three come from the registry. Configuring the same provider step id twice is now refused at startup rather than quietly losing one. The message says where the capabilities belong instead, because the mistake is easy to make and impossible to see: provider step "mausamgram" is configured more than once; a step serving several capabilities lists them in its own config Widening the config must not widen the dispatch, so a test pins that a capability the step is NOT configured for still passes through untouched. That is the mechanism several provider steps depend on to coexist. Verified end to end, including the duplicate-id case: with two entries sharing an id the adapter refuses to start, where before it started and served one capability fewer.

mausamgram held two things that had grown apart: the machinery for calling an API that has never heard of Beckn, and the fact that it was IMD's weather forecast. By the time preconditions and extraction had moved into the mapping, the second was nothing but a package name -- every remaining line was generic. internal/upstream is the machinery. "upstream" is the registry's own word for such an API, a Participant of type upstream as against a node that speaks Beckn, so the package says what it does in the network's vocabulary rather than a new one. It recognises a capability, resolves the call plan, authenticates, calls with the registry's budget, and translates in both directions. None of that differs by domain. weather is the domain package, and it is 56 lines. One package per schema pack family, so which plugin owns a capability is readable from its binding key: openagrinet:WeatherObservation and openagrinet:WeatherAdvisory are weather's, openagrinet:MandiPrice will not be. A market or knowledge plugin is now a package of the same size and a cmd directory. What a domain package owns is its name and its prerequisites -- the work a mapping cannot express, keyed by binding key. The map is empty, deliberately: every capability so far is served by reading the payload, which the mapping does. An entry is needed only for real I/O, a station id from a spatial lookup or a token from an exchange, because no expression language should be able to do those. Adding one is a function and a line, and nothing else in the package moves. So _local returns, and this time it earns the name: it carries whatever a prerequisite produced, and an empty map when there is none. A mapping reading _local.stationId on a capability without one finds a missing field rather than failing. There is no default binding key any more. A package serving a family cannot guess which of its capabilities a deployment has providers for, so naming one would be wrong for every other domain built on the same machinery -- and silently wrong. It is required, and refused at startup. The mausamgram name survives where it belongs: as a provider id in fixtures, in resource ids, and as the directory its mapping lives in. That is the provider, not the plugin. Verified end to end: the step loads as "weather", serves the capability its config names, and the mapping's own preconditions still refuse a Polygon and an absent location with the message the mapping supplies.

Where the two halves of a binding key sit in a payload was a typed struct, so a Beckn shape change meant editing Go, rebuilding and redeploying every adapter on the network at once. oanbinding.BecknV2 is that shape as data, and From walks it. A deployment can override both paths, which exists for one situation: the spec moves a field and someone needs to track it without waiting for a release. It is a DEFAULT, not a setting, and the distinction matters. Where a binding key lives is a network convention -- every participant has to agree, or two adapters disagree about what a binding key even is and requests silently fail to match, with no error anywhere to say why. So absent means correct, and overriding is something an operator types deliberately. Both halves or neither, refused at startup: overriding one and leaving the other on the convention matches nothing, and would do so silently on every request. The walk understands two things: dotted segments, and [] meaning "this is an array, look in each element". No wildcards, no filters, no indices. Each of those would be another way to write something subtly wrong in configuration nobody reviews, to buy an expressiveness no payload shape has needed. It is an escape hatch, not a query language -- and it is 40 lines rather than a dependency, which for an escape hatch is the right trade. From now takes the paths and walks a generic document rather than unmarshalling a typed one. Every existing test runs against BecknV2, which is the regression guard: the default has to give exactly the answers the typed walk gave. Verified live. With the paths pointed somewhere the payload does not use them, the request stops matching and falls out of the pipeline as a 404 -- the unanswered-request guard catching the consequence, which is what proves the override is genuinely in effect rather than ignored.

The answer carried one resource holding every day under an "observations" field of its own invention. The WeatherObservation pack has no form for that: each of its examples carries a single validity and a flat parameters array, so a period is a resource. Minting a resource per day was tried before and reverted because it left the offer dangling -- the offer is echoed from the request, so its resourceIds still named the id that was asked for while the resources carried freshly derived ones. Rewriting those references is what makes the split safe, so the offer is $merge-ed rather than echoed and points at the days actually returned. Ids derive from the forecast date. They are new by design: the request names an abstract point forecast, the answer returns the concrete days that satisfy it.

The registry accepted "//get-daily" and the step joined it verbatim, so a typo in a registry row surfaced as a provider 404 three hops away with nothing naming the cause. The registry schema is tightened alongside this, but it is a separate deployable that may not be updated in step, so a row that slipped through has to fail here with something an operator can act on. A trailing slash is deliberately still accepted: "/api/" and "/api" are a distinction some APIs genuinely make, so stripping it would silently change the URL the operator published. An empty segment is the only case that is never deliberate.

The response context echoed bapId, bapUri, bppId and bppUri straight from the request. A mapping transforms a payload; asserting who the parties are is not its job, and the two Uri fields were only ever whatever the caller happened to send -- in a deployed stack a container-internal address that means nothing outside that network, republished as though it were ours. Identity on an answer is the signature the adapter puts on it, using the key the registry publishes for it. Nothing downstream reads these four from a response, and the provider path runs no response schema validation, so the shorter context changes nothing but what is claimed. The context keeps what correlates the answer to the request: version, action, networkId, transactionId, messageId and a fresh timestamp. The test asserted the echo; it now asserts the absence.

The Participant schema dropped keyId and use from a published key, stopped prefixing key material with "base64:", and replaced the BAP/BPP/NETWORK role enum with consumer/provider/network. This captures a real response from a registry running the new schema, so the plugin is pinned against what a registry writes today rather than only what one wrote in August. Two things it proves that were previously incidental rather than tested: a key with no encoding label arrives at the verifier byte-for-byte, and a key with no "use" still resolves -- had an absent use been read as "unknown, so refuse", every key the registry now writes would be unusable. The older capture stays exactly as it is. The registry is append-only, so a row written before this change keeps keyId, use and the label forever, and the plugin has to go on reading both shapes.

Two things in the answer were refused by base schema validation against the pinned LTS spec, found by validating a live response rather than the request: status.descriptor.code was QUOTED, and the spec's enum is DRAFT, ACTIVE and CLOSED. QUOTED read better and validated nowhere. DRAFT is also the honest value: a quote is a draft commitment, since nothing is committed until init and confirm. each resource lacked quantity, which Commitment.resources requires while the spec defines no quantity property and carries no Quantity schema at all. That defect is upstream, but the consequence is ours: an answer without it fails validation for any consumer who validates. One resource is one day's observation, so one. The resourceAttributes were already valid -- all three per-day resources validate against openagrinet:WeatherObservation v0.1 in Direct mode. The test asserted QUOTED, so it asserted a value the spec refuses; it now asserts DRAFT and that every resource carries a quantity.

Some upstreams take their token as a query parameter rather than a header or basic auth. The step could not do that: authScheme accepted none, basic and header only, so such a provider was unservable whatever the registry said. authScheme query adds it, named symmetrically with the header scheme -- queryName and queryValueEnv -- so the credential is still never in this config or in the registry, only the name of the variable holding it. Half a configuration is refused at startup, as the header scheme's is. It also redacts. Go quotes the whole URL in a transport error, so one unreachable host would otherwise write the token into the log at warn level: `Get "http://host/p?token=s3cr3t": dial tcp ...`. The retry path now replaces the value with REDACTED before logging or returning it. That is the reason this scheme is documented as the least safe of the four -- a query string is also logged by proxies, which nothing here can do anything about. Tests cover the parameter arriving alongside the mapped request rather than replacing it, the credential being absent from a failure's text, and a half-configured scheme being refused.

There was no line saying what was asked of a provider or what came back, so diagnosing one meant reading the mapping and inferring the call. Now every attempt logs the URL as it went on the wire, the status and the response size: upstream: GET http://host/v1/x?statecode=CG&token=REDACTED -> 200 OK, 2 bytes At info rather than debug, because this is the first question any provider problem raises and it should not need a log level change to answer. Logging a URL is only safe because the previous commit can remove a query-string credential from text, so redact is split: an error form and a string form, the latter used here. With any other scheme there is nothing in a URL to hide and the text passes through untouched.

manjudr

Review

Reviewed all three plugins and the core changes at e64a1fe.

Baseline: go build ./... and go vet ./... clean, full go test ./... passes, go mod tidy is a no-op. Coverage on the new packages is 88-100%. The commenting discipline is unusually good — most of what follows I found because a comment states an intent the code does not quite deliver.

The top findings were verified with throwaway tests against this branch rather than asserted from reading; those tests are not part of this review.

Blocking

upstream.call retries everything with no backoff — a 400 is retried 6x in 1.77ms; a missing credential is retried 4x and then reported as 502 NET_DOWNSTREAM_UNAVAILABLE; a cancelled request is retried 5x.
jsonmapper stops caching permanently once MaxCacheEntries distinct refs have been seen — expired entries are never purged and the cap then refuses every new ref. Verified: 5 requests to one new ref produced 5 fetches.
A withdrawn capability returns 500, not 404 — which contradicts the reasoning this PR adds to the no-route path in stdHandler.go.

Should fix

config/oan-provider-adapter.yaml sets cacheTTL: 60s with no cache: plugin, so caching is silently off and every message costs three registry round trips.
Response-leg mapping failures are classified 400, blaming the caller for the provider's answer.
hasProviderSteps is derived from config presence, not from the step being wired into steps: — a missing steps: entry 404s the whole module with no startup error.
A payload with several commitments naming the same capability passes binding derivation, then loses all but commitments[0] in the mapping.
The generated on_select is signed and sent without schema validation. Concrete case: if the provider omits location, JSONata drops the undefined values and coordinates becomes [] — an invalid Point, signed and delivered.

Nits

Stale "Mausamgram" naming in two places after the rename; an orphaned doc comment in weather/cmd/plugin.go; a blank line detaching the parseConfig comment in jsonmapper/cmd/plugin.go:21; oanregistry/cmd missing the var _ definition.RegistryLookupProvider = Provider assertion both other new plugins added; an ambiguous lookup cache key; | not rejected inside either half of a binding key; CONFIG.md not updated for the new mapper and providerSteps keys; the 27-line commented registry block duplicated 5x across the two sample configs; no single-flight in jsonmapper.compiled, so a just-expired popular mapping is fetched and compiled once per concurrent request; and no README for weather / internal/upstream while both other new plugins have a good one.

What is good

The registry plugin's security posture is genuinely careful: resolveStatus built as an allow-list because IsKeyStatusUsable is a deny-list, per-key status checked separately from participant status, cache entries re-validated on read because a shared cache outlives a deploy, the unclamped Retry-After clamp, and caching only usable results. Splitting ProviderRecordLookup from RegistryLookup and asserting the narrowing at startup in loadProviderStep is the right call. And the ackSigner/sendResponse pairing — signing exactly the bytes that will be written — is the subtle bug this PR could easily have shipped and did not.

manjudr · 2026-09-03T10:45:06Z

+		}
+		steps[c.ID] = step
+	}
+	h.hasProviderSteps = len(cfg.Plugins.ProviderSteps) > 0


hasProviderSteps keys off configuration, not wiring.

This is true as soon as providerSteps is present, but a provider step only executes if its id appears in the module's steps: list. Declare providerSteps and forget the steps: entry and the module 404s every single request while the step never runs — no startup error, no log line, and the 404s look exactly like the deliberate "nothing here serves that" case this flag exists to produce.

initSteps already rejects duplicate ids a few lines up; requiring that each configured provider step is actually referenced would be the same shape of check and would close this.

Leaving this one as it is for now. Every plugin behaves the same way — declare it and forget the steps: entry and it is skipped silently — so a check belongs at step registration where it would cover all of them, rather than here where it only covers provider steps. Worth doing, but as its own change.

Seven findings from the review at e64a1fe, in one commit because they are one pass over the same three packages. Two of them were things a comment claimed and the code did not do. RETRY CLASSIFICATION. The loop retried every non-nil error at full rate with no wait, so a 400 burned a retryMax of 5 inside two milliseconds and a missing credential was retried four times and then reported as 502 NET_DOWNSTREAM_UNAVAILABLE -- an operator's unset environment variable laundered into "the provider is down", which points the investigation at the wrong system. Failures no further attempt can change are now marked and not repeated: a request this step could not build, a credential it could not read, a response over the size cap, and any 4xx other than 429. 5xx and 429 still retry, because those are the provider asking to be. Between attempts there is exponential backoff from 50ms capped at 800ms, and a dead context breaks the loop before the call rather than after it. THE MAPPER STOPPED CACHING PERMANENTLY. cached() treated an expired entry as a miss but left it in the map and nothing ever deleted one, so the count only grew; once it reached MaxCacheEntries the cache refused every reference it was not already holding, for the life of the process. The comment said it "just pays to compile again next time" -- it paid every time, for every mapping the deployment had. Expired entries are now swept before the cap is measured. The test reproduces the reviewer's observation: five requests for a fourth reference produced five fetches before the fix and one after. A WITHDRAWN CAPABILITY REPORTED 500. ErrProviderRecordNotFound was wrapped in a plain fmt.Errorf, which lands in the unclassified 500 path, so "binding withdrawn" and "provider suspended" read as this adapter failing. It is a 404 now, with the sentinel still wrapped so errors.Is keeps matching. A registry that could not be consulted stays a 500, because that one is us. A RESPONSE-LEG MAPPING FAILURE BLAMED THE CALLER. Both directions returned 400 SCH_SCHEMA_ADAPTATION_FAILED, justified as the caller's payload being wrong. That holds for the request leg. On the response leg the input is the provider's answer, so a provider that changed shape sent the caller off to fix a request that was fine. The response leg is a 502. SEVERAL COMMITMENTS WERE HALVED. A payload naming the same provider and capability across N commitments resolved to one binding key without complaint, and the mapping then read commitments[0] -- so the caller received a confident, signed, spec-valid answer to part of what it asked. Refused, with the count and the advice to send them separately. The test that asserted the old behaviour is replaced by its inverse. A TTL THAT CACHED NOTHING. config/oan-provider-adapter.yaml set cacheTTL with no cache plugin, so caching was silently off and every message made three registry calls inside signature validation's budget -- while the commented block this PR added to the bpp sample warns about exactly that. The TTL is commented out with the reason, and the registry plugin now says so at startup when a TTL is set without a cache, so it cannot recur quietly. AN INVALID POINT, SIGNED. If the provider answered without echoing its location, JSONata dropped the undefined values and the mapping emitted "coordinates": [] -- an invalid Point, signed and delivered. It now emits location only when both coordinates exist. Absent is honest; empty is a lie in the shape of an answer. Also the two stale Mausamgram names after the rename to weather, and an orphaned doc comment that documented var Provider from 25 lines away. Not taken, both by decision: requiring every declared providerStep to appear in steps: -- declare-without-wiring is how every plugin behaves and the resulting 404 is truthful, though a startup line naming which are wired would have saved some debugging. And validating the generated response before signing it, which touches the handler pipeline and is its own change; the one demonstrated case it would have caught is fixed above in the mapping instead.

Refusing multi-commitment payloads was right and its status was not: the error from oanbinding.From was unclassified, so it landed in the 500 path and the caller got NET_INTERNAL_ERROR with the reason only in this process's log. That is the same fault the review raised about a withdrawn binding, in the fix for it. Found by running the live stack rather than the unit tests, which asserted the refusal without looking at the status. Everything From refuses is a statement about the payload -- unreadable JSON, or a request naming more than one call -- so 400, carrying the message that says which.

Two things the step got wrong about a provider's answer. ONLY 200 COUNTED AS SUCCESS. A provider is entitled to answer 202 for work it accepted, 201 for something it created, or 204 for nothing to report, and all three were treated as failures -- so a perfectly good exchange was refused on the status line. Any 2xx is an answer now. 3xx never reached here: the client follows redirects. THE RESPONSE BODY WAS THROWN AWAY ON FAILURE. It was read, its length logged, and then discarded, so a failure reported "provider returned 400 Bad Request" and nothing about why. That is the first thing an operator needs, and the thing that makes a real provider's behaviour observable at all -- Agmarknet reports "no data" in the body of a 400, which until now was invisible. A failure now quotes the body: whitespace collapsed so an indented JSON or an HTML error page does not spread one failure across forty log lines, truncated at 300 characters so a page of HTML does not end up in a NACK, and "(no body)" when there is nothing to quote. It goes through the same redaction as everything else, which a test covers by having the provider echo the query string back -- credential and all -- and asserting it does not survive. One behaviour is now asserted rather than left to be discovered: a 204 passes the status check and then fails decoding, because there is no JSON to map, and the error says the body could not be read rather than blaming the status. If a real provider uses 204 for "nothing to report", that is the line that needs a decision.

…eam samples [#1] Two things, both about where an explanation belongs. REVERTS THE BECKN-ONE SAMPLES. 5ef4cbd added a commented-out oanregistry block to config/local-beckn-one-bap.yaml and -bpp.yaml, 135 lines across the two. They are upstream beckn-onix example configs and our fork has no business annotating them: the note is about an OAN plugin, so it belongs in the OAN config, which is what the rest of this commit makes it worth reading. Pure deletion -- the additions were comments, so nothing changes behaviour. DOCUMENTS THE OAN CONFIG PROPERLY. Every option the three plugins actually read, checked against the code rather than remembered: oanregistry url (required, no default), entity, providerEntity, timeout, retry_max, retry_wait_min, retry_wait_max, cacheTTL jsonmapper fetchTimeout, cacheTTL, negativeTTL, maxMappingBytes, maxCacheEntries -- all optional, defaults stated provider step bindingKeys (required), providerIdAt, capabilityCodeAt, authScheme and the env-var keys for each of basic, header and query, maxResponseBytes And, more useful than any of them, WHAT IS NOT CONFIGURED HERE. The call plan -- method, path, mappings, timeoutMs, retryMax -- is the registry's ProviderSchema row, read per request. So repointing a provider or giving a slow one longer is a registry write, not an edit here and a restart. Retry classification is not configurable at all: 4xx permanent, 5xx retried, backoff 50ms rising to 800ms. Also adds schemaValidator and validateSchema, which the deployed configs have had for a while and this reference lacked, and mounts at / rather than /beckn/ to match them. Fixes a comment that was simply wrong: it said which action a request is "comes from the payload's context.action, never from the URL". It is the other way round. Routing strips the mount path and matches what remains; the validator is the sole exception, reading context.action and ignoring the path it is handed. Nothing reconciles the two. Verified by booting this config in the published image: all seven plugins load, the module registers at /, and the server listens. The documented default binding paths are quoted from oanbinding.BecknV2 including the "[]" array markers -- without them the path matches nothing, which is exactly the kind of error a reference config should not teach.

@context

#1] The response half hardcoded the pack URL, so the mapping had to know which one is current and could contradict what the request actually declared. It reads it off the incoming select now -- the payload always carries @context and @type on each resource, so there is nothing to invent. Backticks because @ is an operator in JSONata. Verified by running the real response mapping against a real mock upstream response and a real select: three resources out, @context matching the request in both cases tried -- the old schemas.openagrinet.global identifier and the GitHub pack URL.

] beckn.org came from the upstream sample and nothing here fetches from it. An allowlist earns its keep by listing what is actually used, so it is raw.githubusercontent.com alone -- where the schema packs are published.

manjudr · 2026-09-07T05:58:58Z

+                     consumer who validates. */
+                  "quantity": 1,
+                  "resourceAttributes": {
+                    "@context": "https://schemas.openagrinet.global/schema/WeatherObservation/v0.1/context.jsonld",


Please don't hard code the value, get the context and value field values from the request and replace it or Define it as a constant and read it from the config

manjudr · 2026-09-07T06:02:34Z

+        keyManager:
+          id: simplekeymanager
+          config:
+            subscriberId: provider-network-vistaar.da.gov.in


please define it as a placeholder ex:

manjudr · 2026-09-07T06:24:06Z

+            type: url
+            location: "https://raw.githubusercontent.com/beckn/protocol-specifications-v2/refs/tags/core-v2.0.0-lts/api/v2.0.0/beckn.yaml"
+            cacheTTL: "3600"
+            extendedSchema_enabled: "false"


Enable the L2 validation

manjudr · 2026-09-07T06:28:32Z

+        # rather than calling the provider unauthenticated.
+        # ------------------------------------------------------------------
+        providerSteps:
+          - id: weather


Keep it as capability

manjudr

Requesting changes

This is a well-built PR, and I want to lead with two things I verified rather than assumed:

The tests are real. ~131 assertions across 55 tests in upstream_test.go, ~122 across 38 in oanregistry_test.go, table-driven, with named regression tests for the instrumentor copy-back, signing the answer vs the generated ACK, duplicate provider-step ids, and cache-cap thrash. For ~5,500 lines of new test code that is a genuinely strong suite, not decoration.

It builds and passes. I cloned the branch and ran it, because CI doesn't (see below): go build ./..., go vet ./..., go test ./core/... ./pkg/..., and go test -race over core plus the three new plugin trees are all clean. The function main is undeclared in the main package lines are the pre-existing -buildmode=plugin pattern — they appear for cache/registry/signer/router too — not a regression.

Scope is tight, which I appreciate on an 11k-line PR. The only pre-existing files touched are core/module/handler/*, pkg/model/model.go, pkg/plugin/{manager.go,definition/*} and install/build-plugins.sh, and all of it is plumbing the new plugins genuinely need. One duplication note inline (servedActions).

Detailed findings inline. Summarising what needs action.

Blockers

oanbinding.go:59 — the multi-commitment guard doesn't do what its own comment says. It counts provider-id values resolved, not commitments, and walk returns nothing for a missing or non-string leaf. A two-commitment payload whose second commitment omits offer.provider.id passes the guard, resolves off commitments[0], and serves half the request — verbatim the "confident, signed, spec-valid answer to part of what it asked" the comment exists to prevent. The tests only cover the single-commitment case, so the suite can't catch it.
oanregistry.go:356 — ambiguous cache key on the signing-key path. oan_lookup_%s_%s joined on _ collides for participants a/key b_c and a_b/key c, and cached() re-checks shape but never identity. Inbound signatures claiming to be one participant then verify against the other's public key. Latent while cacheTTL is 0, arms the moment caching is enabled.

Should fix before merge

oanbinding.go:60 + upstream.go:266 — arity refused before ownership, so one provider step 400s multi-commitment payloads addressed to a different step. As written the documented pass-through dispatch only works with exactly one provider step configured.
providerrecord.go:328 — unbounded io.ReadAll on the per-request signature-validation path, against a plain-HTTP registry. Sibling code in this PR (upstream.go:588, jsonmapper.go:487) correctly uses io.LimitReader.
upstream.go:602 — up to 300 bytes of the provider's body, and internal env-var names, are echoed to the network caller in the NACK. providerrecord.go:335 already gets this right and explains why.
responsestep.go:52 — the mapped response is never required to be a JSON object, so a scalar JSONata result ships as a signed 200 application/json.
upstream.go:480 — registry-supplied retryMax/timeoutMs have no ceiling; one row can pin a goroutine and an inbound connection for ~17 hours per request.
stdHandler.go:695 — hasProviderSteps comes from declaration rather than the executed step list, so a declared-but-unlisted provider step puts the whole module into 404 mode.

Lower severity

upstream.go:541 (backoff shift overflows past attempt 59 → tight retry loop), :498 (redact discards the wrap chain, breaking errors.Is downstream), :565 (method not normalised, so post → 405 → a 502 blaming the provider), :720 (baseURL never validated; a # in path silently drops the query string and produces a plausible wrong answer), jsonmapper.go:312 (no single-flight; at the cache cap every request re-fetches the mapping over HTTP indefinitely).

Separately — this PR has no CI validation at all

Not a defect in the diff, but it's why I ran the toolchain by hand, and it's worth fixing alongside:

All three Go workflows on development gate on upstream branch names that don't exist in this flow — ci.yml on [beck-onix-v1.0-develop, beck-onix-v1.0], beckn_ci.yml and beckn_ci_test.yml on [beckn-onix-v1.0-develop]. A PR into development matches none.
Even if beckn_ci.yml fired, it sets APP_DIRECTORY: "shared/plugin", while these plugins live under pkg/plugin/implementation/ — so it would test the wrong tree and pass vacuously.
build-and-deploy-plugins.yml is workflow_dispatch-only, so the new plugins have no automated build path either.
The one check that does run, "Terraform Plan Only", fails on pre-existing Gerrit auth (git clone … UNAUTHENTICATED), unrelated to this PR.

So +11,205 lines are landing with no automated proof they compile. PR #14 adds proper test/security workflows but gates them on main only, so it doesn't close this gap — adding development to those filters would.

Findings 1 and 2 are the ones I'd hold the merge on; the rest are straightforward. Happy to re-review quickly once those are addressed.

manjudr · 2026-09-07T07:04:48Z

+	// dropped, leaving the caller a confident, signed, spec-valid answer to
+	// part of what it asked. One request maps to one call, so several is a
+	// request this design cannot express and is refused rather than halved.
+	providerValues := valuesAt(payload, paths.ProviderID)


Blocker — this guard doesn't do what the comment above it says it does.

providerValues := valuesAt(payload, paths.ProviderID) if len(providerValues) > 1 { ... refuse ... }

This counts provider-id values successfully resolved, not commitments. And walk (paths.go:63) returns nil for a leaf that is missing or not a string:

if value, ok := node.(string); ok { return []string{value} } return nil

So for a payload with two commitments where the second omits offer.provider.id — or carries it as a number or an object — len(providerValues) == 1. The guard passes, distinct() collapses to one provider, the binding resolves off commitments[0], and the mapping serves only that one.

That is verbatim the failure the comment at lines 53-58 says this check exists to prevent: "leaving the caller a confident, signed, spec-valid answer to part of what it asked."

The test suite structurally cannot catch it — oanbinding_test.go:70-75 covers the missing-provider case only with a single commitment, so the two-commitment-one-resolvable shape is never exercised.

Suggested fix: count the commitments themselves, or compare the commitment count against the resolved-value count and refuse on mismatch — that way a commitment the paths can't read is a refusal rather than a silent drop.

manjudr · 2026-09-07T07:04:48Z

+	// part of what it asked. One request maps to one call, so several is a
+	// request this design cannot express and is refused rather than halved.
+	providerValues := valuesAt(payload, paths.ProviderID)
+	if len(providerValues) > 1 {


Arity is refused before ownership is determined, which breaks the documented dispatch model.

upstream.go:266 maps any non-ErrNoBinding error to a hard 400:

binding, err := oanbinding.From(s.paths, ctx.Body) if errors.Is(err, oanbinding.ErrNoBinding) { return nil } if err != nil { // -> 400

But the design comment at upstream.go:262 states: "several provider steps sit in one pipeline and each recognises its own work, so adding a provider is one more entry rather than a change to a routing table."

Those two don't compose. With weather plus a second provider step in steps, a legitimate two-commitment request addressed to the second provider is 400'd by weather before the second step ever runs. Same for a single commitment carrying two resource @types (line 77).

A binding that isn't this step's work should return nil regardless of arity — check serves() before refusing arity, or return ErrNoBinding when no configured key matches. As written, the pass-through dispatch only works while exactly one provider step is configured.

manjudr · 2026-09-07T07:04:48Z

+		return nil, nil
+	}
+
+	cacheKey := fmt.Sprintf("oan_lookup_%s_%s", req.SubscriberID, req.KeyID)


Blocker — ambiguous cache key on the signing-key path, and cached() never re-checks identity.

fmt.Sprintf("oan_lookup_%s_%s", req.SubscriberID, req.KeyID) joins on _, which can legitimately appear in either field. And cached() (line 575) validates only shape, never identity:

if len(results) != 1 || results[0].Status == "" {

So participants a (key b_c) and a_b (key c) both produce oan_lookup_a_b_c. Whichever is looked up first wins the cache entry, and inbound signature validation then verifies requests claiming to be a_b against a's public key. Anyone able to register a participant id containing an underscore gets an impersonation path.

Two fixes, both worth doing:

Use a separator that cannot appear in either field (or length-prefix / hash the parts).

Re-check the cached entry's subscriber and key against the request in cached() — the function already argues that a shared cache "outlives a deploy and can hold entries written by another version" and re-validates Status for exactly that reason. Identity deserves the same treatment, and it's the part with security consequences.

Latent today, since cachingEnabled() requires cacheTTL > 0 and the default is 0 — but it arms the moment an operator opts into caching, which is the normal production path.

manjudr · 2026-09-07T07:04:48Z

+	}
+	defer resp.Body.Close()
+
+	respBody, err := io.ReadAll(resp.Body)


Unbounded io.ReadAll on the per-request signature-validation path.

Every other new read in this PR caps itself — upstream.go:588 and jsonmapper.go:487 both use io.LimitReader with an explicit maximum. This one doesn't.

It matters more here than in most places: this runs inside validateSign on every inbound request, against a registry URL that the sample config sets to a plain http://registry:8081. A registry bug or a MITM answering with a multi-GB body OOMs the adapter, and it's the most attacker-reachable read in the module.

respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxSearchResponseBytes+1))

(Consistency note: registry/registry.go has the same unbounded pattern, so there's some precedent in the tree — but the new code in this PR is otherwise careful about it, so this reads as an oversight rather than a house style.)

manjudr · 2026-09-07T07:04:48Z

+
+// servedActions lists the actions a plan covers, sorted so the same record logs
+// the same way twice.
+func servedActions(plan *model.ProviderRecord) []string {


Scope / duplication: servedActions is duplicated verbatim from upstream.go:407.

Two copies of a helper whose whole purpose is that "the same record logs the same way twice" — they now have to agree on sort order across two packages, and one will drift. Worth hoisting into a shared internal package, especially since both callers are new in this PR.

manjudr · 2026-09-07T07:04:49Z

+	attemptCtx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
+	req, err := http.NewRequestWithContext(attemptCtx, call.Method, endpoint, requestBody(call.Method, mapped))


hasBody does strings.ToUpper(method), which implies the method is case-insensitive — but http.NewRequestWithContext transmits it verbatim.

A registry row with method: "post" therefore sends post /path HTTP/1.1. The body is attached correctly (because hasBody normalised), but nginx and most gateways answer 405, which is classified permanent and surfaces as a 502 "provider did not answer" — blaming the provider for a registry typo.

Normalise the method when building the request, or validate it against GET/POST at read time so the error names the real cause.

manjudr · 2026-09-07T07:04:49Z

+	// separator appears between them. The trim is belt and braces: the registry
+	// refuses a trailing slash on baseUrl, and this keeps a row that predates
+	// that from producing a doubled one.
+	endpoint := strings.TrimSuffix(baseURL, "/") + call.Path


verifyPath carefully validates the path half, but nothing validates baseURL — no scheme or host check, unlike jsonmapper.verifyFetchable — and path isn't checked for .. or #.

Two concrete consequences:

a row with baseUrl: "registry:8081" (scheme omitted) fails inside http.NewRequest and is reported as a 502 provider did not answer after 1 attempts: could not build the request — a config error dressed as a provider outage

a path containing # silently turns the mapped GET query string into a URL fragment, so the provider receives no parameters and answers with wrong data rather than an error

The second is the worse one: it fails silently and produces a signed, plausible answer to a query the provider never saw.

manjudr · 2026-09-07T07:04:49Z

+
+// compiled returns the compiled mapping for a reference, fetching and compiling
+// it on first use. A failure is cached too, for a shorter time.
+func (m *Mapper) compiled(ctx context.Context, mappingRef string) (cacheEntry, error) {


compiled() has no single-flight around fetchAndCompile.

On a cold start, N concurrent requests for the same mapping each make the HTTP fetch and each pay the compile. The comment acknowledges the duplicate-compile cost, but the HTTP round trip is the bigger one and isn't mentioned.

The sharper problem is at the cap: once len(entries) >= MaxCacheEntries, remember()'s cap-refusal (line 366) means every request for an unheld ref re-fetches the mapping over the network on the request path, indefinitely. That turns a cache-sizing mistake into a permanent per-request HTTP dependency rather than a slow path that eventually warms.

golang.org/x/sync/singleflight around the fetch, or an LRU eviction instead of a hard cap refusal, would fix it.

manjudr · 2026-09-07T07:04:49Z

+}
+
+// writeJSONResponse writes body as a 200 JSON response, reporting what it wrote.
+func writeJSONResponse(ctx context.Context, w http.ResponseWriter, body []byte) []byte {


A scalar JSONata result ships as a signed 200 application/json.

serve (line 48) only checks len(ctx.ResponseBody) > 0, and this function then sets Content-Type: application/json with a 200 over whatever bytes arrived.

So a mapping whose response half is $.response.temperature produces 28.5, and the adapter answers 200 application/json with body 28.5. ackSignerStep then signs it, and the caller receives a signed non-envelope. Nothing errors anywhere.

Given the reasoning elsewhere in this PR that "returning the provider's own shape instead would be worse than failing", the mapped response should at minimum be required to decode as a JSON object before it's written and signed.

manjudr · 2026-09-07T07:04:49Z

+		}
+		steps[c.ID] = step
+	}
+	h.hasProviderSteps = len(cfg.Plugins.ProviderSteps) > 0


hasProviderSteps is derived from declaration, not from what actually runs:

h.hasProviderSteps = len(cfg.Plugins.ProviderSteps) > 0

The resulting 404 guard (line 241) is module-wide, while dispatch is per-request. So an operator who pre-declares a soil provider step for a capability that isn't live yet — and correctly leaves it out of cfg.Steps — puts the whole module into 404 mode. Nothing about soil runs, but any action that legitimately terminates on the publisher path (no route, no answer) stops ACKing and starts returning NET_ENTITY_NOT_FOUND.

The config comment already acknowledges the declared-but-unlisted case as legitimate, so this should be computed from the provider steps that actually made it into h.steps.

manjudr

Follow-up review: the three standing checks

Reviewing again against the defaults you asked for — no hardcodes (placeholder + example, read from config), proper logs, no unused methods or logic. These are additions to my earlier review; the two blockers there (oanbinding.go:59 commitment arity, oanregistry.go:356 cache key) still stand.

1. No unused method or logic — clean

I swept mandi, weather, internal/upstream, internal/oanbinding, oanregistry, jsonmapper and their cmd packages for unexported functions with no production caller, config fields never read, and consts never referenced. Nothing real. The two candidates my scan raised both dissolved on checking:

Default{RetryMax,RetryWaitMin,RetryWaitMax} are consumed in oanregistry/cmd/plugin.go:23-25; my scan was package-scoped and missed it. The declaration comment already explains they are exported to be the single source of truth for cmd.
jsonmapper.cachedCount() is a documented test accessor, which is a legitimate pattern.

No dead code, no speculative knobs, no orphaned helpers. Reporting this as a pass rather than inventing findings.

2. Proper logs — one security bug, one gap

The logging style is right: log.Debugf/Infof/Warnf(ctx, ...) matching the repo's house convention, with levels chosen deliberately (debug for pass-through, info for what was asked and answered, warn for a failed attempt). No fmt.Println, no logging in tight loops, no logged payload bodies.

But one finding is a blocker, inline at upstream.go:706:

redactString matches the raw env value while authenticate writes the credential through query.Encode(). Any token containing +, /, = or a space — every standard-base64 token — is not redacted, and goes to the log in recoverable percent-encoded form at info level on every successful call. redact() has the same hole at warn. I verified this by running the real authenticate -> redactString sequence; output quoted inline.

The existing test passes only because its token is s3cr3t, whose encoded and raw forms are identical — the one input class that cannot show the bug.

The gap: oanbinding has zero log calls across 205 lines despite being the component that rejects traffic, so refusals are unattributable to a subscriber or transaction. Inline at oanbinding.go:74.

3. No hardcodes — two committed environment values

Two values in oan-provider-adapter.yaml are deployment-specific but committed as literals, inline above:

:62 subscriberId: provider-network-vistaar.da.gov.in — a real network identity, and duplicated at :113. This is what signatures are checked against, so an operator who edits one occurrence and misses the other signs as the wrong subscriber.
:81 url: http://registry:8081/api/v1 — a Compose service name and port that resolves in exactly one deployment topology.

Both want the pattern you asked for: a ${VAR} placeholder with the working value kept as a commented example.

Credit where due — the auth scheme itself already does this correctly. queryValueEnv: MANDI_TOKEN reads the secret from the environment rather than committing it, and lines 217-232 document each scheme with examples. The request is to extend that same discipline to the identity and endpoint values.

The magic numbers I checked are fine: explainLimit = 300 and DefaultMaxCacheEntries = 200 are named, defaulted, and overridable — the correct pattern, not hardcodes.

manjudr · 2026-09-07T07:25:45Z

+	if value == "" {
+		return text
+	}
+	return strings.ReplaceAll(text, value, "REDACTED")


The credential redaction misses any token that percent-encodes, so the token reaches the log in recoverable form at info level.

redactString matches the raw env value:

value := os.Getenv(s.config.QueryValueEnv) return strings.ReplaceAll(text, value, "REDACTED")

But authenticate puts the credential into the URL through query.Encode() (line 666), which percent-encodes it. For any token containing +, /, = or a space — i.e. every standard-base64 token — the string in req.URL.String() is not the string being searched for, so ReplaceAll matches nothing.

I ran the real production sequence (authenticate -> req.URL.String() -> redactString) against the actual code:

tok = s3cr3t (the existing test's token) wire = ...?statecode=CG&token=s3cr3t logged = ...?statecode=CG&token=REDACTED OK tok = YWJjZGVm+Zm9vL2Jhcg== (realistic base64) wire = ...?statecode=CG&token=YWJjZGVm%2BZm9vL2Jhcg%3D%3D logged = ...?statecode=CG&token=YWJjZGVm%2BZm9vL2Jhcg%3D%3D LEAK tok = "tok en/with+chars=" wire = ...?statecode=CG&token=tok+en%2Fwith%2Bchars%3D logged = ...?statecode=CG&token=tok+en%2Fwith%2Bchars%3D LEAK

The leaked form is trivially reversible (%2B -> +, %3D -> =), so this is a full credential disclosure, not an obfuscated one.

Two things make it worse than a one-line bug:

It lands at info, on every successful call (line 592), not on a rare error path. So the token is written to the log continuously in normal operation, wherever those logs ship.

redact() has the same hole at warn (line 499, attempt %d/%d failed: %v). Its doc comment is explicit that this is the case it exists to prevent: "one unreachable host writes the credential into the log at warn level." Go's transport errors quote the URL in its encoded form, so they evade the replacement too.

TestRedactStringRemovesTheCredentialFromTheURL (line 568) passes only because its token is s3cr3t — alphanumeric, so Encode() is a no-op and the raw and encoded forms coincide. The test asserts the right property against the one input class that cannot exhibit the bug.

Fix by redacting structurally rather than by string match — operate on the parsed query, so encoding never enters into it:

u, err := url.Parse(text) if err == nil && u.Query().Has(s.config.QueryName) { q := u.Query() q.Set(s.config.QueryName, "REDACTED") u.RawQuery = q.Encode() return u.String() }

For redact() on error text, where the URL is embedded in prose and may not parse, match both forms — value and url.QueryEscape(value).

Worth adding a base64 token (+, /, =) as a test case; it fails against the current code and is the realistic shape for the MANDI_TOKEN this scheme was built for.

manjudr · 2026-09-07T07:25:45Z

+		// its own business.
+		query := req.URL.Query()
+		query.Set(s.config.QueryName, value)
+		req.URL.RawQuery = query.Encode()


Context for the redaction finding at line 706: this is where the encoded form originates.

query := req.URL.Query() query.Set(s.config.QueryName, value) req.URL.RawQuery = query.Encode()

Encode() percent-encodes value, so from here on the credential exists in the URL in a form that a strings.ReplaceAll on the raw env value will not match. The Set-not-Add reasoning in the comment above is right; the issue is only that the redaction downstream assumes the value survives unchanged.

manjudr · 2026-09-07T07:25:45Z

+      plugins:
+        # ------------------------------------------------------------------
+        # oanregistry -- the OAN Registry (SunbirdRC) client.
+        #


Hardcodes: subscriberId: provider-network-vistaar.da.gov.in is a real deployment's identity committed into the reference config, and it is repeated at line 113.

This is the one value that is guaranteed to differ per deployment — it is the participant's network identity, and it is what signatures are checked against. Committing a specific one means every operator must find and edit two occurrences, and any who miss one get a config that signs as somebody else's subscriber.

Please make it a placeholder read from the environment, with the example alongside it, e.g.:

# The participant identity this adapter signs as. Must match the registry entry. # Example: provider-network-vistaar.da.gov.in subscriberId: ${OAN_SUBSCRIBER_ID}

and state it once rather than twice, or derive line 113 from the same value.

manjudr · 2026-09-07T07:25:45Z

+            # The two entity names, both defaulted. Only worth setting if a
+            # deployment renamed the schemas.
+            entity: Participant               # default: Participant
+            providerEntity: ProviderSchema    # default: ProviderSchema


Hardcodes: url: http://registry:8081/api/v1 bakes a Compose service name and port into committed config.

registry resolves only inside a Docker network that happens to name the service that way — it will not resolve in Kubernetes, in a local run, or against a hosted registry. Same treatment as the subscriber id: a placeholder plus a documented example.

# Registry base URL. Example (Compose): http://registry:8081/api/v1 url: ${OAN_REGISTRY_URL}

Since plain http is also being used for a signing-key lookup, worth noting in the comment that this must be TLS anywhere the registry is not on the same host — the keys fetched over this URL are what ACK verification trusts.

manjudr · 2026-09-07T07:25:45Z

+		return Binding{}, ErrNoBinding
+	}
+	if len(providers) > 1 {
+		return Binding{}, fmt.Errorf("oanbinding: payload names %d providers (%s); one request maps to one call",


Logs: oanbinding is 205 lines with no logging at all, and it is the component that rejects traffic.

All four refusal paths (lines 50, 61, 74, 78) return an error and nothing else. The returned text is good — it names the counts and the offending values — but there is no log line, so if the handler that consumes this error does not log the full chain, a rejected payload leaves no trace of which payload it was: no subscriber id, no transaction id, no message id.

That is the difference between "provider X's integration is sending two commitments" and an unattributable error count. Compare internal/upstream, which logs the binding key at both debug and info for exactly this reason.

A single log.Warnf(ctx, ...) at the refusal points, carrying the transaction/message id, would make these diagnosable. That needs a ctx on From — worth doing, since these are the errors an operator will actually be paged about.

manjudr

Spec-alignment review: `WeatherObservation` v0.1

Reviewed the mapping against the actual schema pack in OpenAgriNet/network-specs (branch schema-packs-v0.1, schema/WeatherObservation/v0.1/), and validated the mapping's real output shapes against the pack's own subschemas with a JSON Schema validator. Two blockers, both empirically confirmed and quoted inline.

1. The `required:` guard demands a field the pack forbids — every conformant request is refused

WeatherObservation OnDemand does not leave location optional; its not/anyOf clause excludes it, along with validity, parameters, observationType, source, generatedAt, observedAt and modelRunAt.

payload satisfying the mapping's Point-location guard   -> VIOLATES PACK
the pack's OWN OnDemand example (coverageAreas)         -> CONFORMS

The adapter therefore rejects the pack's own example with "this capability needs a Point location", and every payload it does accept is one a conformant validator would refuse. The request half reads $ra.location.coordinates from the same forbidden field, so this is structural rather than cosmetic.

The pack's mechanism for OnDemand geography is coverageAreas, whose items are oneOf [AdministrativeAreaReference, CompleteGeoJSONGeometry] — a Point geometry fits there — with geographicGranularities stating the granularity. Suggested rewrite inline at :83.

2. `aggregation` invalidates every parameter entry, and the comment claiming otherwise is wrong

The comment at :47-52 says "The pack sets no additionalProperties, so it validates." The pack sets additionalProperties: false on parameters.items:

pack allows keys = ['parameter', 'unit', 'value']
$reading('Temperature','Maximum','Cel',31.4)  -> INVALID  ('aggregation' was unexpected)
$alert('Heavy rain expected')                 -> VALID

All six $reading call sites pass a non-empty aggregation, so every rainfall/temperature/humidity/wind entry is invalid — i.e. every normal response. The need is real (the provider reports min and max), but the fix belongs in the pack: propose aggregation as an optional enum, or get distinct parameter values added. Detail and options inline at :48.

3. Fewer hardcodes — and one place that needs more

You asked for fewer hardcodes; on this file the specific ones are mostly defensible, and one runs the other way:

@context (:164) should be stated, not echoed. Right now @type is hardcoded while @context is taken from the caller, though the pack pins both together in x-jsonld. Echoing reflects a caller's stale or absent context back inside a response the adapter signs. Inline.
"subjectCategories": ["Weather"] (:237) is correct as a literal — it is a pack enum value ([Crop, Livestock, Weather, Market, Scheme, Practice]) and a property of the capability, not of the deployment. Keep it.
unit values ("Cel", "mm", "%", "m/s", "1") are correct as literals — they describe what this provider actually returns, and "1" for unitless is what the pack prescribes.

The values genuinely worth moving to configuration are the deployment ones I flagged separately (subscriberId, the registry URL), not these. Worth stating the rule explicitly somewhere: pack constants belong in the mapping; deployment values belong in config. By that rule this file is close to right once @context moves to the stated side.

Verification

Schema facts above come from schema/WeatherObservation/v0.1/attributes.yaml and schema/AgricultureResource/v0.1/attributes.yaml at 200d963, checked with jsonschema (Draft 2020-12) against the pack's own conditional clauses and the self-contained parameters.items subschema. No external $ref was stubbed or approximated for any result quoted here.

manjudr · 2026-09-07T07:36:42Z

+  - check: |
+      (
+        $ra := beckn.message.contract.commitments[0].resources[0].resourceAttributes;
+        $exists($ra.location) and $ra.location.type = "Point"


Spec alignment (blocker): this guard requires a field the pack forbids, so every conformant request is refused.

The WeatherObservation v0.1 OnDemand branch does not merely leave location optional — it excludes it:

- if: properties: { informationMode: { const: OnDemand } } required: [informationMode] then: required: [supportedObservationTypes, supportedParameters, geographicGranularities] not: anyOf: - required: [observationType] - required: [source] - required: [location] # <-- here - required: [generatedAt] - required: [observedAt] - required: [modelRunAt] - required: [validity] - required: [parameters]

A select payload is OnDemand, so this guard demands $ra.location from a resource that is not allowed to have it. I validated both directions against the pack's own conditional clause:

payload satisfying this Point-location guard -> VIOLATES PACK ("location" must not be present in OnDemand mode) the pack's OWN OnDemand example (coverageAreas) -> CONFORMS

So the adapter rejects the pack's own example with "this capability needs a Point location", and the only payloads it accepts are ones a conformant validator would refuse. The request half at lines 112-113 reads $ra.location.coordinates from the same forbidden field.

The pack's mechanism for "where" in OnDemand mode is coverageAreas, and it accepts a geometry, not just an area code:

coverageAreas: items: oneOf: - $ref: ".../AdministrativeAreaReference" - $ref: ".../CompleteGeoJSONGeometry" # <-- a Point fits here

Alongside it, geographicGranularities: [Point, Village, Block, District, State] is how a caller states the granularity it wants. So the spec-aligned version of this guard is roughly:

$point := $ra.coverageAreas[type = "Point"][0]; $exists($point) and $exists($point.coordinates)

with the request half reading $point.coordinates[1] / [0].

Worth confirming the intended direction with the spec owners before rewriting — but as it stands, location on a select payload is not a spec-optional field, it is a spec violation, and the mapping is built on it.

One genuine spec gap to raise rather than fix here: the comment at lines 97-100 suggests adding $ra.validity.startsAt/endsAt for a date range. validity is on the forbidden list too, and OnDemand offers only forecastHorizon — which declares what the provider can do, not what the caller wants. There is currently no conformant way for an OnDemand request to name a requested window. That needs a pack change, not a mapping change.

manjudr · 2026-09-07T07:36:42Z

+# that appears nowhere in the answer.
+#
+# ONE FIELD HERE IS NOT IN THE PACK, deliberately. The pack sets no
+# additionalProperties, so it validates; it is simply not governed.


Spec alignment (blocker): this claim is factually wrong — the pack does set additionalProperties: false, so aggregation invalidates every reading.

The comment states:

ONE FIELD HERE IS NOT IN THE PACK, deliberately. The pack sets no additionalProperties, so it validates; it is simply not governed.

But WeatherObservation/v0.1/attributes.yaml closes parameters.items explicitly:

parameters: items: type: object required: [parameter, value, unit] properties: { parameter: ..., value: ..., unit: ... } additionalProperties: false # <-- set, and false

Validated against that exact subschema:

pack allows keys = ['parameter', 'unit', 'value'] $reading('Temperature','Maximum','Cel',31.4) -> INVALID Additional properties are not allowed ('aggregation' was unexpected) $alert('Heavy rain expected') -> VALID same reading without 'aggregation' -> VALID

This is not a benign ungoverned extra. All six $reading call sites (lines 260-265) pass a non-empty aggregation — "Total", "Minimum", "Maximum", "Average" — so every rainfall, temperature, humidity and wind entry is invalid. Only the $alert entries pass. In practice that means every normal mausamgram response fails pack validation.

The underlying need is real and the comment identifies it correctly: the provider reports min and max for temperature and humidity, and parameter/value/unit alone cannot distinguish them. But the pack has to be the one to solve it. Three options, in the order I would prefer them:

Propose aggregation to the pack as an optional enum on the parameter entry (Total, Minimum, Maximum, Average). This is the honest fix — the field is genuinely needed and the spec repo is right here.

Use distinct parameter values if the pack will extend that enum (TemperatureMin / TemperatureMax), which keeps entries closed and self-describing.

Drop aggregation and emit only what the pack allows, accepting the information loss until (1) lands.

What should not ship is a resource the adapter signs and publishes as openagrinet:WeatherObservation while it fails that pack's validation — especially with a code comment asserting the opposite. Please also correct the comment, since it is the reason this was believed safe.

manjudr · 2026-09-07T07:36:42Z

+       mapping that hardcodes it has to be reissued whenever the pack URL
+       moves, and it can disagree with what the request actually declared.
+       Backticks because @ is an operator in JSONata. */
+    $ctx := beckn.message.contract.commitments[0].resources[0].resourceAttributes.`@context`;


Less hardcodes — the opposite direction here: @context is a pack constant and should be stated, not echoed.

$ctx := beckn...resourceAttributes.`@context`;

The reasoning given — that hardcoding means reissuing when the pack URL moves — is fair, but note the inconsistency two lines from where $ctx is used: @type is hardcoded ("openagrinet:WeatherObservation") while @context is echoed. Both are pack identity, and the pack pins them together:

x-jsonld: "@context": "https://schemas.openagrinet.global/schema/WeatherObservation/v0.1/context.jsonld" "@type": openagrinet:WeatherObservation

Echoing means a caller that sends a stale, wrong, or absent @context gets it reflected back inside a response the adapter signs — now asserting a context the adapter never validated, paired with a @type it did assert. If @context is absent from the request, the key simply drops and the answer has no context at all.

Since the file is already per-provider and per-capability (mappings/mausamgram/weather-observation.select.yaml), pinning the version it was written against is the accurate thing to do — the mapping is not portable across pack versions anyway, because field names change between them. That is also what the pack's own examples do.

If you would rather not pin it in the mapping, the alternative that keeps it out of the file is to source it from the plugin config beside bindingKeys, so it is one config edit rather than a mapping edit. Either is better than trusting the caller.

ameersohel45 added 4 commits August 31, 2026 11:47

ameersohel45 changed the title ~~Add the OAN registry, JSONata mapper and Mausamgram provider plugins~~ feat: add the OAN registry, mapper and Mausamgram provider plugins Aug 31, 2026

ameersohel45 added 16 commits August 31, 2026 23:57

manjudr reviewed Sep 3, 2026

View reviewed changes

ameersohel45 changed the title ~~feat: add the OAN registry, mapper and Mausamgram provider plugins~~ feat: add the OAN registry, JSONata mapper and weather provider plugins Sep 3, 2026

ameersohel45 added 2 commits September 3, 2026 16:45

ameersohel45 mentioned this pull request Sep 3, 2026

feat: add the Mandi provider plugin #13

Open

ameersohel45 added 4 commits September 3, 2026 23:09

manjudr reviewed Sep 7, 2026

View reviewed changes

manjudr requested changes Sep 7, 2026

View reviewed changes

Uh oh!

Conversation

ameersohel45 commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

oanRegistry plugin — pkg/plugin/implementation/oanregistry/

Mapper plugin — pkg/plugin/implementation/jsonmapper/

Provider step: machinery and domain

Core changes

The shipped mapping

Why

Testing

Notes for review

Uh oh!

manjudr left a comment

Choose a reason for hiding this comment

Review

Blocking

Should fix

Nits

What is good

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manjudr left a comment

Choose a reason for hiding this comment

Requesting changes

Blockers

Should fix before merge

Lower severity

Separately — this PR has no CI validation at all

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manjudr left a comment

Choose a reason for hiding this comment

Follow-up review: the three standing checks

1. No unused method or logic — clean

2. Proper logs — one security bug, one gap

3. No hardcodes — two committed environment values

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ameersohel45 commented Aug 31, 2026 •

edited

Loading

oanRegistry plugin — `pkg/plugin/implementation/oanregistry/`

Mapper plugin — `pkg/plugin/implementation/jsonmapper/`

Spec-alignment review: `WeatherObservation` v0.1

1. The `required:` guard demands a field the pack forbids — every conformant request is refused

2. `aggregation` invalidates every parameter entry, and the comment claiming otherwise is wrong