feat: add the Mandi provider plugin

[ameersohel45]

Targets feat/41-oan-adapter-plugins rather than development, because it builds on the machinery that PR #2 introduces. Merge that one first.

What

A second provider capability, openagrinet:MandiPrice, served against Agmarknet's Vistaar select. Additive: six new files, one line added to the plugin build list, and nothing existing changed.

The domain package — pkg/plugin/implementation/mandi/

38 lines, plus a 20-line prerequisites file that is deliberately empty. It supplies its name and nothing else; recognising the capability, resolving the call plan, authenticating, calling with the registry's budget and translating in both directions are all internal/upstream's.

internal/upstream needed no change for it. That was the test of whether the machinery and domain split in PR #2 actually held, and it is the reason this PR is as small as it is.

No prerequisites, and not by luck

A MandiPrice select names the market it wants — market.marketCode, market.district, market.state — and carries a commodity code and a validity window, which is every parameter the upstream takes. The pack has no top-level location, so nothing has to turn a point into a market.

That matters because turning a point into a market is exactly what the provider backend needs a PostGIS query for, and the adapter may not talk to a database. The pack's shape and the upstream's contract happen to agree, so the plugin is data only.

The mapping — config/mappings/agmarknet/mandi-price.select.yaml

Where the actual work is. Three things in it are not obvious:

• the upstream's records use Title Case keys containing spaces — `Modal Price` — so they need backticks, and its prices are strings, so they need $number() before they satisfy the pack's numeric types. A verbatim capture from the provider backend's own documentation is pinned as a fixture, so neither can drift unnoticed.
• dates convert twice. The pack speaks ISO, the upstream speaks dd-MM-yyyy, so the request half converts out and the response half converts back.
• the pack requires none of the fields the upstream needs. An OnDemand select requires only supportedCommodities and supportedPriceFields, leaving market and validity optional, and defines market.district and market.state as "name or governed code". So a spec-valid select can still be unanswerable, and the required: block refuses those with its own message rather than earning a 400 from Agmarknet — or worse, an empty result that reads as "no prices".

Resource ids are built from codes rather than the names the upstream reports: Kasdol APMC and Paddy(Common) carry spaces and brackets, and an id a consumer may put in a URL should not.

Query-string auth

Agmarknet takes its token as a query parameter. authScheme: query for that arrives in PR #2, since it is machinery rather than anything mandi-specific. The config here holds the parameter's name and the name of the environment variable carrying the value, never the value.

Why

Two capabilities from two domain packages in one pipeline, dispatched by binding key read from the payload. Which one answers is decided by the request, not by a routing table, a URL or a domain — each step recognises its own key and passes through anything else.

It also settles a claim PR #2 could only assert: that adding a domain costs a small package and a mapping. It cost 58 lines of Go and one line in the build script.

Testing

go vet ./... clean and go test ./... green across 63 packages.

The shipped mapping is run through the real mapper and the real provider step against a captured Agmarknet response, rather than asserted about. Both directions validate with jsonschema — the select against beckn.yaml and MandiPrice v0.1 in OnDemand mode, the on_select against beckn.yaml and the pack in Direct mode — with zero errors.

End to end on the local stack, both capabilities loaded into one provider adapter:

• a select naming the mandi provider returns one Direct resource per price record, prices converted from strings, dates in ISO, the offer's references rewritten to what was returned, and a record that reported no minimum or maximum coming back with those absent rather than zeroed
• a select naming the weather provider still returns its per-day forecast, unaffected
• a publish and a discover for each capability, since publishing is capability-agnostic and a catalogue entry advertises OnDemand while carrying no prices

The local Postman collection covers all of that: 8 requests, 39 assertions, no failures.

Notes for review

• Tested against a mock that reproduces the awkward parts of the real API deliberately — Title Case keys with spaces, string prices, dd-MM-yyyy, and a query token it rejects when absent. The real API's credentials, host and exact record set are untested.
• v1 only. Agmarknet's v2 endpoint takes a point rather than market codes and resolves the market itself, which fits our payload better — but it needs a POST token exchange, and that needs pre-call I/O whose design is still open. Not attempted here.
• One behaviour the provider backend has and this does not: when a market-scoped query returns nothing it retries without marketcode, widening to the district. Our step makes one call, so an empty result stays empty. Left alone until the real API's behaviour is observed, along with the question of whether Agmarknet reports "no data" as an HTTP 400.

