feat: enforce the schema packs on resourceAttributes, and rename the capability plugins

[ameersohel45]

Targets feat/8-mandi-plugin rather than development, because it changes both capability plugins that PR #13 introduces. That makes this the third in a stack — merge #2, then #13, then this one.

What

Extended schema validation, on for the first time, plus the rename that makes a plugin id read as the capability it serves. 13 files, +359 / -61, four commits.

The validator — pkg/plugin/implementation/schemav2validator/extended_schema.go

The one substantive code change, and the only reason the layer could not be switched on before.

validateReferencedObject stripped both @context and @type from every object before validating it, on the reasoning that they are JSON-LD plumbing the validator handles itself. The OAN packs disagree: they declare @type and list it in allOf[].required. So a conforming payload was rejected for a missing @type that it had sent and the validator had just removed, and nothing in configuration could bridge it.

The fix is not "stop stripping @type". That would break the other schema style, which the plugin's own tests use — a schema that closes itself with additionalProperties: false and never mentions either key rejects the payload if @type is left in. The two styles need opposite treatment, so the decision is made per key by asking the resolved schema whether it declares that key, as a property or in required, anywhere in its composition tree. The packs declare @type one level down inside allOf, so the walk covers allOf/anyOf/oneOf and the then/else branches.

not and if are deliberately not walked. Naming a property under not forbids it and under if only selects a branch, so a match in either says nothing about whether the key is permitted.

Keeping @type has a second effect worth noting: its const is now checked, so a resource claiming a @type its capability does not declare stops passing silently.

Where the schemas come from — no mount, no vendored copy

Each resource's @context names the published pack, and the validator swaps context.jsonld for attributes.yaml to fetch the schema beside it. So the pack revision a payload is judged against comes from the payload, and nothing in this repo can go stale against what the network publishes.

A local mounted directory was built first and then removed. It existed only because the @context the payloads carried did not resolve, and a failed fetch rejects the payload — but the published packs do serve context.jsonld, so pointing @context at them makes the fetch the validator already knew how to do work, and the mount, the fetch script and a startup dependency all became unnecessary.

extendedSchema_allowedDomains is narrowed to the host @context actually resolves to, and is now load-bearing rather than naming a host that never answered: an @context on any other host is refused before a fetch is attempted.

The rename — weather → WeatherObservation, mandi → MandiPrice

Mechanical, but breaking for any deployment. A step name that is not one of the built-ins is looked up among the loaded plugins, and a plugin's id is the basename of its .so, so the directory rename carries the config ids with it: providerSteps and steps in config/oan-provider-adapter.yaml and the entry in install/build-plugins.sh move together with the packages. An adapter running this config against an image built before the rename exits with unrecognized step: WeatherObservation, which reads like a config typo and is not — config and image have to roll forward and back together.

Both package docs claimed "one package per schema pack family", which the new names contradict. They now say what is true: one package per capability, named for the capability, with the binding keys it answers to still configuration.

Why

Base validation checks the envelope and treats resourceAttributes as a free-form object, so until now nothing checked a capability's own attributes at all. A wrong unit, a missing required attribute or a field the pack does not define all passed, and became something a consumer discovered later. The plugin already had the machinery; it was configured and never ran.

Testing

Full suite green — 63 packages. Four new tests cover the per-key strip, including a pack-shaped schema that requires @type, one that rejects a wrong @type, and the composition walk.

End to end against a running stack, on an image built from this branch, with both plugins loading under their new ids: publish and select pass for both capabilities, the first payload logs fetching from network and every later one LRU cache hit, and the local collection is 51 of 51.

Negative controls, so a pass reads as real checking rather than an absence of it — each rejected with SCH_SCHEMA_VALIDATION_FAILED and the JSON path that failed:

• a value outside an enum
• a bare date in a date-time field
• a field undefined in a closed sub-object, which resolves through a schema.beckn.io $ref

And an @context on a host outside the allowlist rejected with SCH_INVALID_JSONLD_CONTEXT, before any fetch.

Notes for review

• if/then/else is not enforced, and a green result is misleading without knowing that. kin-openapi v0.144.0 has the struct fields, so the keywords look supported, but visitJSON never dispatches them. Verified empirically rather than read off the struct. Enforced: types, string formats, enum, const, required, minItems, additionalProperties, not, allOf/anyOf/oneOf.
• A consequence of that, worth raising on the packs rather than working around here. The packs require informationMode and allow only OnDemand or Direct. OnDemand forbids exactly the fields a select must carry to be a question — location for weather, market and validity for mandi — while Direct requires answer data a consumer does not have. There is no query mode, so our select requests are not pack-conformant on paper and pass only because the forbidding sits under then.
• One known non-conformance, not fixable here. The published weather mapping emits parameters[].aggregation, which the pack forbids: its parameter item is {parameter, value, unit} with additionalProperties: false, and the parameter enum has no minimum or maximum variants, so a daily minimum and maximum temperature cannot be distinguished at all. Nothing validates responses today, so it does not bite until a consumer checks. It needs a change to the packs.
• Deployment side is done separately, in OpenAgriNet/helmcharts on feat/4-docker-compose: the renamed ids, the same validator settings, and a pinned adapter image so the shipped default cannot meet a pre-rename build.
• Commit messages carry [#8] rather than [#16]. Issue Adapter: validate resourceAttributes against the schema packs, and name the plugins after their capabilities #16 was opened after the work, and the SHAs are referenced from it and from a published image tag, so rewriting them would cost more than the inconsistency.

Closes #16

[manjudr]

Review

Read the whole diff, then verified each substantive claim against the head tree and the published packs in network-specs.

The rename half is clean. No surviving weather/mandi plugin-id references anywhere in the head tree; install/build-plugins.sh, config/oan-provider-adapter.yaml (providerSteps + steps), package clauses and imports all move together; step resolution in core/module/handler/stdHandler.go:757 is an exact case-sensitive match; .github/workflows/build-and-deploy-plugins.yml derives the .so name from the directory basename, so it follows automatically. go vet and go test ./pkg/plugin/implementation/schemav2validator/... pass.

The per-key strip logic is sound. stripUnaccountedJSONLDKeys / schemaDeclaresProperty are cycle-guarded, non-mutating, and correctly exclude not and if. Asking the schema per key rather than adding a config switch is the right call, and the comment explaining why earns its length.

Every problem is in switching the layer on. Five findings inline — two on the config, one on the new test, two file-level on extended_schema.go (pre-existing code, but this PR is what makes it reachable, so it's in scope).

Suggested sequencing

The two config findings together mean the adapter rejects its own traffic the moment it deploys, and no test in the repo can catch it. Either:

1. fix the captured payloads + packs and add schemas.openagrinet.global to the allowlist before merging, or
2. land the validator code with extendedSchema_enabled: "false" and flip it in a follow-up, once a test exercises a real capture through the full schemav2validator path.

The $ref allowlist bypass should be fixed either way, independent of the flag.

[manjudr]

Enabling this rejects the repo's own verbatim captured /select payloads.

WeatherObservation/mappings_test.go:45 is labelled "the verbatim /select captured from the OAN network". Its resourceAttributes:

{
  "@context": "https://schemas.openagrinet.global/schema/WeatherObservation/v0.1/context.jsonld",
  "@type": "openagrinet:WeatherObservation",
  "subjectCategories": ["Weather"],
  "location": { "type": "Point", "coordinates": [73.7898, 19.9975] },
  "validity": { "startsAt": "2026-08-26", "endsAt": "2026-08-30" }
}

Two independent failures against AgricultureResource/v0.1/attributes.yaml:

1. informationMode is missing. attributes.yaml:17 puts required: [informationMode] on the root object — not under a then, so the if/then/else gap this block honestly documents at lines 141-145 does not excuse it.
2. validity.startsAt / endsAt are bare dates. attributes.yaml:138 and :143 declare format: date-time, and the validator is configured with EnableFormatValidation(). "2026-08-26" is not a date-time.

The mandi capture fails the same way.

What makes this worse than an ordinary bug: mappings_test.go calls the mapping step directly and never traverses schemav2validator, so CI stays green while the deployed adapter refuses the exact payload those tests assert it serves. Worth adding a test that pushes one capture through the full validator so this can't regress silently.

[ameersohel45]

Both failures are real. I verified each against the pack rather than the description, because "not under a then" is the whole load-bearing part of your first point:

• informationMode — required: [informationMode] sits at the root of AgricultureResource, confirmed by walking the schema and printing every required with its path. Root, not under if/then. So the if/then gap does not excuse it, exactly as you say.
• validity.startsAt/endsAt — format: date-time at pack lines 138 and 143, and EnableFormatValidation() is on at extended_schema.go:625. "2026-08-26" fails it.

Your point about why this is worse than an ordinary bug is the one I would most want kept: mappings_test.go calls the mapping step directly and never traverses schemav2validator, so CI stays green while the deployed adapter refuses the exact payload those tests assert it serves. That is a test suite that cannot see the failure it is meant to catch.

Not fixing it in this PR, and leaving this thread open. Payload and mapping changes are being batched separately from code fixes, and the end-to-end test you suggest belongs with them — a test pushing a capture through the full validator would fail today, so it lands with the payload fix or not at all.

One thing to settle before that batch, since it changes what the fix should be. Adding informationMode satisfies the required field, but the only permitted values are OnDemand and Direct, and OnDemand's then forbids location, market and validity — the exact fields a select must carry. So informationMode: "OnDemand" passes only because if/then is never evaluated, trading a visible non-conformance for an invisible one that a conformant validator would reject. The packs have no query mode; that gap is raised on PR #13 and is a spec change rather than ours.

Neither the pack nor the mappings are on a pinned version yet — the pack is a branch ref and MAPPING_URL points at a branch — so both are expected to move, and this gets revisited when they settle.

[manjudr]

This allowlist excludes the canonical @context host, so only non-canonical contexts can pass.

schemas.openagrinet.global is what the packs publish in their own x-jsonld and context.jsonld, and it is what every fixture on this branch uses — see WeatherObservation/mappings_test.go:62. With only raw.githubusercontent.com allowed, extended_schema.go:598 returns SCH_INVALID_JSONLD_CONTEXT before any fetch is attempted.

The net effect is inverted from the intent: a payload pointing at the canonical schema host is rejected, while a payload pointing at an arbitrary GitHub user's raw content is accepted. Add schemas.openagrinet.global, and keep raw.githubusercontent.com only if something actually needs it (see the file-level note on extended_schema.go).

[ameersohel45]

The diagnosis is right, and I checked every step of it: the fixtures on this branch do use schemas.openagrinet.global, the allowlist does permit only raw.githubusercontent.com, and extended_schema.go:598 does refuse before any fetch. Your "the net effect is inverted from the intent" is a fair reading of it.

Leaving the allowlist as it is for now, and leaving this thread open rather than closing it as handled. Config changes on this branch are being batched separately from code fixes, so this is deferred, not dismissed.

One thing that does change the remedy, though. I checked whether the canonical host serves anything, and schemas.openagrinet.global has no DNS at all, while raw.githubusercontent.com resolves.

So allowlisting the canonical host does not make those payloads validate — it converts an immediate, clearly-coded SCH_INVALID_JSONLD_CONTEXT into a fetch that waits out the download timeout and then fails as SCH_SCHEMA_ADAPTATION_FAILED. Same rejection, later and less legibly. In JSON-LD the @context is an identifier rather than a fetch target, which is why the packs can name a host that does not serve — but this validator resolves a schema from it, so it needs one that answers.

That makes it a choice between two things, and it is worth being explicit about which:

• allowlist the canonical host and fix the fixtures to point at a host that resolves — the identifier stays canonical, the fetch target has to be real;
• or keep the raw mirror as the @context and record in the config why the canonical host is deliberately absent.

Your point about arbitrary GitHub raw content stands under either, and it connects to the file-level $ref comment: raw.githubusercontent.com is world-writable, so allowlisting it trusts every GitHub account. Narrowing it to a path prefix or mirroring the packs somewhere we control would be the real fix, and neither is a one-line allowlist edit.

Also relevant to whatever gets chosen: the allowlist cannot shrink to one host once $refs are host-checked. Loading one pack pulls 15 documents across three hosts — the one named above plus two external spec hosts the packs $ref into — so all three have to be in it. Measured, not assumed; details in the reply on the $ref thread.

[manjudr]

Requesting changes

Formalising the previous review as a blocking one. The detail is in the inline comments there; this is the short version of what needs to change before merge.

Blocking:

1. config/oan-provider-adapter.yaml:154 — with extendedSchema_enabled: "true", the adapter rejects this repo's own verbatim captured /select payloads: informationMode is missing (required at AgricultureResource/v0.1/attributes.yaml:17, on the root object, not under a then), and validity.startsAt/endsAt are bare dates against format: date-time. Both the weather and mandi captures fail. mappings_test.go never traverses schemav2validator, so CI stays green while the deployed adapter refuses the exact payload those tests assert it serves.

2. config/oan-provider-adapter.yaml:174 — extendedSchema_allowedDomains omits schemas.openagrinet.global, the canonical @context host the packs publish and every fixture here uses. Only non-canonical contexts can pass.

3. extended_schema.go:598 / :345 — the allowlist gates only the top-level @context; $refs in the fetched schema reach freshReadFromURI with no host or scheme check. Since raw.githubusercontent.com is writable by any GitHub account, an unauthenticated select can drive arbitrary outbound fetches or local file reads. Pre-existing code, but this PR is what makes it reachable — should be fixed regardless of the flag's state.

Non-blocking but worth addressing: the array-form @type silent skip and the unreachable wrong-@type test, both in the inline comments.

Either fix (1) and (2) before merge, or land the validator code with extendedSchema_enabled: "false" and flip it in a follow-up once a test pushes a real capture through the full validator path. The rename half and the per-key JSON-LD strip logic are good to go as-is.

[ameersohel45]

Review round: what is fixed here, and what is deliberately not

All five comments were genuine — each verified against the code and the packs before changing anything, and each fix has a test that fails without it.

Fixed in this branch

Finding	Commit
$ref resolution bypassed the allowlist, allowing arbitrary local-file reads	63fc9bf
List-form @type was silently skipped, so those attributes were validated by nothing	59421ba
The @type const test asserted on a state the real path cannot produce	59421ba

Worth recording one measurement from the first of those, since it decided the scope: loading a single capability pack pulls 15 documents across 3 hosts, and the allowlist was checked on exactly one of them. The local-file read is now closed unconditionally, with no config change.

Deliberately not fixed here — threads left open

Two findings need a config or payload change, which this branch is batching separately from code fixes. Their threads are left unresolved rather than closed, so they are not mistaken for handled:

• the allowlist excluding the canonical @context host — and note schemas.openagrinet.global has no DNS, so allowlisting it alone would move the failure later rather than remove it;
• informationMode missing and validity carrying bare dates in the captured payloads, plus the end-to-end validator test that would catch it.

One thing not done that a reader might expect. Enforcing the allowlist on $ref hosts, not just the entry @context, is the right shape and is not in this PR: the packs $ref two external spec hosts, so host-checking refs against a one-host allowlist stops every pack loading. Measured, not assumed. TestValidateReferencedObject_AllowsARefToAHostOutsideTheAllowlist pins the current behaviour and explains why, so the change cannot be made by accident.

Neither the schema packs nor the mappings are on a pinned version yet, so both are expected to move.

[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 — 92% of changed lines covered, min 80%

[manjudr]

Code review

Found 2 issues (head 9a1594f32c1c45f99eeda9c7daf78332cabbfd28):

1. SSRF risk in the new schema fetcher — the host allowlist only covers the entry @context, not $ref targets it leads to. payloadDirectedReader restricts scheme (http/https only) but not host, and the code's own comment says so: "This does NOT restrict which hosts may be reached; isAllowedDomain still guards only the entry @context." @context is read straight out of the request payload, and the shipped allowlist (config/provider-adapter.yaml) is raw.githubusercontent.com — a public, anyone-can-publish host. A payload can point @context at an attacker-controlled file on that host, pass the allowlist, and have the returned document $ref any other http(s) host (e.g. an internal service or metadata endpoint), causing this process to fetch it server-side.

network-adapter/pkg/plugin/implementation/schemav2validator/extended_schema.go

Lines 452 to 462 in 9a1594f

	//
	// This does NOT restrict which hosts may be reached; isAllowedDomain still
	// guards only the entry @context. Enforcing the allowlist here as well is the
	// right shape, but the packs $ref two external spec hosts, so it needs those
	// named in the allowlist or no pack loads at all.
	func payloadDirectedReader(loader *openapi3.Loader, u *url.URL) ([]byte, error) {
	if u.Scheme != "http" && u.Scheme != "https" {
	return nil, fmt.Errorf("refusing to read schema from %q: only http and https are read for a location a payload chose", u.String())
	}
	return freshReadFromURI(loader, u)
	}

network-adapter/config/provider-adapter.yaml

Lines 191 to 211 in 9a1594f

	extendedSchema_enabled: "true"
	# Resolution is a FETCH of the @context each resource declares,
	# not a directory mounted here. The validator swaps context.jsonld
	# for attributes.yaml to get the schema beside it:
	#
	# @context .../schema/MandiPrice/v0.1/context.jsonld
	# fetched .../schema/MandiPrice/v0.1/attributes.yaml
	#
	# So a payload names the pack revision it wants to be judged
	# against, and no copy of the schemas here can drift from the
	# published ones. The packs' relative $refs (into
	# AgricultureResource) resolve against that same base; their
	# absolute ones resolve directly, against whichever host they name.
	#
	# Fetched once per @context and cached for the TTL below, so only
	# the first payload after a restart pays for it. A fetch that
	# FAILS rejects the payload -- it does not skip validation, which
	# is the right way round, but it does mean this adapter needs
	# egress to the host allowed below.
	extendedSchema_allowedDomains: "raw.githubusercontent.com"

2. Stale comment reference after the plugin rename — points at a path that no longer exists. weather/ was renamed to WeatherObservation/ in this PR, but this comment still says "the note in weather/prerequisites.go."

network-adapter/pkg/plugin/implementation/MandiPrice/prerequisites.go

Lines 14 to 19 in 9a1594f

	// An entry would be needed only for real I/O: a commodity name to resolve to a
	// code, a token to exchange, a point to turn into a market. Each of those is a
	// different upstream than the one this was written against, and each would
	// bring the question of where the provider-to-function binding belongs -- see
	// the note in weather/prerequisites.go and prefer keeping the payload explicit
	// over adding an entry here.

Separately, note that manjudr's CHANGES_REQUESTED review is still open, and its two stated blocking conditions are both unresolved at this head: extendedSchema_allowedDomains still excludes the canonical schemas.openagrinet.global host every fixture uses, and extendedSchema_enabled is "true" while the adapter's own captured payloads reportedly fail the new validation (missing informationMode, date format mismatch).

🤖 Generated with Claude Code

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

[ameersohel45]

Both fixed — and the first one changed my mind about a decision I'd defended

1 · SSRF via $ref — 5132832. You're right, and I'd argued the other way, so it's worth saying why I was wrong.

I had left this deliberately and pinned it with a test, on the grounds that host-enforcing refs breaks pack loading. That reasoning was sound about the mechanism and wrong about the conclusion: I treated "the allowlist can't be one host" as a reason not to check refs, when it was a reason to fix the allowlist. Your framing — that the returned document is not trusted because a payload named it, on a host anyone can publish to — is the part I'd underweighted.

The reach is the argument for fixing rather than documenting. Loading one pack pulls 13–16 documents, of which exactly one is the entry. The refs were never the corner case; they were the traffic.

The same allowlist is now checked on every read. Two refusals with different scopes, because they answer different questions:

• scheme — http/https only, and only on the payload-directed path. An operator who set extendedSchema_localSchemaPath is asking for files to be read. I initially installed it on both branches and TestLoadSchemaFromPath_LocalFile caught it.
• host — on both paths. localSchema falls back to the network for a ref it doesn't hold locally, so it had the same exposure by a longer route. That closes a gap neither of us had listed.

The allowlist had to grow, and this is a code+config pair that cannot land half-applied. Measured against the published packs, not assumed — every pack (WeatherObservation, MandiPrice, KnowledgeResource) touches raw.githubusercontent.com, schema.beckn.io and schema.nfh.global:

shipped 3-host allowlist  ->  loaded and validated
1-host allowlist          ->  SCH_SCHEMA_ADAPTATION_FAILED on every payload

The test that pinned the old behaviour is inverted, not deleted: it now asserts a cross-host $ref is refused, and a second test asserts the chain loads once both hosts are named — the case the packs actually need.

What this does not fix, and I'd rather name it than let it read as closed: raw.githubusercontent.com is world-writable, so the allowlist still trusts every GitHub account for schema content. A payload can name a schema it authored and be judged by it. Narrowing to a path prefix, or mirroring the packs on a host we control, is the real fix and is not a one-line change. It's written in the config as such.

One deployment coupling this creates: helmcharts sets the single-host allowlist in four places. Once an image carrying this code is built, those deployments need the widened value in the same pass or the provider adapter refuses every payload. The deployed image is deliberately pinned to an older commit today, so nothing is broken right now — flagging it so the two move together.

2 · Stale path after the rename — cca7ff2. Correct. MandiPrice/prerequisites.go still pointed at weather/prerequisites.go. Fixed, and I checked it was the last stale plugin path in any Go comment.

On the two open blocking conditions — both still stand, unchanged and deliberately:

• extendedSchema_allowedDomains excludes schemas.openagrinet.global, which has no DNS. Allowlisting it would convert an immediate coded rejection into a timeout, not a pass — so the fixtures need a host that resolves, or the config needs to record why the canonical host is absent.
• extendedSchema_enabled: "true" while the captured payloads omit informationMode (required at the root of AgricultureResource, so genuinely enforced) and send bare dates where validity wants date-time.

Both are payload/config work, batched separately from code fixes and tracked by the two unresolved threads above.

Verified: make test green — 64 packages, 0 failures, 0 races — changed-line coverage 92%, and both real packs load and validate through the new path.