fix: narrow exception in extension registration fallback from Exception to specific types - #3919
Quratulain-bilal wants to merge 2 commits into
Conversation
…on to specific types Bare 'except Exception' silently swallows all errors. Narrow to (TypeError, ValueError, KeyError) which are the realistic failure modes in extension registration.
There was a problem hiding this comment.
Pull request overview
Narrows extension-registration fallback exceptions to avoid swallowing unexpected errors.
Changes:
- Replaces broad
Exceptionhandling with specific exception types. - However, omits the expected
ValidationErrorfrom invalid manifests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/presets/__init__.py |
Narrows extension registration fallback exceptions. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| record_written(written) | ||
| registered = True | ||
| except Exception: | ||
| except (TypeError, ValueError, KeyError): |
…on to specific types Catch (TypeError, ValueError, KeyError, ValidationError) instead of bare Exception. ExtensionManifest raises ValidationError (not ValueError) for malformed/unreadable manifests, so it must be caught to preserve the fallback to generic path-based registration. Add regression test verifying ValidationError during manifest load is silently skipped. Co-authored-by: GitHub Copilot (model: mimo-v2.5-free, supervised)
|
|
||
| def test_resolve_extension_command_via_manifest_skips_validation_error(self, project_dir): | ||
| """resolve_extension_command_via_manifest skips extensions whose manifest raises ValidationError.""" | ||
| import unittest.mock as mock |
| def test_resolve_extension_command_via_manifest_skips_validation_error(self, project_dir): | ||
| """resolve_extension_command_via_manifest skips extensions whose manifest raises ValidationError.""" |
mnriem
added
triage-can-wait
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Thanks @Quratulain-bilal . A few things here, and a priority note.
First — AI disclosure. Per [CONTRIBUTING](https://github.com/github/spec-kit/blob/main/CONTRIBUTING.md#ai-contributions-in-spec-kit), any AI-assisted contribution must disclose it: the agent/tool, the model(s), the settings/mode (autonomous vs. supervised), and the extent. There's no disclosure on this PR — please add it.
The narrowing is now correct — adding ValidationError to the except means malformed manifests still fall back safely. But three things block it: (1) CI is red on ruff F401 — the mock import in tests/test_presets.py is unused, remove it; (2) that test calls resolve_extension_command_via_manifest, whose ValidationError handling already existed, so it doesn't actually exercise the changed fallback in _reconcile_composed_commands — please target the code you changed; (3) the branch has conflicts with main and needs a rebase.
On priority: this is defensive exception-narrowing with no reported failure it fixes, so it's triage-can-wait — valid, revisited behind evidence-backed work. If the broad except Exception was masking a real failure, share it and I'll reprioritize.
(Drafted with AI assistance — GitHub Copilot.)
AI Assistance DisclosureI used AI assistance to narrow a broad |
mnriem
removed
the
author-needs-disclosure
|
Please resolve conflicts |
Problem
Bare
except Exceptionsilently swallows all errors in extension registration.Fix
Narrow to
(TypeError, ValueError, KeyError)which are the realistic failure modes.Testing