Closes #8

[manjudr]

Requesting changes

Small, focused PR and the mapping test is the strongest part of it — it asserts real output rather than just that the mapping compiles: outbound query params, on_select, DRAFT status, resource count, id shape, the six Direct-required fields, string→number price coercion, ISO arrival date, and the absent-not-zero partial-record case. That's the right kind of test.

Its blind spot is that every fixture is well-formed and code-bearing, which is exactly why the findings below survive it. None of them need much code — they need adversarial fixtures.

Verified before reviewing: go build ./..., go vet, gofmt -l, go test ./pkg/plugin/implementation/mandi/... and go test -race all pass. Findings 1, 2, 3 and 5 were confirmed empirically against the real mapper and the shipped mapping, and the observed outputs are quoted inline.

Should fix before merge

1. mandi-price.select.yaml:120 — $resourceId omits Variety/Grade, so rows that differ only by those collide on one id. The PR's own fixture has such a pair, saved only by differing dates. District-wide queries collapse every market onto one id too.
2. :130 — $priced checks existence but not castability. Agmarknet's NR / - markers make $number() throw D3030, so one unreported cell 500s the entire multi-row answer. Line 193's bare $number on modal is the same hazard and contradicts the file's stated rule.
3. :37 — the guard's message says "governed state and district codes" but the check is bare $exists, and the pack permits names. Names get sent as codes, Agmarknet returns nothing, and the caller gets a signed "no prices" for a market that had prices — verbatim the failure lines 20-26 say the guard prevents.
4. :68 — only supportedCommodities[0] is ever read, so a three-commodity request is answered for one with no complaint. Same class as the oanbinding refusal, without the guard.
5. :189 — a missing Arrival Date produces arrivalDate: "--" and a degenerate resource id. Weather guards this exact case at weather-observation.select.yaml:248 ("Absent is honest; empty is a lie in the shape of an answer") — an accidental divergence between siblings.
6. :171 — subjectCategories echoed from the caller rather than stated from the pack, where weather hardcodes ["Weather"]. Unvalidated, so a caller can make the adapter republish the wrong category under its own signature.

Lower severity

:185 mixes request scope with record identity (marketCode from the request, everything else from the record), so district-wide answers name several markets with no code. :191 ignores supportedPriceFields entirely.

Reuse

mandi/cmd/plugin.go is a verbatim 104-line copy of weather/cmd/plugin.go. Since mandi.Config = upstream.Config, none of it is domain-specific. It has already cost something concrete: mandi/cmd ships with no tests while weather/cmd has five covering parseConfig's rejection paths, so the copied error handling is unexercised. Sharing the helper closes the gap rather than requiring a second copy of the tests.

Scope

Clean. The +25/-1 in oan-provider-adapter.yaml is ~6 lines of real config plus ~19 of comment prose matching the file's house style, and the steps addition is required. install/build-plugins.sh +1 is the only other registration point — I checked every place weather appears for a missed one. No dead code, no unrelated edits, no speculative knobs.

One cross-PR note inline: registering a second provider step is what makes PR #2's "arity refused before ownership" issue live rather than theoretical. Not a defect here, but it should be fixed on #2 first.

Sequencing

This targets feat/41-oan-adapter-plugins, so PR #2 needs to land first — and #2 has two blockers of its own. Worth resolving those before merging this.

[manjudr]

This whole 104-line file is a verbatim copy of weather/cmd/plugin.go, comments included — parseConfig, New, splitList, the maxResponseBytes parsing and validation, and the Provider/var _ declarations are byte-identical modulo four identifier substitutions.

Since mandi.Config is already = upstream.Config (mandi.go:31), nothing in here is domain-specific. It belongs in a shared internal/upstream helper taking a package name and a New func — otherwise every future domain plugin re-copies it, and any fix to the config parsing has to be applied N times.

