Repository navigation
fix(input): recognize ZIP extensions case-insensitively - #727
Conversation
Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
rng1995
left a comment
There was a problem hiding this comment.
[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
- [Non-blocking]
src/skillspector/input_handler.py:816: the extension check runs before theis_dir()check. A directory named likeskills.ZIP(passed without a trailing slash) now goes to_extract_zip()and fails with "Refusing to open a symlinked or non-regular file". Onmainit was scanned as a directory.mainalready fails this way for*.zipdirectories, so the PR only widens an existing edge case. To close it, take the ZIP branch only whennormalized_local_pathis 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.ZIPnames 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, exclusivexbwrites, the ingest deadline and shared-budget truncation. Downloads are still written to the fixed namedownload.zipbefore extraction. - No other code keys on the
.zipsuffix. Nested archives already usePath.suffix.lower()plus byte-signature recognition (nested_artifacts.py:343), andsource_typeis only passed through inresolve_input.py. - Context for prioritization:
mainalready byte-recognizes renamed local ZIPs downstream (thebundle.datcases intests/test_primary_input_completeness.py). Before this fix, a local.ZIPwas 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 extractedSKILL.mdcontent. The download test servesapplication/octet-stream, so extraction depends only on the filename, and it covers both the direct and the budgeted handler. input_handler.pyandtests/unit/test_input_handler.pyare unchanged onmainsince the merge base4a550627, andgit merge-treewithmainis clean. All 6 CI checks passed on this head. No other open PR addresses #726. #736 also editsinput_handler.pyand 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)
A valid archive named
skill.zipis extracted, but the same archive namedskill.ZIPorskill.ZiPis treated as an ordinary file, leaving itsSKILL.mdunavailable 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.ZiPfor local archives and both download paths. Download tests use a mocked HTTP response withapplication/octet-streamso extraction depends on the filename. Tests are added to the existingtest_input_handler.pyfile.Validation
make lint,make format, andmake format-checkpassed.make testpassed: 8,639 unit tests and 124 integration tests passed (22 skipped, 4 expected failures).Closes #726