Skip to content

fix: narrow exceptions in get_speckit_version() from Exception to specific types - #3917

Open
Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/assets-narrow-exception-v2
Open

Quratulain-bilal wants to merge 2 commits into
github:mainfrom
Quratulain-bilal:fix/assets-narrow-exception-v2

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Problem

Outer catch narrowed to PackageNotFoundError. Inner catch narrowed to (OSError, ValueError, KeyError) for file/parse errors.

Fix

Narrowed both exception handlers to specific types.

Testing

  • Verified version is returned correctly
  • Verified fallback to pyproject.toml works

…cific types

Outer catch narrowed to PackageNotFoundError (the only expected
failure from importlib.metadata.version). Inner catch narrowed to
(OSError, ValueError, KeyError) for file/parse errors.

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

Narrows version-resolution exception handling to expected metadata, file, and TOML parsing failures.

Changes:

  • Handles missing package metadata explicitly.
  • Limits pyproject fallback errors to specific exception types.
Show a summary per file
File Description
src/specify_cli/_assets.py Narrows exceptions in get_speckit_version().

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

Comment thread src/specify_cli/_assets.py
Comment thread src/specify_cli/_assets.py Outdated

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please address Copilot feedback

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has had no activity for 60 days. It will be closed in 30 days unless there is a substantive update. If you intend to continue, please describe the current status, address or acknowledge outstanding feedback, and confirm whether the branch can be updated and the change remains ready for review. A comment that only states that the pull request is "still relevant" does not provide enough context for maintainers.

@github-actions github-actions Bot added the stale label Oct 11, 2026
Addresses 2 Copilot findings:

1. Guard InvalidMetadataError (Medium): importlib.metadata.version()
   can raise InvalidMetadataError on malformed metadata. Mirrors the
   guard in _version._get_installed_version() — the exception class
   is looked up dynamically for Python versions where it is absent.

2. Add regression tests (Low): tests that FAIL against the previous
   bare except Exception implementation:

   - test_get_speckit_version_survives_invalid_metadata:
     InvalidMetadataError falls back correctly
   - test_get_speckit_version_reads_pyproject_fallback:
     PackageNotFoundError triggers pyproject.toml fallback
   - test_unrelated_exception_from_version_lookup_propagates:
     TypeError now propagates (was swallowed)
   - test_unrelated_exception_from_pyproject_propagates:
     RuntimeError now propagates (was swallowed)
   - test_pyproject_io_error_returns_unknown:
     OSError still caught (intended path preserved)

Assisted-by: GitHub Copilot (autonomous)

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants