[cmungall]

Summary

Tightens BindingValidationPlugin to enforce LinkML EnumBinding.obligation_level across all metamodel levels and support multi-level binds_value_of path resolution with . as the implicit default:

• Implicit . default & multi-level binds_value_of paths:
  • Omitting binds_value_of (or setting . / "") binds the slot's own value directly (single or multivalued scalar slots). Non-scalar values at . emit a clean binding_validation result instead of raising TypeError.
  • Multi-level nested dict paths (e.g. term.id, wrapper.term.id) resolve nested objects and track the immediate enclosing dict + its LinkML class so validate_labels checks the right label slot (implements: [rdfs:label], slot_uri, or label) and reports the exact nested path.
  • Auto-fan-out across list fields (coding.code / coding[*].code, as well as multivalued leaf lists) validates every item; explicit integer indices (extensions.0.value / extensions[0].value) constrain only that element.
• obligation_level enforcement:
  • REQUIRED (default when unset): ERROR when value is outside enum
  • RECOMMENDED (and ENCOURAGED alias): WARN when value is outside enum (without suppressing ERROR from Not4Curation or strict existence/label checks)
  • OPTIONAL / EXAMPLE: skips enum membership (and skips greedy dynamic-enum pre-expansion in pre_process when only referenced by OPTIONAL/EXAMPLE bindings)
  • DISCOURAGED: WARN when value is inside the enum
  • Deduplicates existence, Not4Curation, and label checks across multiple bindings composed on the same slot (REQUIRED + RECOMMENDED + DISCOURAGED), and respects severity_overrides.

Test plan

• Added 34 unit and CLI tests in tests/test_binding_obligations_and_nesting.py covering static/dynamic enums, progressive/greedy caching, all 5 obligation levels, multi-binding composition, scalar . bindings, nested dict paths, list fan-out, and explicit integer indices
• Added doctests in src/linkml_term_validator/plugins/binding_plugin.py
• Updated binding documentation in docs/binding-validation.md, docs/bindings-explained.md, and docs/plugin-reference.md

[claude]

Claude finished @cmungall's task in 1m 20s —— View job

---

PR #83 review: binding obligation levels and nested binds_value_of paths

• Read the diff and CLAUDE.md
• Review binding_plugin.py
• Review the tests and docs
• Run the tests. Not done: the runner wasn't allowed to run uv run pytest, so I only read the code. Please make sure CI is green.
• Post review

Overall this is well structured. Moving path walking into _extract_targets / _step_segment is easy to follow. Fixing the TypeError for non-scalar values at . is a good change. The test matrix (static/dynamic × progressive/greedy × obligation level) is broad. Below are the points worth looking at before merge, most important first.

---

Possible bugs and behavior changes

1. A missing bound field under REQUIRED is no longer reported (binding_plugin.py:426-429)
On main, _validate_binding reported "Required binding field '<path>' not found at <path>" when _extract_field returned None and the level was REQUIRED. That branch is gone. _extract_targets now returns [] and the binding produces no result at all.

• This may be on purpose: whether a value must be present is arguably the slot's required job, not the binding's. The old comparison obligation_level == "REQUIRED" may also never have matched a PermissibleValue object.
• The PR description doesn't mention the change, though, and no test covers it.
• Please either say in the PR and docs that a missing field is not a binding violation, or bring the check back using the normalized level.

2. Whether a Not4Curation result appears depends on binding order (binding_plugin.py:453-503)
hard_rejected_fields is filled one binding at a time. Take a slot with [RECOMMENDED E1, REQUIRED E2] and a Not4Curation term that is in E1 but not in E2:

• The RECOMMENDED binding runs first. The term isn't hard-rejected yet, so binding_not4curation (ERROR) is emitted.
• The REQUIRED binding then emits binding_validation (ERROR).
• Swap the two bindings and you get only the binding_validation error.

The run fails either way, so this is minor, but the report changes with schema order. One fix: run all membership checks for a target first, then do existence, Not4Curation and label checks once.

3. severity_overrides overrides the obligation level (binding_plugin.py:794-795)
severity_for(BINDING_VALIDATION, default=...) returns the override for every level. With {"binding_validation": "ERROR"}, a RECOMMENDED miss and a DISCOURAGED hit both become ERROR. With WARN, REQUIRED is demoted. This may be what you want, but it should be documented in docs/binding-validation.md and covered by a test. Otherwise users will expect "WARN for RECOMMENDED" to always hold.

4. Offline errors from an un-materialized enum ignore the obligation level (binding_plugin.py:872-887)
This branch uses severity_for(BINDING_VALIDATION) with no default, so it reports ERROR even for RECOMMENDED, and even for DISCOURAGED, whose result is supposed to be a warning only when the value is in the enum. Per #51, treating it as a cache problem is defensible. Two suggestions:

• Skip the error for DISCOURAGED, because "not in enum" is the good outcome there.
• Add a short comment saying it is ERROR on purpose regardless of level.

---

Code quality

• ENCOURAGED is not part of the LinkML metamodel. As far as I know, ObligationLevelEnum only has REQUIRED, RECOMMENDED, OPTIONAL, EXAMPLE and DISCOURAGED. The docstring (:359) calls it a "metamodel alias", and docs/binding-validation.md:82 documents it. Keeping it for leniency is fine, but please don't present it as part of the metamodel.
• Unknown levels raise ValueError (:380-384). This happens inside pre_process/process, so the user gets a traceback instead of a validation result. linkml_runtime probably rejects invalid enum values when the schema loads, which would make this mostly unreachable. If it can be reached, a clear schema-level error message would be friendlier.
• _extract_field is no longer used by the plugin itself (:714-750). Only doctests call it now. The expected_single_path logic used to decide between scalar and list is hard to read. Either remove it and move the doctests to _extract_targets, or document it as a debugging helper.
• _step_segment repeats the same block twice (:635-643 and :647-655). The "dict has key seg" logic appears for numeric and non-numeric segments. Checking if isinstance(curr_val, list) and seg.isdigit() first, then a single dict branch, would remove the copy.
• The cursor type is spelled out three times: tuple[Any, Optional[str], str, Optional[dict], Optional[str], str]. A second NamedTuple like _Cursor alongside _ExtractedTarget would make it self-documenting and easier to type-check.
• Style: _ExtractedTarget is followed by only one blank line before # Ontology properties… (:64-66). The VALID_OBLIGATION_LEVELS literal could fit on one line. just format runs only ruff check, so CI won't catch this, but ruff format would change it.

---

Performance

• Extra slot lookups per value. _validate_binding calls _get_slot_definition once per value per binding (:419). _step_segment calls it once per path segment per cursor, and each call scans class_induced_slots linearly. SchemaView caches the induced slots, so this is probably fine. For big datasets, a per-run (class, slot) -> range dict built in pre_process (next to bindings_map) would make these lookups O(1).
• _normalize_obligation_level runs per binding per value. It's cheap, but the level could be normalized once in pre_process and stored next to the binding.
• Skipping greedy pre-expansion for enums bound only via OPTIONAL/EXAMPLE (:211-219) avoids downloading ontologies for nothing.

---

Security

