Skip to content

fix(integrations): wrap a non-UTF-8 catalog response - #4011

Merged
mnriem merged 3 commits into
github:mainfrom
Noor-ul-ain001:fix/integration-catalog-non-utf8
Aug 7, 2026
Merged

fix(integrations): wrap a non-UTF-8 catalog response#4011
mnriem merged 3 commits into
github:mainfrom
Noor-ul-ain001:fix/integration-catalog-non-utf8

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Problem

IntegrationCatalog._fetch_single_catalog decodes the response body before parsing it:

catalog_data = json.loads(
    read_response_limited(
        resp,
        max_bytes=MAX_JSON_METADATA_BYTES,
        error_type=IntegrationCatalogError,
        label=f"catalog from {entry.url}",
    ).decode("utf-8")
)

A non-UTF-8 body raises UnicodeDecodeError at .decode(), before json.loads ever runs. UnicodeDecodeError and json.JSONDecodeError are siblings under ValueError, not parent/child:

UnicodeDecodeError -> UnicodeError -> ValueError
json.JSONDecodeError -> ValueError

So neither the except urllib.error.URLError nor the except json.JSONDecodeError handler catches it, and the raw exception escapes the method.

Why it matters

The caller, _get_merged_integrations, is explicitly written to tolerate one bad catalog:

except IntegrationCatalogError as exc:
    print(f"Warning: Could not fetch catalog '{entry.name}': {exc}", file=sys.stderr)
    continue

Because UnicodeDecodeError is not an IntegrationCatalogError, that resilience is bypassed. A single catalog served through a misconfigured proxy, or truncated mid-multibyte-sequence, aborts specify integration search with a bare traceback instead of degrading to a warning — even when every other configured catalog is healthy.

Worth noting the cache-read path in this same method already handles this, via UnicodeError in its except tuple. Only the network path was unguarded, so the failure is inconsistent between a cache hit and a cache miss.

Fix

Wrap it in the module's own error type. This matches the convention already established in authentication/azure_devops.py, which names UnicodeDecodeError alongside JSONDecodeError around the identical read_response_limited(...).decode("utf-8") call.

Scope checked, deliberately not changed

I swept every read_response_limited(...).decode("utf-8") call site:

  • workflows/catalog.py (both the workflow and step catalog fetches) and bundler/services/adapters.py — already covered by a broad except Exception.
  • authentication/azure_devops.py — already names UnicodeDecodeError.
  • _version.py::_fetch_latest_release_tagintentionally left alone. Its docstring states: "On anything else — including a malformed response body — the exception propagates; there is no catch-all (research D-006)." That is a deliberate design decision, not a gap.

integrations/catalog.py was the one genuine outlier.

Tests

Two regression tests in TestCatalogFetch:

  • test_fetch_wraps_non_utf8_catalog_response — pins the wrapped-error contract.
  • test_search_skips_non_utf8_catalog — covers the behaviour that actually motivates the fix: a broken catalog is skipped with a warning on stderr while a healthy sibling catalog still resolves.

Both fail on main with the raw UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 35: invalid start byte, and pass with the fix. Full file suite: 123 passed. ruff check clean.

The new tests need raw bytes on the wire, which the existing _patch_urlopen helper cannot express (it JSON-encodes its input), so they use a small sibling helper that patches open_url to serve raw bodies keyed by URL.

🤖 Generated with Claude Code

`_fetch_single_catalog` decodes the response body with `.decode("utf-8")`
before handing it to `json.loads`. A non-UTF-8 body therefore raises
`UnicodeDecodeError`, which is a sibling of `json.JSONDecodeError` under
`ValueError` rather than a subclass of it, so neither the `URLError` nor the
`JSONDecodeError` handler catches it.

The raw exception escapes `_get_merged_integrations`, whose
`except IntegrationCatalogError` is specifically designed to warn and skip a
bad catalog and carry on with the remaining ones. One catalog served over a
misconfigured proxy or truncated mid-multibyte-sequence thus takes down
`specify integration search` entirely instead of degrading to a warning.

Wrap it in `IntegrationCatalogError`, matching the convention already used
for the same decode in `authentication/azure_devops.py`, which lists
`UnicodeDecodeError` alongside `JSONDecodeError`.

Note that the cache-read path in this same method already tolerates this via
its `UnicodeError` clause; only the network path was unguarded.

Two regression tests: one pins the wrapped-error contract on the fetch, and
one covers the behaviour that actually motivates it — a broken catalog is
skipped with a warning while a healthy sibling catalog still resolves.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Noor-ul-ain001
Noor-ul-ain001 requested a review from mnriem as a code owner August 7, 2026 15:41
Noor-ul-ain001 and others added 2 commits August 7, 2026 20:50
The raw-bytes helper patched `open_url` wholesale, which skipped the real
URL validation and redirect handling inside it. This module already imports
`route_opener_open_through_urlopen`, the repo's shared fixture that routes
`build_opener().open()` back through `urlopen` for exactly this reason, so
patching `urlopen` instead keeps the stub effective while still exercising
`open_url` itself.

Renamed to `_patch_urlopen_bytes` to sit alongside the existing
`_patch_urlopen`, whose signature it now mirrors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit reverted the source change by accident while reworking
the tests, leaving the regression tests passing against an unfixed module.
Restores the `except UnicodeDecodeError` clause.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mnriem
mnriem requested a balanced review from Copilot August 7, 2026 17:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Wraps non-UTF-8 integration catalog responses in IntegrationCatalogError, preserving multi-catalog search resilience.

Changes:

  • Converts UnicodeDecodeError into a catalog-specific error.
  • Adds regression coverage for direct fetching and skipping malformed catalogs.
Show a summary per file
File Description
src/specify_cli/integrations/catalog.py Handles non-UTF-8 network responses.
tests/integrations/test_integration_catalog.py Tests error wrapping and fallback behavior.

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: 0
  • Review effort level: Balanced

@mnriem
mnriem merged commit 2a28f62 into github:main Aug 7, 2026
14 checks passed
@mnriem

mnriem commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants