fix(integrations): report a falsy non-mapping integration descriptor as a shape error - #4187
Open
Noor-ul-ain001 wants to merge 4 commits into
Open
Conversation
…as a shape error
`IntegrationDescriptor._load` did `yaml.safe_load(fh) or {}`. `_validate`
opens with an `isinstance(self.data, dict)` check, so a truthy non-mapping
(`- a`, `hello`) is reported correctly -- but `or {}` replaced the falsy
non-mappings with an empty mapping first, so those descriptors were
reported as "Missing required field: schema_version" instead of the wrong
shape:
'false' -> Descriptor root must be a YAML mapping, got bool
'0' -> Descriptor root must be a YAML mapping, got int
"''" -> Descriptor root must be a YAML mapping, got str
'[]' -> Descriptor root must be a YAML mapping, got list
`safe_load` also returns None for an explicit null scalar (`null`, `~`,
`NULL`) as well as for an empty document, so those three hit the same
masking. Use `yaml.compose`, which yields no node only for a genuinely
empty document, to tell the two apart -- only an empty document still
normalizes to `{}` and reports its missing fields.
Same bug class just fixed in the sibling overlay-manifest loader
(upstream commit 39c36c4, PR github#3884); this is the unfixed twin in the
integration catalog's descriptor loader.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes integration descriptor validation so falsy non-mapping YAML values report shape errors.
Changes:
- Distinguishes empty YAML from explicit null values.
- Adds regression coverage for non-mapping and empty descriptors.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/integrations/catalog.py |
Refines YAML loading and empty-document handling. |
tests/integrations/test_integration_catalog.py |
Tests descriptor shape errors and empty files. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
mnriem
requested changes
Aug 19, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IntegrationDescriptor._load(insrc/specify_cli/integrations/catalog.py) doesyaml.safe_load(fh) or {}._validateopens with anisinstance(self.data, dict)check, so a truthy non-mapping (- a,hello) is reported correctly as a shape error — butor {}replaces falsy non-mappings with an empty mapping before that check ever runs, so those descriptors were reported asMissing required field: schema_versioninstead of the actual problem:integration.ymlcontentfalseMissing required field: schema_versionDescriptor root must be a YAML mapping, got bool0Missing required field: schema_versionDescriptor root must be a YAML mapping, got int''Missing required field: schema_versionDescriptor root must be a YAML mapping, got str[]Missing required field: schema_versionDescriptor root must be a YAML mapping, got listnull/~/NULLMissing required field: schema_versionDescriptor root must be a YAML mapping, got NoneTypeMissing required field: schema_versionMissing required field: schema_versionsafe_loadreturnsNonefor both an explicit null scalar and a genuinely empty document, so a plaindata is Nonenormalization can't tell them apart either. This usesyaml.compose, which yields no node only for a genuinely empty document, to keep the empty-file case reporting its missing fields while every other non-mapping shape (including explicit null) now surfaces the real error.This is the same bug class a maintainer fixed the same day in the sibling overlay-manifest loader (
ProjectOverlaySource.collect, commit 39c36c4 / #3884) — that PR's description even calls out that the pattern should be checked elsewhere. This is the unfixed twin in the integration catalog's descriptor loader.Test plan
TestIntegrationDescriptor::test_falsy_non_mapping_descriptor_reports_shape_error(parametrized over[],false,0,'',null,~,NULL,- a,hello) andtest_empty_document_still_reports_missing_fieldspytest tests/integrations/test_integration_catalog.py— 133 passedruff checkon both changed files — clean🤖 Generated with Claude Code