This already has a concrete cost: go test reports pkg/plugin/implementation/mandi/cmd [no test files], while weather/cmd/plugin_test.go has five test funcs covering parseConfig — including the maxResponseBytes non-integer and non-positive rejections at lines 47-53. So the copied error-handling path is entirely unexercised in mandi.

Sharing the helper closes the test gap instead of requiring a second copy of the tests.

[ameersohel45]

Agreed on the substance, and holding this one rather than doing it now — flagging so it isn't mistaken for handled. Leaving the thread open.

One correction to the shape of the fix, though. plugin.go can't move to internal: each plugin is built as its own .so from ./pkg/plugin/implementation/<dir>/cmd/plugin.go, and the loader does pgn.Lookup("Provider"), so every domain plugin needs its own package main exporting that symbol. Move it and there's nothing to build a .so from.

What can move is the ~45 lines that aren't domain-specific — parseConfig and splitList. plugin.go would stay put as a ~20-line shim calling a shared upstream.ParseConfig. Domain-specific parts are only the provider type, the New it delegates to, and the exported symbol.

Your concrete-cost point is the reason it's worth doing at all, and I verified it: mandi/cmd has 0 test files against weather/cmd's 5 test funcs, so the maxResponseBytes bounds checking and the auth-scheme validation are untested in this copy. Shared, they'd be tested once and every future plugin would inherit that.

Also confirmed your "verbatim modulo identifiers" claim — after substituting mandi→weather the diff is comment-only.

[manjudr]

Follow-up review: the three standing checks

Second pass against the defaults — no hardcodes (placeholder + example, read from config), proper logs, no unused methods or logic. Additions to my earlier review; the findings there still stand.

1. No hardcodes — one real operability gap

oan-provider-adapter.yaml:257 — MANDI_TOKEN appears exactly once in the entire repository outside tests: the line that references it. No .env.example, no README section, no deploy manifest, nothing in install/build-plugins.sh.

The result is a plugin that loads, registers, and validates cleanly, then fails every request permanently (doNotRetry), with the only clue being a runtime string naming a variable nobody was told to set. Detail and a suggested .env.example entry inline.

There is an irony worth fixing while you are there: the only env vars this config documents (MAUSAMGRAM_X_API_KEY, MAUSAMGRAM_TOKEN at lines 228/232) are commented-out examples that nothing reads, while the one variable actually required at runtime is undocumented.

Otherwise the hardcode picture here is good. The secret itself is correctly read from the environment rather than committed, and the pack-level constants (on_select, DRAFT, the @type values) are legitimately literal. One judgement call inline at mandi-price.select.yaml:194 about "currency": "INR".

2. Proper logs — nothing to fix in this PR

mandi and mandi/cmd follow the same convention as their weather counterparts (log.Debugf/Infof/Warnf(ctx, ...), no fmt.Println, no payload bodies logged), and all the request-path logging lives in internal/upstream, which this PR does not touch.

One thing this PR inherits rather than causes: I found a credential-redaction bug in internal/upstream that writes the auth token to the log in recoverable form at info level on every successful call, because the redaction string-matches the raw env value while the URL carries it percent-encoded. It affects MANDI_TOKEN specifically — mandi is the only step configured with the query auth scheme, so this PR is what makes the bug reachable in production. I have filed the detail on PR #2, where the code lives. Worth tracking as a dependency of this PR rather than fixing here.

3. No unused method or logic — clean

Swept the mandi packages for unexported functions with no production caller, config fields never read, and consts never referenced. Nothing. mandi.Config is a type alias to upstream.Config so it has no unread fields of its own, and every helper in mandi/cmd/plugin.go is called.

The related point stands from my earlier review: that file is not unused, but it is a verbatim 104-line copy of weather/cmd/plugin.go, and the duplication is why mandi/cmd ships with no tests while weather/cmd has five.

[manjudr]

Spec-alignment review: MandiPrice v0.1

Reviewed the mapping against the actual pack in OpenAgriNet/network-specs (branch schema-packs-v0.1, schema/MandiPrice/v0.1/), and validated the mapping's real output shapes against the pack's own subschemas with a JSON Schema validator. The response half is in good shape; the request half is built on fields the pack forbids.

1. Blocker: the OnDemand branch forbids market and validity, and two guards require them