Nothing to flag. No new code turns exception text into messages, so the redact_secrets() rule (#75) doesn't come into play. Every new network access (label, existence, Not4Curation lookups) still goes through the existing OntologyAccess methods, so the retry handling still applies.

---

Test coverage

The coverage is broad and uses the offline simpleobo fixtures as CLAUDE.md asks, and the CLI test uses CliRunner. Missing cases:

• A REQUIRED binding whose bound field is absent (point 1). Pin down whichever behavior you choose.
• severity_overrides={"binding_validation": ...} combined with RECOMMENDED / DISCOURAGED (point 3).
• Two bindings in reversed order with a Not4Curation term (point 2).
• DISCOURAGED with an offline, un-materialized dynamic enum (point 4).
• Label checks on a fanned-out leaf list (coding.code where code is multivalued). The code sets label_parent=None there, so labels are skipped without any message. A test would document that.
• binds_value_of pointing at a key whose value is None or missing in only some list items (partial fan-out).

---

Docs

The docs match the code, apart from the ENCOURAGED wording above. Consider adding a line saying an explicit severity_overrides entry for binding_validation wins over the obligation level.

[claude]

Claude finished @cmungall's task in 1m 7s —— View job

---

Re-review of PR #83 after 763ee1e

• Read CLAUDE.md and the new commit
• Review the changes in binding_plugin.py
• Review the new tests and docs
• Run the tests. Not done: this runner has no uv and linkml isn't installed, so I only read the code. Please check that CI is green.
• Post review

The follow-up commit fixes every point from my first review. I found nothing that should block merging. Only small items remain.

How the earlier points were handled

#	Earlier point	Status
1	A missing bound field under REQUIRED	✅ Now documented: bindings only check values that are present, and required: true enforces presence. test_required_binding_skips_absent_or_null_bound_field locks this in.
2	Not4Curation result depended on binding order	✅ Fixed. _validate_bindings_for_value (binding_plugin.py:429) now runs in two passes: membership first, then existence, Not4Curation and label checks once each. The test runs both binding orders.
3	severity_overrides vs. obligation level	✅ Documented in both docs files and tested.
4	Offline un-materialized enum is ERROR at every level	✅ The comment now says this is on purpose. Tests cover RECOMMENDED/DISCOURAGED × progressive/greedy.
—	ENCOURAGED wording	✅ Removed from the YAML docs. The docstring now correctly says it is a metamodel alias of RECOMMENDED.
—	_PathCursor, the duplicated logic in _step_segment, and the precomputed range and level maps	✅ All done. _step_segment is now much easier to read.

Remaining small items (all optional)

1. _get_binding_range_class can pick the wrong class (binding_plugin.py:572-576, existing code)
The label check falls back to this function when label_class is None and parent_subpath == "". That happens when the slot's range isn't a class in the schema.

• The function returns the range of the first class in the schema that has a bound slot with the same name. It ignores target_class.
• It also loops over every class's induced slots, which undoes some of the O(1) lookup work in this commit.
• The case is rare, but if self._get_slot_class_range(target_class, slot_name) is None, the fallback would usually be None too. Dropping the fallback, or using self._slot_range_map.get((target_class, slot_name)), would be both correct and faster.

2. Some state is reset in pre_process and some isn't (binding_plugin.py:216-218)
_slot_range_map and _binding_levels are now cleared. bindings_map, slot_properties_map and expanded_enums are not. This was already true before the PR. But if a plugin instance is reused across two schemas, old bindings with matching (class, slot) names would still be applied. Clearing bindings_map and slot_properties_map next to the new .clear() calls costs nothing. expanded_enums might be meant to persist as a cache, so a short comment there would help.

3. Keying levels by id(binding) (:236)
This works because bindings_map keeps the binding objects alive, and there is a fallback for _validate_binding calls made outside pre_process. Still, it relies on object identity in a way that's easy to break. Storing (binding, level) pairs in bindings_map, or a parallel list per key, would be sturdier. Not urgent.

4. Behavior to keep in mind: existence checks now run for OPTIONAL/EXAMPLE targets
Pass 1 adds every string target to post_check_targets, including targets of bindings that skip membership. So in strict mode an OPTIONAL binding still causes existence and label lookups (Not4Curation only when the prefix is configured or validation is offline). This is reasonable: the value is still a CURIE, so it should exist. Consider one sentence in the docs so users know OPTIONAL means "skip membership" and not "skip all checks".

5. Wording leftovers

• The PR description still lists "RECOMMENDED (and ENCOURAGED alias)". That's accurate now that it's described as an alias, so it's only a note.
• The _validate_against_enum docstring (:837) also lists RECOMMENDED / ENCOURAGED, which is fine.
• _extract_field (:801) is still only used by its doctests. It's now labelled a convenience helper, which is acceptable.

Security

Nothing new. No new exception text reaches messages, so the redact_secrets() rule (#75) isn't involved. Every network access still goes through OntologyAccess, so the retry handling still applies.

Test coverage

The new tests cover each gap I listed before: absent/null fields, override precedence, binding order, offline DISCOURAGED/RECOMMENDED, labels on a fanned-out leaf list, and partial fan-out with preserved indices. One more test would be useful: an OPTIONAL binding in strict mode with a non-existent CURIE, to lock in point 4 whichever way you choose.

[claude]

Claude finished @cmungall's task in 1m 42s —— View job

---

Re-review of PR #83 after 2d8612b

• Read CLAUDE.md and the new commit (git show 2d8612b)
• Review the changes in binding_plugin.py
• Review the new tests and the docs change
• Run the tests. Not done: linkml isn't installed on this runner, so I only read the code. Please check that CI is green.
• Post review

This commit deals with every optional item from my last review. I found nothing that blocks merging. LGTM.

How the earlier items were handled

#	Earlier item	Status
1	_get_binding_range_class could pick another class's range	✅ The label check now passes target.label_class straight through, with no fallback across classes. test_plugin_reuse_across_schemas_resets_state_and_scopes_range_class checks that TargetClass.term uses the plain label slot and does not borrow custom_title from UnrelatedClass.
2	Some state was reset in pre_process and some wasn't	✅ bindings_map, slot_properties_map and expanded_enums are now cleared too. The comment says the disk caches still persist across runs. Clearing expanded_enums costs nothing extra: greedy mode expands again in the same pre_process, and progressive mode only reads it at :911.
3	Obligation levels were keyed by id(binding)	✅ They now live in _slot_binding_levels[key], a list in the same order as bindings_map[key]. _validate_binding still works when called directly, because it falls back to normalizing the level itself (:481-487).
4	OPTIONAL/EXAMPLE still run existence and label checks	✅ Documented in docs/binding-validation.md:97, and the new parametrized test covers it: term_not_found in strict mode, and binding_label_mismatch with no binding_validation.

Remaining small items (all optional)

1. _get_binding_range_class is now dead code (binding_plugin.py:605-621). Nothing in src/ or tests/ calls it since the fallback was removed. You could delete it. If you keep it as a public helper, it needs a test, because its target_class=None branch still scans every class and returns the first match.
2. The two lists must stay in step. bindings_map[key] and _slot_binding_levels[key] are matched by position. That is fine because only pre_process writes them. The idx < len(obligation_levels) guard keeps a mismatch from crashing, but a mismatch would quietly fall back to normalizing. Storing list[tuple[binding, level]] would make this structural, but that would change bindings_map's public shape, so leaving it as is seems fine.
3. Plugin reuse test: it checks that ("Annotation", "term") is gone and that expanded_enums == {} for a static-only schema. You could also assert that plugin.slot_properties_map has no "Annotation" key after the second pre_process, so all three resets are covered.

Code quality, performance, security

• The pass-2 tuple was simplified to list[_ExtractedTarget] now that binding is no longer needed there. Good.
• Performance: label lookup no longer scans every class's induced slots per target, so the O(1) range map is used everywhere it matters.
• Security: nothing new. No exception text reaches messages, so redact_secrets() (API key printed in error output when a BioPortal lookup fails #75) doesn't come into play, and network access still goes through OntologyAccess and its retry handling.

Test coverage

The two new tests cover the behavior from my last round: OPTIONAL/EXAMPLE with strict existence and label checks, plugin reuse, and the range lookup scoped to target_class. I see no gaps that need tests before merging.