Skip to content

fix: narrow bare except Exception in version fallback - #3835

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

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

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

Summary

Replace overly broad except Exception with specific exception types in get_speckit_version() fallback.

Changes

  • _assets.py: Narrow outer exception to PackageNotFoundError
  • _assets.py: Narrow inner exception to (OSError, KeyError, ValueError)

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 exception handling in the Spec Kit version fallback.

Changes:

  • Handles missing package metadata explicitly.
  • Limits project-file errors to expected exception types.
Show a summary per file
File Description
src/specify_cli/_assets.py Narrows version lookup fallback exceptions.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread src/specify_cli/_assets.py Outdated
try:
return importlib.metadata.version("specify-cli")
except Exception:
except importlib.metadata.PackageNotFoundError:
Replace overly broad except Exception with specific exception types:
- importlib.metadata.PackageNotFoundError for missing package
- (OSError, KeyError, ValueError) for pyproject.toml read/parse errors

This prevents silently swallowing unexpected errors like AttributeError
or RecursionError from broken tomllib or malformed pyproject.toml.
@Quratulain-bilal
Quratulain-bilal force-pushed the fix/assets-narrow-exception branch from fdb5353 to 56ea7d4 Compare July 29, 2026 20:33
@mnriem
mnriem requested a balanced review from Copilot August 6, 2026 20:25

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

github-actions Bot commented Oct 6, 2026

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 6, 2026
…allback

importlib.metadata.version() can raise InvalidMetadataError for a
malformed installed distribution, which is not a PackageNotFoundError,
so the narrowed handler let it escape get_speckit_version() instead of
using the pyproject/unknown fallback. Build the guarded exception tuple
before the lookup, mirroring _version._get_installed_version().

The narrowed pyproject branch also called .get() on whatever the
project key held, so a non-mapping value turned the fallback into an
AttributeError; read it through isinstance checks instead.

Regression tests cover both paths (corrupt metadata and a non-mapping
project table) plus the pyproject fallback itself. Both new tests fail
on the previous implementation: one with the escaping
InvalidMetadataError, the other with AttributeError: 'int' object has
no attribute 'get'.

Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
@mnriem
mnriem requested a balanced review from Copilot October 7, 2026 15:16

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.

🟡 Changes recommended

The added tests pass against the previous implementation and do not verify the narrowing regression.

2 open findings

🧠 Review effort: Balanced

Comment on lines +25 to +29
def test_get_speckit_version_survives_invalid_metadata(monkeypatch):
"""A corrupt installed distribution must fall back, not raise.

``InvalidMetadataError`` is not a ``PackageNotFoundError``, so catching
only the latter lets it escape the version fallback (the same guard
@mnriem

mnriem commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

@github-actions github-actions Bot removed the stale label Oct 8, 2026

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants