fix(catalogs): reject boolean priority in extension and preset catalo…#2589
Open
Quratulain-bilal wants to merge 2 commits into
Open
fix(catalogs): reject boolean priority in extension and preset catalo…#2589Quratulain-bilal wants to merge 2 commits into
Quratulain-bilal wants to merge 2 commits into
Conversation
…g readers
`bool` is a subclass of `int` in Python, so `int(True)` silently returns
`1`. The extension- and preset-catalog config readers coerced priority
with a bare `int(item.get("priority", idx + 1))`, which meant a YAML
config like:
catalogs:
- name: mine
url: https://example.com/catalog.json
priority: yes # parses to True
was silently accepted as a valid priority of 1, quietly reordering the
catalog stack instead of raising the same `Invalid priority` error a
typo of `priority: not-a-number` already raises.
The sibling integration-catalog reader in `src/specify_cli/catalogs.py`
already guards this case (see `catalogs.py:137`). This change mirrors
that pattern in `extensions.py` and `presets.py` so the three catalog
validators stay consistent, and adds regression tests for both readers
matching the existing `test_load_catalog_config_rejects_boolean_priority`
template in `tests/integrations/test_integration_catalog.py`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens catalog config validation so extension and preset catalog readers reject boolean priority values consistently with the integration catalog reader.
Changes:
- Adds explicit
boolchecks before integer coercion in extension and preset catalog config loading. - Updates invalid-priority error reporting to use the captured raw priority value.
- Adds regression tests for boolean priority rejection in extension and preset catalogs.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions.py |
Rejects boolean catalog priorities before int(...) coercion. |
src/specify_cli/presets.py |
Applies the same boolean priority validation for preset catalogs. |
tests/test_extensions.py |
Adds coverage for extension catalog boolean priority rejection. |
tests/test_presets.py |
Adds coverage for preset catalog boolean priority rejection. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
…bool-guard # Conflicts: # src/specify_cli/extensions.py # tests/test_extensions.py
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.
What
The extension- and preset-catalog config readers silently accept a boolean
priorityvalue. Becauseboolis asubclass of
intin Python, a config like:is coerced via
int(True) == 1and quietly treated as a valid priority of 1 — silently reordering the catalog stackinstead of raising the same
Invalid priorityerror that a typo likepriority: "fast"already raises.The sibling integration-catalog reader in
src/specify_cli/catalogs.pyalready guards this case (seecatalogs.py:137,plus the existing
test_load_catalog_config_rejects_boolean_priorityintests/integrations/test_integration_catalog.py). The extension and preset readers had drifted from that precedent.Why this matters
priority: yes/priority: no/priority: trueare easy YAML typos — silently accepting them changes catalogresolution order without any warning, which is a frustrating bug to diagnose because the config "looks valid" and no
error fires.
catalogs.py,extensions.py,presets.py) validate the same shape and should fail inthe same way. Today, the same malformed input raises in one and is silently accepted in two.
The change
src/specify_cli/extensions.pyaddisinstance(raw_priority, bool)guard beforeint(...)inExtensionCatalog._load_catalog_config, raisingValidationErrorwith the same message shape used for other invalidpriorities.
src/specify_cli/presets.pysame guard inPresetCatalog._load_catalog_config, raisingPresetValidationError.catalogs.py:137pattern exactly, with a short comment explaining thebool-is-intsubtlety so the rationale is visible to the next reader.Tests
tests/test_extensions.py—test_load_catalog_config_rejects_boolean_prioritytests/test_presets.py—test_load_catalog_config_rejects_boolean_priorityBoth follow the template of the existing integration-catalog regression test, so the three catalog validators now have
parallel coverage.
Scope
Intentionally small: no behaviour change for any priority that was previously accepted (real integers, numeric strings,
missing values all behave exactly as before). Only
priority: true/priority: falsewhich were almost certainlynever intended as priority 1 / priority 0 now raise a clear error.