feat: add the OAN registry, JSONata mapper and weather provider plugins

[ameersohel45]

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

[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

1. 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.
2. 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.
3. A withdrawn capability returns 500, not 404 — which contradicts the reasoning this PR adds to the no-route path in stdHandler.go.

Should fix

4. 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.
5. Response-leg mapping failures are classified 400, blaming the caller for the provider's answer.
6. 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.
7. A payload with several commitments naming the same capability passes binding derivation, then loses all but commitments[0] in the mapping.
8. 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]

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

1. 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.

2. 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]

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]

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]

Code review

Found 1 issue:

1. The envelope refusal added in 2b2fd45 ships a NACK body under a Signature header computed over the rejected answer. ackSignerStep.RunOnResponse runs as a response step and signs ctx.ResponseBody, setting the header on the live ResponseWriter map; sendResponse then runs verifyEnvelope and, on failure, calls sendNack, which does no signing. signNackResponse is a stdHandler method and is unreachable from the package-level sendResponse.

Verified against da6480d with a step answering 28.5:

status    = 500
body      = {"message":{"status":"NACK","messageId":"msg-1","error":{"code":"NET_INTERNAL_ERROR",...}}}
Signature = ...signature="sig-over-the-scalar"
signed bytes = "28.5"

This is the invariant the change states for itself one path over — the 404 case is checked before the response steps precisely so this cannot happen:

network-adapter/core/module/handler/stdHandler.go

Lines 234 to 240 in da6480d

	//
	// Checked before the response steps rather than after, because
	// ackSigner signs the body it expects to be written; NACKing later
	// would ship a signature over the ACK with a NACK body.
	//
	// Only for modules with provider steps. Elsewhere an unanswered
	// request is the publisher path doing exactly what it should.

The refusal path is the one that reverses that order:

network-adapter/core/module/handler/responsestep.go

Lines 58 to 67 in da6480d

	// where the mapping is, and the NACK names the step so it is findable.
	if err := verifyEnvelope(ctx.ResponseBody); err != nil {
	log.Errorf(ctx, err, "a step produced a response that is not a Beckn envelope; refusing to sign it")
	// A plain error on purpose: nackBecknError's default branch turns it
	// into a generic 500, so the caller learns the answer failed without
	// being handed the internals of a mapping it does not own. The detail
	// is in the log line above, where the operator is.
	return sendNack(ctx, w, err)
	}
	return writeJSONResponse(ctx, w, ctx.ResponseBody)

network-adapter/core/module/handler/responsestep.go

Lines 334 to 345 in da6480d

	// ACK. Either way this signs exactly what sendResponse will write -- signing
	// the ACK while sending an answer would put a valid signature over the wrong
	// bytes.
	ackBody := ctx.ResponseBody
	if len(ackBody) == 0 {
	built, err := buildAckBody(ctx.ProtocolVersion, ctx.MessageID)
	if err != nil {
	return fmt.Errorf("ackSigner: failed to build ack body: %w", err)
	}
	ackBody = built
	}
	// signBodyAndSetHeader writes to ctx.RespHeader which IS the http.ResponseWriter

A peer that verifies the signature on the NACK sees a digest mismatch, so a mapping bug reaches it as suspected tampering rather than as the 500 the code intends. The len(envelope) == 0 branch ({}) behaves the same way. Moving the verifyEnvelope check into ServeHTTP ahead of the response-step loop, next to the existing hasProviderSteps guard, would put it back on the same side of signing as the 404.

Also worth noting: TestAckSignerSignsTheAnswerThatWillBeSent pins that the answer gets signed, but no test covers a non-envelope answer together with the signature header, which is why the gap is invisible to the suite.

Otherwise clean on the areas I checked: query-credential redaction covers both raw and percent-encoded forms and is correctly scoped to the query scheme (basic/header never reach the URL); jsonmapper guards err/hasTransform before evaluate, so the nil evaluating mutex on a compile-failure mapping is unreachable; plugin closers for both new kinds are registered on m.closers per the existing pattern; cacheTTL: 0 disables caching rather than writing zero-TTL entries, and warns when a TTL is set with no cache plugin; ResponseBody is propagated through InstrumentedStep; the fcstday sort keys on the numeric suffix as documented.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[manjudr]

Code review (follow-up)

Four documentation-vs-code contradictions and one refactor leftover, all verified against the code at da6480d. Separate from the signing issue above; none of these is a runtime defect, but in a change where the rationale comments are part of the deliverable, each one tells the next reader the opposite of what the code does.

1. The reference config states that 4xx is never retried; 429 is retried. resp.StatusCode != http.StatusTooManyRequests is an explicit exception, and upstream.go says so two lines up.

network-adapter/config/oan-provider-adapter.yaml

Lines 274 to 281 in da6480d

	#
	# Retry classification is not configurable at all: 4xx is permanent and is not
	# retried, 5xx and transport errors are, and the backoff rises from 50ms to a
	# 800ms ceiling. A non-2xx reaches the caller as an error with the provider's
	# own response included, and the mapping never runs on it -- which is why an
	# upstream that signals "no data" with a 4xx surfaces as a failure rather than
	# an empty result.
	# ----------------------------------------------------------------------------

network-adapter/pkg/plugin/implementation/internal/upstream/upstream.go

Lines 662 to 667 in da6480d

	err := fmt.Errorf("provider returned %s", resp.Status)
	// 5xx and 429 are the provider asking to be tried again. Every other
	// 4xx is a statement about the request, which will not improve.
	if resp.StatusCode < http.StatusInternalServerError && resp.StatusCode != http.StatusTooManyRequests {
	return nil, doNotRetry(err)
	}

2. Same block: "A non-2xx reaches the caller as an error with the provider's own response included" is backwards, and in the direction that matters. The error is fmt.Errorf("provider returned %s", resp.Status) — status only. The body goes to the log, deliberately, because "what a provider puts in a failure body is its own business -- a stack trace, an internal hostname, a database error." An operator reading the config would expect the upstream body to surface to the BAP; the code prevents exactly that.

network-adapter/pkg/plugin/implementation/internal/upstream/upstream.go

Lines 650 to 662 in da6480d

	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
	// The body is logged, not returned. It goes into a 502 that is signed
	// and sent to the network caller, and what a provider puts in a failure
	// body is its own business -- a stack trace, an internal hostname, a
	// database error. The status is the caller's business and stays; the
	// body is the operator's, and the log is where the operator looks.
	// Redacted on the way to the log too. A provider that rejects a
	// request often quotes it back, credential and all -- so the body is
	// exactly where a query-string token turns up, and moving it from the
	// error to the log would only move the leak.
	log.Warnf(ctx, "upstream: provider returned %s for %s %s: %s",
	resp.Status, method, requested, s.redactString(explain(body)))
	err := fmt.Errorf("provider returned %s", resp.Status)

3. The mapper README makes SCH_SCHEMA_ADAPTATION_FAILED a blanket bad request. Only the request leg is a 400; a response-leg failure is a 502, for the reason the code gives — the input there is the provider's answer, not the caller's request. An integrator classifying that code as 4xx for retry or alerting will misfile upstream-shape failures as client errors.

network-adapter/pkg/plugin/implementation/jsonmapper/README.md

Lines 187 to 192 in da6480d

	## Failure
	A mapping that cannot be fetched, parsed or compiled is an operator or registry
	fault and surfaces as a plain error. A mapping that ran but could not be applied
	is the payload's shape being wrong, and surfaces as a `SCH_SCHEMA_ADAPTATION_FAILED`
	bad request.

network-adapter/pkg/plugin/implementation/jsonmapper/jsonmapper.go

Lines 590 to 604 in da6480d

	if err != nil {
	log.Errorf(ctx, err, "JSON mapping %s %s half failed to evaluate: %v", mappingRef, direction, err)
	wrapped := fmt.Errorf("mapping %q %s half could not be applied: %w", mappingRef, direction, err)
	if direction == definition.DirectionResponse {
	// The input here is the PROVIDER's answer, not the caller's
	// request. A provider that changed shape, or a bug in the response
	// half, is nothing the caller did -- reporting 400 sends them off
	// to fix a request that was fine. 502: the upstream exchange is
	// what failed.
	return nil, model.NewCodedErr(http.StatusBadGateway, codeAdaptationFailed, wrapped)
	}
	// On the request leg the mapping is valid and the payload is not what it
	// expected, so this is the caller's request being wrong.
	return nil, model.NewBadReqErr(codeAdaptationFailed, wrapped)
	}

4. The registry README's operation=provider_record outcome list reads as exhaustive and omits binding_no_actions, which is live and reachable. Dashboards built off the documented enumeration would silently undercount the active-and-owned-but-serves-nothing refusal — the case the code calls out as worth keeping distinct.

network-adapter/pkg/plugin/implementation/oanregistry/README.md

Lines 222 to 227 in da6480d

	Provider-record lookups report under `operation=provider_record`, with their own
	outcomes: `binding_not_found` · `binding_inactive` · `binding_unowned` ·
	`participant_not_found` · `participant_inactive` · `no_upstream_url` ·
	`no_binding_key`. Each refusal is kept distinct: they all deny the call, but a
	withdrawn capability and a suspended provider are different operational events.

network-adapter/pkg/plugin/implementation/oanregistry/providerrecord.go

Lines 194 to 200 in da6480d

	}
	if len(servableActions(binding)) == 0 {
	// Active, owned, and callable for nothing. Refusing here says so, rather
	// than letting every action fail one at a time further down.
	log.Errorf(ctx, nil, "OAN registry capability binding bindingKey=%s serves no active actions", bindingKey)
	return providerBinding{}, outcomeBindingNoActions, nil
	}

5. Leftover from dc3a1af: the servedActions doc comment survived the hoist onto model.ProviderRecord and now sits directly above refuse, documenting nothing in this file. Two functions deep, it reads as refuse's own comment.

network-adapter/pkg/plugin/implementation/oanregistry/providerrecord.go

Lines 138 to 145 in da6480d

	return plan, nil
	}
	// servedActions lists the actions a plan covers, sorted so the same record logs
	// the same way twice.
	// refuse records a deliberate denial and returns the caller's sentinel. The
	// registry answered; the answer was no.

Checked and consistent: all five jsonmapper config keys and defaults (fetchTimeout 5s, cacheTTL 1h, negativeTTL 1m, maxMappingBytes 262144, maxCacheEntries 200) and all eight oanregistry keys match their struct tags and parsing; TimeoutMs/RetryMax pass through without local default substitution as documented; BindingKey is carried verbatim, never reconstructed; the "providerIdAt/capabilityCodeAt: both or neither, refused at startup" claim holds; ProviderStep returns nil rather than an error for a capability it does not serve, on both the ErrNoBinding and !serves() paths; StepContext.ResponseBody is only read when rctx == nil, so the routing path genuinely never consults it; the oanbinding path-walk []-only semantics match their doc comment. No TODO or FIXME in the new Go files.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[manjudr]

Code review (follow-up 2)

One more correctness issue, verified at da6480d.

1. Body-echo credential redaction covers only the query scheme, so a basic or header credential echoed back by a provider is logged in the clear at warn level. redactString hard-returns on anything that is not AuthSchemeQuery:

network-adapter/pkg/plugin/implementation/internal/upstream/upstream.go

Lines 796 to 806 in da6480d

	// first thing anyone wants when a provider misbehaves. This is what makes that
	// safe to do at info level.
	func (s *Step) redactString(text string) string {
	if s.config.AuthScheme != AuthSchemeQuery {
	return text
	}
	value := os.Getenv(s.config.QueryValueEnv)
	if value == "" {
	return text
	}
	text = strings.ReplaceAll(text, value, redactedMarker)

It is applied not just to the URL but to the provider's response body, on the non-2xx path:

network-adapter/pkg/plugin/implementation/internal/upstream/upstream.go

Lines 659 to 662 in da6480d

	// error to the log would only move the leak.
	log.Warnf(ctx, "upstream: provider returned %s for %s %s: %s",
	resp.Status, method, requested, s.redactString(explain(body)))
	err := fmt.Errorf("provider returned %s", resp.Status)

That body-echo case is one the change already treats as in scope. TestRunRedactsACredentialEchoedInABody mocks "a provider quoting the request it rejected, credential and all" and asserts on step.redactString(explain(...)) precisely so that "moving the body from the error to the log cannot quietly move the leak with it" — but it only ever exercises AuthSchemeQuery:

network-adapter/pkg/plugin/implementation/internal/upstream/upstream_test.go

Lines 490 to 530 in da6480d

	// provider that echoes the query string back would defeat it.
	func TestRunRedactsACredentialEchoedInABody(t *testing.T) {
	// No t.Parallel: t.Setenv forbids it.
	t.Setenv("TEST_ECHO_TOKEN", "s3cr3t")
	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	w.WriteHeader(http.StatusBadRequest)
	// A provider quoting the request it rejected, credential and all.
	fmt.Fprintf(w, `{"rejected":%q}`, r.URL.RawQuery)
	}))
	defer upstream.Close()
	mapper := &stubMapper{requestResult: []byte(`{}`), responseResult: []byte(`{}`)}
	step := newStep(t, &stubRegistry{plan: testPlan(upstream.URL, http.MethodGet)}, mapper,
	func(c *Config) {
	c.AuthScheme = AuthSchemeQuery
	c.QueryName = "token"
	c.QueryValueEnv = "TEST_ECHO_TOKEN"
	})
	_, err := runStep(t, step, selectBody)
	if err == nil {
	t.Fatal("expected the failure to be reported")
	}
	if strings.Contains(err.Error(), "s3cr3t") {
	t.Errorf("the credential leaked through the quoted body: %v", err)
	}
	// The body no longer reaches the error at all, so its absence from the
	// wire is not what needs proving here -- the LOG is where it goes now, and
	// a provider echoing the request back is exactly where a query-string
	// token turns up. Assert on the same expression the code logs, so moving
	// the body from the error to the log cannot quietly move the leak with it.
	echoed := `{"rejected":"token=s3cr3t"}`
	logged := step.redactString(explain([]byte(echoed)))
	if strings.Contains(logged, "s3cr3t") {
	t.Errorf("the credential survives into the log line: %s", logged)
	}
	if !strings.Contains(logged, "REDACTED") {
	t.Errorf("logged body = %q, want the credential replaced", logged)
	}
	}