The comment at :19-23 reads the pack as leaving market and validity optional. It excludes them — not/anyOf over required is an exclusion list, covering source, commodity, commodityGroup, grade, variety, market, arrivalDate, prices, generatedAt and validity.

payload satisfying all three of this file's guards   -> VIOLATES PACK
the pack's OWN OnDemand example (coverageAreas)      -> CONFORMS

The adapter refuses the pack's own example, and every payload it accepts is spec-invalid. This is structural: statecode, districtcode, from_date, to_date and marketcode all come from those fields.

The pack's OnDemand scope mechanism is coverageAreas ({codeScheme: ISO-3166-2, areaCode: IN-TS, areaLevel: State}). Mapping that onto Agmarknet's numeric codes needs a gazetteer lookup rather than a JSONata expression — which is presumably why the shortcut was taken. Detail and a suggested guard inline at :19 and :37.

2. A real gap in the pack, not in this PR

The date-range guard cannot be made conformant today. validity is forbidden in OnDemand, and the available OnDemand fields (historicalDataAvailable, historyPeriod, updateFrequency) all describe provider capability, not caller intent. Since this upstream requires from_date/to_date, MandiPrice v0.1 has no way to express what this integration needs.

That is worth taking to the spec repo — a requestedPeriod, or permitting validity in OnDemand as the requested window. I would not hold this PR for the pack change, but the file should say the guard is knowingly non-conformant rather than let it read as satisfied. Inline at :43.

3. Validation failures confirmed against the pack

Line	Case	Result
:195	upstream omits Price Unit	INVALID: 'unit' is a required property
:189	record has no Arrival Date	INVALID: '--' is not a 'date'
:171	caller echoes ["Weather"]	VALID — but asserts the wrong subject
:171	caller echoes []	INVALID: [] should be non-empty

These sharpen three findings from my earlier review from "looks wrong" to "fails the pack".

4. Hardcodes — you asked for fewer; two here should stay, one should be added

Applying the rule that makes this decidable — pack constants belong in the mapping, deployment values belong in config — this file is mostly on the right side:

• "currency": "INR" (:194) — keep. Pack requires ^[A-Z]{3}$, Agmarknet reports only INR, file is Agmarknet-specific. This supersedes my earlier suggestion to move it.
• "@type" (:169) — keep. Pack constant.
• subjectCategories (:171) — should become a hardcode: ["Market"]. It is a closed-enum pack value that both of the pack's own examples state, and the weather mapping already hardcodes its equivalent. Echoing it is what lets a caller push the wrong subject through the adapter's signature.

The deployment values worth moving to configuration are the ones I flagged on PR #2 (subscriberId, registry URL) and the undocumented MANDI_TOKEN — not these.

5. Where the mapping is right

The response half checks out field-for-field against the pack: every key it emits is declared, correctly placed, and inside the closed market and prices property sets. variety and grade are populated (:181-182) where a lazier mapping would have dropped them — which is also why using them in $resourceId is a small change, since the data is already in hand. Two optional refinements inline at :13 (sourceId prefix convention, missing sourceUri).

Verification

Schema facts come from schema/MandiPrice/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 its self-contained prices / market / arrivalDate / subjectCategories subschemas. No external $ref was stubbed for any result quoted here.

[github-actions]

🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)

View full run

Go dependencies

No findings at CRITICAL,HIGH,MEDIUM,LOW.

Container image

No findings at CRITICAL,HIGH,MEDIUM,LOW.

[github-actions]

📊 Test Coverage: ✅ Passed — 100% of changed lines covered, min 80%

[manjudr]

@ameersohel45 - Please don't use the OAN in the file names

[manjudr]

Code review

Found 1 issue:

1. The doc comment describing $resourceId's rationale ("bound once because it is used twice", "built from CODES, not the names the upstream reports") sits above the unrelated $iso date-conversion function instead of above $resourceId itself (defined later, with its own different comment already in place). The comment that actually belongs to $iso is left orphaned further down with no code beneath it. Since this file's commit history shows a resource-id collision had to be fixed once already, a future edit to $resourceId risks missing this rationale and reintroducing that bug.

network-adapter/config/mappings/agmarknet/mandi-price.select.yaml

