Skip to content

fix(input): recognize ZIP extensions case-insensitively - #727

Merged
rng1995 merged 2 commits into
NVIDIA:mainfrom
YMuskrat:fix/zip-extension-case
Oct 5, 2026
Merged

rng1995 merged 2 commits into
NVIDIA:mainfrom
YMuskrat:fix/zip-extension-case

Conversation

@YMuskrat

@YMuskrat YMuskrat commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

A valid archive named skill.zip is extracted, but the same archive named skill.ZIP or skill.ZiP is treated as an ordinary file, leaving its SKILL.md unavailable for scanning. Downloads have the same issue when the server sends a generic content type.

This change makes the ZIP extension checks case-insensitive for local inputs, regular downloads, and downloads with a shared workflow resource budget. It normalizes the name only when checking the extension, preserving the original path and existing extraction safeguards.

Regression tests cover .zip, .ZIP, and .ZiP for local archives and both download paths. Download tests use a mocked HTTP response with application/octet-stream so extraction depends on the filename. Tests are added to the existing test_input_handler.py file.

Validation

  • Before the fix: six uppercase/mixed-case regression cases failed; three lowercase controls passed.
  • Focused input-handling tests: 119 passed, 2 skipped.
  • make lint, make format, and make format-check passed.
  • make test passed: 8,639 unit tests and 124 integration tests passed (22 skipped, 4 expected failures).

Closes #726

Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
@YMuskrat
YMuskrat marked this pull request as ready for review October 4, 2026 21:10

@rng1995 rng1995 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.

[SkillSpector Review]

Hi @YMuskrat, thank you for tracking down all three case-sensitive ZIP checks and fixing them together!

Value and readiness: .ZIP and .ZiP inputs now take the same _extract_zip() path as .zip for local files, direct file-URL downloads, and downloads under a shared workflow budget. That resolves #726. Only the extension comparison changed: the original path is untouched and every extraction safeguard still applies. The new tests cover all three entry points, and the uppercase and mixed-case cases fail on main. CI is green and the branch merges cleanly, so this is ready for final maintainer review.

Material findings

  1. [Non-blocking] src/skillspector/input_handler.py:816: the extension check runs before the is_dir() check. A directory named like skills.ZIP (passed without a trailing slash) now goes to _extract_zip() and fails with "Refusing to open a symlinked or non-regular file". On main it was scanned as a directory. main already fails this way for *.zip directories, so the PR only widens an existing edge case. To close it, take the ZIP branch only when normalized_local_path is not a directory, and add one test case.

PIC tradeoffs: None identified.

Verification and gaps:

  • Traced resolve() (input_handler.py:816), _download_file() (:1369) and _download_transitive_file() (:1392). All three now send .ZIP names to _extract_zip(). That function keeps the EOCD/ZIP64 preflight, the member, byte and central-directory caps, _safe_zip_target() containment (zip-slip), casefolded duplicate-path detection, exclusive xb writes, the ingest deadline and shared-budget truncation. Downloads are still written to the fixed name download.zip before extraction.
  • No other code keys on the .zip suffix. Nested archives already use Path.suffix.lower() plus byte-signature recognition (nested_artifacts.py:343), and source_type is only passed through in resolve_input.py.
  • Context for prioritization: main already byte-recognizes renamed local ZIPs downstream (the bundle.dat cases in tests/test_primary_input_completeness.py). Before this fix, a local .ZIP was therefore most likely inspected as a nested archive, not skipped. The gain is consistent first-class handling, not a new detection path. This is inferred from the tests; I did not run it.
  • Tests: the local test asserts source_type == "zip" and the extracted SKILL.md content. The download test serves application/octet-stream, so extraction depends only on the filename, and it covers both the direct and the budgeted handler.
  • input_handler.py and tests/unit/test_input_handler.py are unchanged on main since the merge base 4a550627, and git merge-tree with main is clean. All 6 CI checks passed on this head. No other open PR addresses #726. #736 also edits input_handler.py and merges cleanly with this PR. #579's conflicts are in files this PR does not touch.
  • Tests were not executed locally, per review policy.

Decision: Approved (reviewed head c589c3241fdde50f1b7490bdcd21c70e18c5cbd3)

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.

ZIP inputs with uppercase extensions are not extracted

2 participants