The shipped config is the scheme that is not covered:

network-adapter/config/oan-provider-adapter.yaml

Lines 226 to 231 in da6480d

	# header a named header; headerName + headerValueEnv
	# query a named query parameter; queryName + queryValueEnv.
	# Redacted from the URL the step logs.
	authScheme: basic
	usernameEnv: MAUSAMGRAM_USER
	passwordEnv: MAUSAMGRAM_X_API_KEY

Verified by calling redactString(explain(body)) on a step configured exactly as the sample config does, with the credential in the body:

basic  (wire form)  {"error":"invalid Authorization: Basic bWF1c2FtOnMzY3IzdA=="}   -> unchanged
basic  (raw pass)   {"error":"bad password s3cr3t"}                                 -> unchanged
header              {"error":"bad X-API-Key: hdr-k3y"}                              -> unchanged

An API gateway quoting the rejected Authorization header is the ordinary shape of a 401/403 body, and a wrong-credential 4xx is not retried, so it lands in the log once per request for as long as the credential is wrong. Worth noting the fix is not symmetric with the query case: for basic the leaked value is base64(user:pass), so redacting $MAUSAMGRAM_USER and $MAUSAMGRAM_X_API_KEY individually would miss the wire form — it needs the encoded pair too, the way the query path already handles url.QueryEscape.

The comment at line 228 ("Redacted from the URL the step logs") is accurate for query and is the only scheme that says anything about redaction, so the config is not making a false promise here — but the asymmetry is invisible to an operator choosing basic.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[manjudr]

Please rebase your branch from the development, as there are CI changes applied

[manjudr]

Code review (follow-up 3)

Three more, all verified against da6480d.

1. da6480d's message says the subscriber id was placeholdered, but it was not. The message reads "Same treatment as the subscriber id, and for the same reason: a reference config should show what a value looks like without presenting one deployment's as everyone's." — da6480d only changed url: http://registry:8081/api/v1 to url: <>. Across ced4555..da6480d the subscriberId lines were only ever added, never placeholdered, and both still carry provider-network-vistaar.da.gov.in. The hardcode itself was raised earlier; the new information is that the commit message asserts a fix that isn't in the tree, so it reads as done when it isn't.

network-adapter/config/oan-provider-adapter.yaml

Lines 56 to 58 in da6480d

	role: bpp
	subscriberId: provider-network-vistaar.da.gov.in

network-adapter/config/oan-provider-adapter.yaml

Lines 121 to 123 in da6480d

	config:
	subscriberId: provider-network-vistaar.da.gov.in

2. Second orphaned servedActions doc comment, this one in upstream.go. Same dc3a1af hoist leftover as the one already reported in providerrecord.go — the comment describes servedActions, but sits between the end of one function and // buildRequest produces what the provider is sent., so it documents nothing.

network-adapter/pkg/plugin/implementation/internal/upstream/upstream.go

Lines 419 to 427 in da6480d

	log.Infof(ctx, "upstream: served %s in %d bytes", plan.BindingKey, len(becknResponse))
	return nil
	}
	// servedActions lists the actions a capability covers, sorted so the same
	// record reads the same way twice.
	// buildRequest produces what the provider is sent.
	//