Lines 144 to 160 in c59783c

	$ra := $selected.resources[0].resourceAttributes;
	/* Bound once because it is used twice -- for a resource's own id and for
	the offer's reference to it. Two copies of one expression is how a
	dangling reference gets reintroduced. */
	/* Built from CODES, not the names the upstream reports. A market name
	carries spaces and a commodity name carries brackets -- "Kasdol APMC",
	"Paddy(Common)" -- and an identifier that a consumer may put in a URL or
	a filter should not. The codes are already in the payload, so they cost
	nothing, and they are stable where a display name is not.
	The market code is optional: without it the query widened to the whole
	district, so the district code is what identifies the scope. */
	$iso := function($ddmmyyyy) {
	$substring($ddmmyyyy, 6, 4) & "-" & $substring($ddmmyyyy, 3, 2) & "-" & $substring($ddmmyyyy, 0, 2)
	};

The orphaned $iso comment:

network-adapter/config/mappings/agmarknet/mandi-price.select.yaml

Lines 188 to 193 in c59783c

	/* dd-MM-yyyy back to ISO, so the answer speaks the pack's date format
	rather than the upstream's. */
	/* Absent rather than present-and-empty: a consumer must be able to tell
	"the market reported no minimum" from "the minimum was zero".

🤖 Generated with Claude Code

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

[ameersohel45]

Agreed, and already done in #17 — which targets this branch, so it lands here on merge. Nothing needed on this PR.

This branch has 11 such paths; after #17 the count is zero:

on this branch	after #17
pkg/plugin/implementation/oanregistry/	sunbirdRegistry/
pkg/plugin/implementation/internal/oanbinding/	internal/capabilitybinding/
config/oan-provider-adapter.yaml	config/provider-adapter.yaml

Each is named for what it is rather than for the network it was written for: the registry client speaks SunbirdRC, and the binding package resolves a capability binding. Same reasoning as the capability plugins in your other comment.

The comments were done too, not just the paths — nothing in the Go file or directory names carries the network name, and #17 has a commit for the wording left in the reference config.

[ameersohel45]

Fixed in 80dd82d — and your reasoning about why it matters is the part I'd have missed.

Confirmed all three placements. $resourceId's two blocks — "bound once because it is used twice" and "built from CODES, not the names the upstream reports" — sat above $iso, which converts dd-MM-yyyy to ISO and has nothing to do with either. $iso's own comment was orphaned further down with no code beneath it at all. $resourceId did already have its own comment, so the two now join it.

Your point about the history is exactly the risk: a resource-id collision had to be fixed in this file once already, so someone editing $resourceId next and reading the comment directly above it would have found a date-conversion rationale and none of the reasons the id is shaped the way it is.

One judgement call beyond a straight move. The market-code paragraph — "without it the query widened to the whole district, so the district code is what identifies the scope" — went to $scope rather than travelling with the rest, because it describes that binding specifically rather than how the id is composed. Say the word if you'd rather it stayed with $resourceId.

No expression changed. The shipped-mapping tests serve this exact file over HTTP, so they'd have caught it if one had.

[manjudr]

Merge-readiness re-check (head 80dd82d55f012a29ae4eace3a3335fc9ff2b05bc)

All 8 issues from the three CHANGES_REQUESTED reviews are verified fixed in the current code (resource-id collisions, unguarded price casts, name-vs-code guards, silently-dropped multi-commodity requests, degenerate arrival dates, echoed subjectCategories, MANDI_TOKEN docs) — confirmed by reading the file at head and running the mandi test suite (all passing), not just the commit messages. The prior automated review's finding (misplaced $resourceId/$iso doc comments) is also fixed, in 80dd82d.

Two items remain open, both non-blocking:

• mandi/cmd/plugin.go is still a near-verbatim copy of weather/cmd/plugin.go — explicitly acknowledged and deferred by the author, thread left open on purpose.
• The market/validity guard vs. OnDemand mode is a documented, external pack-spec gap, not fixable in this repo.

No new issues found. From a code standpoint this looks good to merge; the outstanding CHANGES_REQUESTED reviews predate these fix commits and would just need re-approval.

🤖 Generated with Claude Code