fix(extensions): tolerate non-string catalog name in display-name lookup - #3747
Merged
mnriem merged 3 commits intoJul 27, 2026
Merged
Conversation
_resolve_catalog_extension() filters catalog search results by display
name with `ext["name"].lower() == argument.lower()`. Extension catalog
JSON is user-editable, so a hand-authored non-string name (e.g.
`name: 123`) crashes the filter with `AttributeError: 'int' object has
no attribute 'lower'`, taking down `extension info <name>` and
`extension add <name>`. A missing `name` key would likewise KeyError.
Coerce defensively with `str(ext.get("name", "")).lower()`, matching the
ambiguous-match display block just below (which already str()-coerces
name for the same reason). A bad-named entry simply doesn't match,
yielding a clean not-found error instead of a traceback.
Adds a regression test invoking `extension info <name>` against a
mocked catalog whose search result has `name: 123`; it fails pre-fix
with AttributeError.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens extension display-name lookup against malformed catalog names.
Changes:
- Coerces catalog names before case-insensitive comparison.
- Adds a regression test for numeric names.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/_commands.py |
Hardens display-name matching. |
tests/test_extensions.py |
Adds malformed-name coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/specify_cli/extensions/_commands.py:173
- This comment says the argument is passed as a search query, but the call now deliberately fetches all entries. That distinction is important:
ExtensionCatalog.search(query=...)joins the user-editablenameinto searchable text and can raiseTypeErrorbefore this defensive filter runs. Document why the no-query call is required so it is not “corrected” back into the production crash.
# Try by display name - search using argument as query, then filter for exact match.
# Coerce name defensively: catalog JSON is user-editable, so a hand-authored
# non-string/missing name must not crash the match (the ambiguous-match display
# below already str()-coerces name for the same reason).
search_results = catalog.search()
tests/test_extensions.py:6556
- These assertions accept any raw failure other than
AttributeError, so aTypeError/KeyErrortraceback still passes despite the promised clean not-found path. TheMagicMockalso accepts bothsearch(query=...)andsearch(), leaving the production-critical no-query behavior unguarded. Assert the expected CLI output and exact catalog call.
# match, so resolution ends as a clean not-found error exit.
assert not isinstance(result.exception, AttributeError), (
f"non-string catalog name crashed resolution: {result.exception!r}"
)
assert result.exit_code != 0
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
mnriem
requested changes
Jul 27, 2026
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>
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/specify_cli/extensions/_commands.py:172
- This comment no longer describes the implementation:
catalog.search()is deliberately called withoutargumentbecause the real search implementation tries to join the malformed name before this filter runs. Please document that unfiltered lookup so a future cleanup does not restore the crashingquery=argumentcall.
# Try by display name - search using argument as query, then filter for exact match.
# Coerce name defensively: catalog JSON is user-editable, so a hand-authored
# non-string/missing name must not crash the match (the ambiguous-match display
# below already str()-coerces name for the same reason).
tests/test_extensions.py:6556
- These assertions do not establish the promised clean not-found path: a
KeyError,TypeError, or another raw exception would still satisfy both. The mock also returns the same data forsearch(query=...), so the test would miss reintroducing the real catalog's pre-filter crash while joining a numeric name. Assert the user-facing error and that search was called without a query.
assert not isinstance(result.exception, AttributeError), (
f"non-string catalog name crashed resolution: {result.exception!r}"
)
assert result.exit_code != 0
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Medium
mnriem
self-requested a review
July 27, 2026 20:00
mnriem
approved these changes
Jul 27, 2026
mnriem
pushed a commit
that referenced
this pull request
Jul 28, 2026
…nfo display (#3770) `workflow search`, `workflow info`, `extension search` and `extension info` crashed with `TypeError: 'int' object is not iterable` when a catalog entry carried a scalar `tags:` value (e.g. `tags: 5`). Catalog payloads are user-editable YAML/JSON, so this shape reaches the display unvalidated. Both backends already guard their tag *filter* with `isinstance(raw_tags, list)` — `WorkflowCatalog.search` and `ExtensionCatalog.search` skip a non-list `tags` cleanly. Only the display paths were unguarded: they tested truthiness (`if info.get("tags"):`) and then iterated. A scalar is truthy but not iterable, so `--tag` filtering survived while plain `search`/`info` rendering blew up. Note this is distinct from the non-string *element* handling added in #3746/#3747: coercing elements with `str(t) for t in ...` does not help when `tags` is not a sequence at all. The fix is the guard the sibling integration commands already use — `integrations/_query_commands.py:332,402` gate on `isinstance(tags, list) and tags`. This aligns workflows and extensions with that reference pattern, leaving all four tag-join display sites in these modules consistent. Regression tests drive the full CLI via CliRunner and cover search + info in one case per module; both fail before the fix with the exact TypeError. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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
_resolve_catalog_extension()(used byextension info <name>andextension add <name>) resolves an argument to a catalog entry by display name:Extension catalog JSON is user-editable, so
catalog.search()can return an entry whosenameis a non-string. A hand-authoredname: 123crashes the filter:which takes down
extension info <name>/extension add <name>with a raw traceback. A missingnamekey would similarly raiseKeyError.Notably, the ambiguous-match display block a few lines below already
str()-coerces the name (_escape_markup(str(ext.get("name", "")))) — so the code already acknowledgesnamemay be non-string/absent; only the filter line missed it.Fix
Coerce defensively with
str(ext.get("name", "")).lower(), matching that sibling block. A bad-named entry simply fails to match, producing a clean not-found error instead of a traceback. Same bug class as the presets fix in #3743.Testing
Adds
test_add_by_name_tolerates_non_string_catalog_name, which invokesextension info <name>against a mocked catalog whose search result hasname: 123. It fails pre-fix withAttributeErrorand passes after; the existing display-name resolution test stays green.🤖 Generated with Claude Code
@