3. TestInitStepsRefusesTwoProviderStepsWithTheSameID passes with the duplicate-ID guard deleted. This is not a coverage nitpick — the test gives false assurance about logic this PR adds. h := &stdHandler{moduleName: "test-module"} never sets h.mapper, so loadProviderStep fails on the first config entry with failed to load ProviderStep plugin (mausamgram): Mapper plugin not configured, and the assertion strings.Contains(err.Error(), "mausamgram") matches the id embedded in that mapper error rather than in the duplicate-id error. Removing the if _, taken := steps[c.ID]; taken block entirely and rerunning still gives --- PASS. The guard has zero reaching coverage.

network-adapter/core/module/handler/responsebody_test.go

Lines 176 to 196 in da6480d

	// id is now a mistake rather than the way to configure a second one.
	func TestInitStepsRefusesTwoProviderStepsWithTheSameID(t *testing.T) {
	h := &stdHandler{moduleName: "test-module"}
	cfg := &Config{
	Plugins: PluginCfg{
	ProviderSteps: []plugin.Config{
	{ID: "mausamgram", Config: map[string]string{}},
	{ID: "mausamgram", Config: map[string]string{}},
	},
	},
	}
	err := h.initSteps(context.Background(), noopPluginManager{}, cfg)
	if err == nil {
	t.Fatal("expected two provider steps with the same id to be refused")
	}
	if !strings.Contains(err.Error(), "mausamgram") {
	t.Errorf("error %q should name the id that repeats", err)
	}
	}

network-adapter/core/module/handler/stdHandler.go

Lines 643 to 645 in da6480d

	if h.mapper == nil {
	return nil, fmt.Errorf("failed to load ProviderStep plugin (%s): Mapper plugin not configured", cfg.ID)
	}

network-adapter/core/module/handler/stdHandler.go

Lines 684 to 689 in da6480d

	// rather than by appearing twice.
	if _, taken := steps[c.ID]; taken {
	return fmt.Errorf("provider step %q is configured more than once; "+
	"a step serving several capabilities lists them in its own config", c.ID)
	}
	step, err := h.loadProviderStep(ctx, mgr, &c)

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[ameersohel45]

Both fixed — and the first one was mine to own

1 · evaluating.Unlock() not deferred — 82a5f1e. Correct, and worse than the comment lets on, because of what my previous commit did.

The lock used to be per mapping. I widened it to package scope to fix a real data race, and should have deferred the unlock in that same change. Without it, a panic inside Evaluate doesn't wedge one mapping — it wedges every mapping for every provider step, for the life of the process. I widened the correctness fix and the blast radius together, and only did the first half.

Fixed with a helper rather than a defer at each call site, because the precondition loop evaluates once per check: a defer there releases only when the whole loop returns, which would hold the lock across every check in the file — a smaller version of the same mistake.

Two things worth flagging:

• The panic is converted to an error, not re-raised. net/http recovers per connection, so re-raising costs the caller its connection with no NACK and nothing in our log naming the mapping — indistinguishable, from the caller's side, from a mapping that could not be applied. So it's reported as one. Push back if you'd rather it propagated.
• It's testable because jsonata.Expression is an interface. The test injects an expression that panics, asserts the error, then takes the lock from another goroutine with a timeout. Un-defer the unlock and it fails on that timeout rather than hanging the suite — verified.

2 · _local contradicting its own contract — 214a793. Verified all four places. The code passes _local on both legs and Prerequisites documents it; Mapper.Transform, the README table, upstream.go's own comment and the mausamgram mapping all said the opposite.

Corrected the three wrong ones rather than the behaviour, since the code and the Prerequisites doc agree. The distinction they were reaching for is kept, because it's worth keeping: _local is for what a payload cannot carry and a mapping cannot obtain — a code looked up from a name, a point resolved to a market — and not for values the plugin already holds and merely used to make the call.

Your closing point is the reason it drifted, and it's now addressed directly: nothing asserted _local at all, so the whole path was dead code. Added a test resolving two values through a real Prerequisites entry and checking they arrive on both legs, plus one checking that no prerequisites yields an empty _local rather than an absent one — so a mapping referring to it reads nothing instead of failing. Both fail if the key is dropped.

This lands just in time to matter: the next capability under discussion may need a real prerequisite, which would have been the first thing to discover the gap at runtime.

Verified: make test green — 62 packages, 0 failures, 0 races on this branch, 64 on both stacked above it — and the changed-line coverage gate passes at 85%. Both branches above rebased and pushed.

[manjudr]

Merge-readiness re-check (head bcd10b3927201c6855eb500322361815db43aea8)

