Skip to content

fix handle non-numeric manifest icon keys in get_extension_icon to prevent ValueError crash #131 #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Th-Shivam wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix handle non-numeric manifest icon keys in get_extension_icon to prevent ValueError crash #131 #132

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
642ea5f
fix: handle non-numeric icon keys in get_extension_icon to prevent Va…
Copilot Apr 4, 2026
9f72592
fix: clarify isinstance guard comment per code review
Copilot Apr 4, 2026
7a79100
Merge pull request #1 from Th-Shivam/copilot/fix-bug-in-repo
Th-Shivam Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/extension_shield/api/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4035,8 +4035,13 @@ def _persisted_icon_response() -> Optional[Response]:
# Check icons object in manifest
manifest_icons = manifest.get("icons", {})
if manifest_icons:
# Get the largest icon
largest_size = max(manifest_icons.keys(), key=lambda x: int(x))
# Get the largest icon - only consider numeric keys (e.g. "128", "64")
# Some extensions use non-numeric keys like "48x48" which would cause ValueError
# isinstance guard handles edge cases where manifest was parsed outside of json.load
numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()]
Copy link
Copy Markdown
Collaborator

@sapnilbiswas sapnilbiswas Apr 4, 2026

Choose a reason for hiding this comment

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

The PR keeps only str keys where k.isdigit(). If a manifest has only non-numeric keys like "48x48", numeric_keys is empty and the code raises ValueError at main.py .

if not numeric_keys:
raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}")
largest_size = max(numeric_keys, key=lambda x: int(x))
Comment on lines +4038 to +4044
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

If an extension’s icons keys are only non-numeric strings (e.g. "48x48"), this block raises a ValueError and the icon selection from the manifest is skipped entirely. That prevents the crash but can still lead to a placeholder icon even when a valid icon path is present in the manifest. Consider extracting a numeric size from keys like "48x48" (e.g., split on "x" / regex for leading digits) so the endpoint can still serve the manifest-declared icon.

Copilot uses AI. Check for mistakes.
Comment on lines +4041 to +4044
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

Raising ValueError here is used for control flow and is immediately caught by the broad except Exception, which will log a warning ("Failed to read manifest for icons") even though the manifest was read successfully. Consider replacing the raise with a non-exceptional fallback (e.g., log at debug/info and continue) to avoid noisy warnings for manifests that simply don’t use numeric size keys.

Copilot uses AI. Check for mistakes.
icon_rel_path = manifest_icons[largest_size]
Comment on lines +4040 to 4045
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

numeric_keys currently filters to isinstance(k, str) and k.isdigit(), but the preceding comment says it should also handle cases where the manifest was parsed outside json.load. If manifest_icons contains integer keys (e.g., {128: "..."}), this code will treat them as non-numeric and skip valid icons. Consider accepting int keys as well (and normalizing to the actual key type used for indexing) so numeric sizes aren’t dropped in that edge case.

Suggested change
# isinstance guard handles edge cases where manifest was parsed outside of json.load
numeric_keys = [k for k in manifest_icons.keys() if isinstance(k, str) and k.isdigit()]
if not numeric_keys:
raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}")
largest_size = max(numeric_keys, key=lambda x: int(x))
icon_rel_path = manifest_icons[largest_size]
# Accept int keys as well for edge cases where manifest was parsed outside of json.load
numeric_icon_candidates = []
for k in manifest_icons.keys():
if isinstance(k, str) and k.isdigit():
numeric_icon_candidates.append((int(k), k))
elif isinstance(k, int) and not isinstance(k, bool):
numeric_icon_candidates.append((k, k))
if not numeric_icon_candidates:
raise ValueError(f"No numeric icon size keys found in manifest: {list(manifest_icons.keys())}")
_, largest_size_key = max(numeric_icon_candidates, key=lambda item: item[0])
icon_rel_path = manifest_icons[largest_size_key]

Copilot uses AI. Check for mistakes.
manifest_icon_path = os.path.join(extracted_path, icon_rel_path)

Expand Down
Loading