Validate a name-matching builder against its named spec instead of a wildcard sibling

[adityasingh2400]

Motivation

Fix #1554

When a group spec has an optional named sub-spec with a data type (name: col1, data_type_inc: TypedVector, quantity: '?') next to a sibling wildcard sub-spec of the parent type (data_type_inc: BaseVector, quantity: '*'), a builder named col1 whose type is only BaseVector was silently absorbed by the wildcard spec and validated clean.

The root cause is the order of the two filters in SpecMatcher._best_matching_spec. _filter_by_name kept both the named col1 spec and the unnamed wildcard, then _filter_by_type dropped the named spec because the builder's type is not a subtype of TypedVector. The name match was therefore discarded rather than treated as authoritative, and the builder fell through to the wildcard. The named spec ended up with zero matched builders, and since it is optional, __validate_presence_and_quantity reported nothing. Enforcement of a named typed sub-spec depended entirely on whether it happened to be required.

This is the mechanism behind the NWB case in the issue, where an EventsTable whose duration column is a plain VectorData rather than a DurationVectorData validates with no errors.

The fix

_filter_by_name now short-circuits, which is the option suggested in the issue. If any candidate spec names the builder explicitly, only those candidates are kept and the unnamed ones are dropped, so a name-matching builder can no longer fall through to a wildcard sibling.

That alone would leave the builder simply unmatched, which is still silent for an optional spec, so SpecMatcher also records builders that matched a named spec by name but not by type. GroupValidator reports each of those as an IncorrectDataType error, an error class that already existed for reference-target type mismatches. A required named spec now reports the type mismatch directly instead of the indirect MissingDataType it produced before.

How to test the behavior?

# the repro from the issue, on this branch
print(vmap.validate(group(dset("col1", "TypedVector"))))  # []                          correct
print(vmap.validate(group(dset("col1", "BaseVector"))))   # [Container/col1 (c): incorrect data_type - expected 'TypedVector', got 'BaseVector']
print(vmap.validate(group(dset("other", "BaseVector"))))  # []                          correct, col1 is optional

TestNamedSubspecTakesPrecedenceOverWildcard in tests/unit/validator_tests/test_validate.py covers this. With the source change reverted and the test kept, the two mismatch cases fail:

tests/unit/validator_tests/test_validate.py::TestNamedSubspecTakesPrecedenceOverWildcard::test_optional_named_subspec_with_wrong_type_returns_error
    AssertionError: 0 != 1
tests/unit/validator_tests/test_validate.py::TestNamedSubspecTakesPrecedenceOverWildcard::test_required_named_subspec_with_wrong_type_returns_error
    AssertionError: False is not true
2 failed, 3 passed

With the fix applied, all 5 pass, and the full suite is clean: 1869 passed, 118 skipped, 1 xfailed, 919 subtests passed.

Checklist

• Did you update CHANGELOG.md with your changes?
• Does the PR clearly describe the problem and the solution?
• Have you reviewed our Contributing Guide?
• Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.13%. Comparing base (4a8967f) to head (c8fe923).

Files with missing lines	Patch %	Lines
src/hdmf/validate/validator.py	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1556      +/-   ##
==========================================
- Coverage   93.15%   93.13%   -0.03%     
==========================================
  Files          41       41              
  Lines       10245    10263      +18     
  Branches     2119     2124       +5     
==========================================
+ Hits         9544     9558      +14     
- Misses        422      424       +2     
- Partials      279      281       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adityasingh2400]

Flagging a problem with this PR that I do not think I should decide on my own.

run-pynwb-tests is the only red check, with every other job green including run-hdmf-zarr-tests. It is 3 errors out of 748 tests, all in back_compat/test_read.py, and all three are validation errors this change introduces:

3.0.0_electrodes_dynamic_table.nwb:
  root/general/extracellular_ephys/electrodes
  incorrect data_type - expected 'ElectrodesTable', got 'DynamicTable'

3.0.0_decompositionseries_bands_dynamic_table.nwb:
  DecompositionSeries/bands
  incorrect data_type - expected 'FrequencyBandsTable', got 'DynamicTable'

3.0.0_optogenetics_extension.nwb:
  Device/model
  incorrect data_type - expected 'DeviceModel', got 'OpticalFiberModel'

The first two are exactly the shape this PR is meant to catch, and that is the awkward part. In both, the builder carries the parent type where the current schema names a child type, DynamicTable where ElectrodesTable or FrequencyBandsTable is named. That is structurally the same as the case in #1554 that motivated the change, where a duration column is a plain VectorData rather than a DurationVectorData.

So I do not think there is a way to keep the #1554 fix and keep these files validating clean. They are the same situation. The difference is only that one is a schema bug and the others are files written before the schema was refined, and the validator has no way to tell those apart from the builder and spec alone.

That makes this a policy question rather than something I can patch around, so I would rather ask than guess:

1. Should reading an older file that predates a type refinement now produce a validation error? If yes, these three pynwb fixtures need updating upstream and this PR has to be coordinated with that.
2. Or should the named-spec match stay authoritative only when the builder's type is unrelated to the named type, and keep tolerating the case where the builder holds an ancestor of it? That preserves back compat, but it also declines to catch Validator lets a name-matching builder fall through to a wildcard spec instead of checking the named typed spec #1554, so I would want to know that is the intent before writing it.

The third error looks different from the other two and I could not fully account for it. There the builder holds OpticalFiberModel where DeviceModel is expected, which is the subtype direction and should pass _filter_by_type through vmap.valid_types, so I suspect the extension types are not registered in the type map on that path. I did not want to assert that without being able to confirm it.

Happy to implement either direction once you tell me which one you want.

[rly]

Hi @adityasingh2400, thanks for the contribution! The PR is a good start, and I will respond to your comment in a follow-up comment.

In the age of coding agents, we want to be careful about how we handle contributions and are starting to implement steps to ensure that all changes are properly reviewed and validated.

Your account opened 133 pull requests today across 57 repositories. Based on this volume (and tells in the PR text), these PRs appear to be largely generated and submitted by an agent.

We are not opposed to AI-assisted contributions. Most of my own recent PRs and issues here are co-authored by a coding agent. But, we require that every change is accountable to two humans, an author and a reviewer. Autonomous PRs without human author oversight shift the entire review burden and accountability onto maintainers.

What is your review process? Could you let me know how much you, the human, reviewed the code changes here, and how much you understand them and their impact on the codebase and users? Same for your other PRs (e.g., #1557, NeurodataWithoutBorders/pynwb#2240, NeurodataWithoutBorders/pynwb#2239, which I will also review). If they were submitted autonomously without human review, please say so. Thank you.