Both findings from the prior automated review are fixed and verified against the diff, not just the commit messages:

1. JSONata mutex deadlock — 82a5f1e adds an evaluateLocked helper in jsonmapper.go that wraps every Evaluate call with defer evaluating.Unlock() plus a recover() converting a library panic into a returned error. Both call sites (Verify's precondition check, evaluate) now use it.

network-adapter/pkg/plugin/implementation/jsonmapper/jsonmapper.go

Lines 106 to 138 in bcd10b3

	// evaluateLocked runs one expression under the package lock.
	//
	// The unlock is DEFERRED, not written after the call. A panic inside the
	// library -- and this is a library we have already found a data race in --
	// would otherwise leave the mutex held with nothing to release it. Because the
	// lock is package-wide, that is not one wedged mapping but every mapping in
	// the process, for every provider step, until a restart. Widening the lock
	// widened that blast radius, so the defer matters more here than it did when
	// the lock was per mapping.
	//
	// A helper rather than a defer at each call site, because the precondition
	// loop evaluates once per check: a defer there would release only when the
	// whole loop returned, holding the lock across every check in the file.
	//
	// The panic is converted to an error rather than re-raised. net/http recovers
	// a panic per connection, so re-raising costs the caller its connection with
	// no NACK and nothing in our log naming the mapping -- for what is, from the
	// caller's side, indistinguishable from a mapping that could not be applied.
	// Reported as one, so it lands where the fault is.
	func evaluateLocked(expr jsonata.Expression, document []byte) (result []byte, err error) {
	evaluating.Lock()
	defer evaluating.Unlock()
	// Registered after the unlock so it runs BEFORE it: recover, name the
	// failure, then release.
	defer func() {
	if recovered := recover(); recovered != nil {
	result = nil
	err = fmt.Errorf("jsonata evaluation panicked: %v", recovered)
	}
	}()
	return expr.Evaluate(document, nil)

2. _local contract contradiction — 214a793 brings all four previously-contradictory sources (mapper.go's Transform doc, upstream.go's comment, the jsonmapper README, and the mausamgram select.yaml comment) into agreement: _local carries prerequisite-resolved values, empty when a plugin declares none. Two new tests exercise the previously dead pass-through path (TestRunHandsResolvedPrerequisitesToTheMappingAsLocal, TestRunPassesAnEmptyLocalWhenThereAreNoPrerequisites in upstream_test.go).

network-adapter/pkg/plugin/definition/mapper.go

Lines 32 to 47 in bcd10b3

	// Which action the mapping serves is settled by the registry entry that
	// named it, so only the direction is passed here.
	//
	// input carries what a party sent -- the inbound payload, and on the way
	// back the provider's answer -- plus, under _local, any values the caller
	// resolved before making the call.
	//
	// _local is for what a payload cannot carry and a mapping cannot obtain: a
	// code looked up from a name, a point resolved to a market. A caller with
	// nothing to add passes an empty map, so a mapping referring to _local
	// reads nothing rather than failing. Values the caller already holds and
	// merely used to make the call do NOT belong here -- routing those back
	// through a mapping is a second name for the same data.
	//
	// A direction the file has no transform for produces nothing, with no error.
	// What nothing means belongs to the caller: on the request leg it means there

No new issues found in either fix commit. All review threads on this PR are already marked resolved. The three CHANGES_REQUESTED reviews on record predate these fix commits (2026-09-08) and would just need re-approval.

From a code standpoint this looks good to merge.

🤖 Generated with Claude Code

[ameersohel45]

Thanks — and thank you for checking the diff rather than the commit messages. That distinction matters here, because one of these fixes was a defect I introduced with the previous one.

Confirming both against the merged head, independently:

• evaluateLocked — jsonmapper.go:125, defer evaluating.Unlock() registered before the recover() so the recover runs first, and both call sites go through it (:335 the precondition check, :640 the mapping half). A helper rather than a defer at each site because the precondition loop evaluates once per check, where a defer would hold the lock across every check in the file.
• _local — all four sources now agree, and the two tests cover a path that was previously dead: nothing had ever asserted _local at all, which is why three of those four descriptions had drifted from the code.

Also confirming the thread state matches: 36 of 36 resolved.

With #13 and #17 merged in, this PR now carries the whole stack — the mandi capability and the schema-pack work included — so the diff is considerably larger than when the CHANGES_REQUESTED reviews were filed. Happy to walk any part of it if that helps the re-review.