Skip to content

fix(extensions): guard the required manifest sections so one bad extension cannot break extension list - #3797

Merged
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/extension-manifest-section-guards
Jul 29, 2026
Merged

fix(extensions): guard the required manifest sections so one bad extension cannot break extension list#3797
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/extension-manifest-section-guards

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

ExtensionManifest.REQUIRED_FIELDS only checks key presence:

for field in self.REQUIRED_FIELDS:      # schema_version, extension, requires, provides
    if field not in self.data:
        raise ValidationError(...)

So a section that is written but left empty (provides:None, which is just how YAML spells an empty field) or given the wrong shape passes that check and then fails on first use:

extension: null  -> TypeError: argument of type 'NoneType' is not iterable
requires:  null  -> TypeError: argument of type 'NoneType' is not iterable
provides:  null  -> AttributeError: 'NoneType' object has no attribute 'get'
provides:  []    -> AttributeError: 'list' object has no attribute 'get'

Neither is a ValidationError, so both escape the callers that already handle malformed manifests. list_installed() catches ValidationError only and has a deliberate ⚠️ Corrupted extension fallback — so a single bad extension took down the entire command. Reproduced end-to-end with one broken and one good extension installed:

specify extension list
before exit 1, raw AttributeError, no output at all
after exit 0 — the good extension is listed, the bad one shows as Corrupted extension

Fix

Add an isinstance guard for each required section. This mirrors the guards already in the same function for the nested optional keys —

if "commands" in provides and not isinstance(commands, list):
    raise ValidationError("Invalid provides.commands: expected a list")
if "hooks" in self.data and not isinstance(hooks, dict):
    raise ValidationError("Invalid hooks: expected a mapping")

— and _load_yaml's document-root check. Only the three required sections lacked one; they were the odd ones out.

Not a behaviour change for valid input

provides: {} is a well-shaped mapping, so the new check does not touch it:

  • with hooks present → still validates (an extension may provide only hooks)
  • with no hooks → still gets the pre-existing must provide at least one command or hook

Both are locked by regression tests. I got this wrong on the first attempt — my initial test asserted provides: {} must raise, which failed because the fixture defines hooks; the tests now assert the correct behaviour.

Verification

  • test_required_section_not_mapping_rejected — 3 sections × 3 bad shapes (None, [], "text"): 9 fail before, all pass after.
  • 2 regression guards for the provides: {} cases above.
  • TestExtensionManifest: 55 passed.
  • uvx [email protected] check src tests clean.

AI-assisted: authored with Claude Code. I reproduced the extension list exit-1 crash end-to-end (correct .registry layout, one broken + one good extension) and confirmed the graceful fallback now engages, rather than only testing the manifest in isolation.

@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 28, 2026 14:20
…nsion cannot break `extension list`

ExtensionManifest.REQUIRED_FIELDS only checks key PRESENCE, so a section that is
written but left empty (`provides:` -> None) or given the wrong shape
(`provides: []`) passes it and then fails on first use:

    extension: null  -> TypeError: argument of type 'NoneType' is not iterable
    requires:  null  -> TypeError: argument of type 'NoneType' is not iterable
    provides:  null  -> AttributeError: 'NoneType' object has no attribute 'get'
    provides:  []    -> AttributeError: 'list' object has no attribute 'get'

Neither is a ValidationError, so both escape the callers that already handle
malformed manifests. list_installed() catches ValidationError only and has a
deliberate "Corrupted extension" fallback, so a single bad extension took down
the whole command -- reproduced end-to-end:

    before: specify extension list -> exit 1, raw AttributeError, no output
    after:  specify extension list -> exit 0, the good extension listed, the
            bad one shown as "Corrupted extension"

Add an isinstance guard for each required section, mirroring the nested guards
already in this function ("Invalid provides.commands: expected a list", "Invalid
hooks: expected a mapping") and _load_yaml's document-root check. Only the three
REQUIRED sections lacked one.

`provides: {}` is unaffected: it is a well-shaped mapping, so an extension that
provides only hooks still validates, and with no hooks it keeps the pre-existing
"must provide at least one command or hook" message. Both are locked by tests.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@jawwad-ali
jawwad-ali force-pushed the fix/extension-manifest-section-guards branch from 85b1b67 to e2cf3ee Compare July 28, 2026 14:57
@mnriem
mnriem requested a review from Copilot July 28, 2026 21:34

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

Adds manifest section type validation so malformed extensions raise ValidationError and do not break extension listing.

Changes:

  • Validate extension, requires, and provides as mappings.
  • Add malformed-shape and empty-provides regression tests.
Show a summary per file
File Description
src/specify_cli/extensions/__init__.py Guards required manifest sections.
tests/test_extensions.py Covers invalid shapes and valid empty mappings.

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: Medium

@mnriem
mnriem merged commit 1fff7a1 into github:main Jul 29, 2026
14 checks passed
@mnriem

mnriem commented Jul 29, 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