fix(python,node): preserve specific exception type from partial extraction (#251) - #252
Conversation
…ction (#251) Since 0.4.0 the bindings collapsed SymlinkEscapeError, HardlinkEscapeError, and SecurityViolationError into a generic PartialExtractionError (Python) or a PARTIAL_EXTRACTION message prefix (Node) whenever a security error occurred after some files had already been written to disk. The core wraps such errors in ExtractionError::PartialExtraction; PR #216 (issue #210) reworked the error mapping to always surface the wrapper so it could expose the partial report, which dropped the inner error type. convert_error now unwraps PartialExtraction to the specific inner error and attaches the partial-extraction report onto that concrete error: files_extracted and bytes_written attributes on the Python exception, filesExtracted and bytesWritten appended to the Node message. Both the #251 type contract and the #210 report-inspection capability are satisfied at once. PartialExtractionError is removed from the Python public API: it is no longer raised, and making concrete types inherit from it would be wrong since those errors also occur non-partially. Callers detect partial output via the report attributes. Specs 006/007 and the .pyi are updated to match; regression tests cover symlink, hardlink, and security-violation sources for both bindings. BREAKING CHANGE: Python PartialExtractionError is removed. Code using "except PartialExtractionError" must catch the specific exception type or ExtractionError, and detect partial output via the files_extracted attribute.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 93.44% 93.46% +0.02%
==========================================
Files 58 58
Lines 13781 13827 +46
==========================================
+ Hits 12877 12923 +46
Misses 904 904
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…matting CI failed on `ruff format --check` for exarch.pyi after the type removal; reformat it. Add Python-level regression tests mirroring issue #251: a regular file written before a symlink/hardlink escape forces the core to wrap the error in PartialExtraction, and the binding must still raise the specific exception type (SymlinkEscapeError / HardlinkEscapeError) with the files_extracted and bytes_written report attributes attached. These execute in the pytest CI job, unlike the Rust unit tests which only type-check for the cdylib crates.
There was a problem hiding this comment.
Pull request overview
This PR restores the pre-0.4.0 behavior of preserving the specific security/quota exception type when exarch-core returns ExtractionError::PartialExtraction, while still exposing the partial-extraction report to callers (Python via exception attributes; Node via message suffix). It also removes the now-unused PartialExtractionError from the Python public API and updates specs/docs to match the corrected contract.
Changes:
- Python: unwrap
PartialExtractionto the inner concrete exception and attachfiles_extracted/bytes_writtenonto that exception; removePartialExtractionErrorfrom exports. - Node: make
PartialExtractionmessages start with the inner error code (e.g.SYMLINK_ESCAPE,QUOTA_EXCEEDED) and appendfilesExtracted/bytesWritten. - Update FR-075 / FR-083 specs, add/adjust regression tests, and document the Python breaking change in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| specs/007-node-bindings/spec.md | Updates FR-083 + tables/notes to reflect preserved inner error code plus appended report fields. |
| specs/006-python-bindings/spec.md | Updates FR-075 + docs/notes to reflect raising the inner exception type with report attributes; removes PartialExtractionError from the documented API. |
| crates/exarch-python/tests/test_cve_regression.py | Adds Python regression coverage for partial extraction preserving specific exception types. |
| crates/exarch-python/src/error.rs | Changes PartialExtraction mapping to return the inner exception type and attach report attributes; removes PartialExtractionError registration. |
| crates/exarch-python/exarch.pyi | Updates type stubs/docs to match the corrected exception mapping and partial-report attributes. |
| crates/exarch-node/src/error.rs | Changes PartialExtraction message formatting to preserve the inner error code prefix and append report fields; expands tests. |
| CHANGELOG.md | Documents the Python breaking change and the corrected Python/Node behaviors. |
Comments suppressed due to low confidence (1)
crates/exarch-python/tests/test_cve_regression.py:266
- This regression test asserts
files_extractedis present, but it doesn’t assertbytes_writtenis attached too. Adding abytes_writtenassertion here would better lock in the intended partial-extraction report contract and prevent future regressions where only one attribute is set.
err = exc_info.value
assert getattr(err, "files_extracted", None) is not None
assert err.files_extracted >= 1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #251. Since 0.4.0 the bindings collapsed
SymlinkEscapeError,HardlinkEscapeError, andSecurityViolationErrorinto a genericPartialExtractionError(Python) /PARTIAL_EXTRACTIONmessage prefix (Node) whenever a security error occurred after some files had already been written to disk.Root cause
exarch-corewraps such errors inExtractionError::PartialExtraction { source, report }oncereport.total_items() > 0. Prior to 0.4.0 the Python binding unwrapped this transparently (convert_error(*source)), surfacing the specific type. PR #216 (issue #210) reworked the mapping to always build the wrapper type so it could expose the partial report — which silently dropped the inner type. The.pyiand specs still advertised the specific types, confirming it was unintentional. Node inherited the same change.Fix
convert_errornow unwrapsPartialExtractionto the specific inner error and attaches the partial-extraction report onto that concrete error:files_extracted/bytes_writtenattributes set on the concrete exception.filesExtracted/bytesWrittenappended to the specific-coded message.Both the #251 type contract and the #210 report-inspection capability hold simultaneously.
PartialExtractionErroris removed from the Python public API: with the unwrap it is never raised, and making the concrete types inherit from it would be semantically wrong (those errors also occur non-partially, on the very first entry). Callers detect partial output via the report attributes.Spec conformance
A spec-conformance check found the fix contradicted FR-075 (006-python-bindings) and FR-083 (007-node-bindings), which had codified the buggy "always
PartialExtractionError" behavior under #210. Both specs, their acceptance tables, and the #210/#216 notes are updated so the documented contract matches the corrected behavior.Tests
Regression tests added for
SymlinkEscape,HardlinkEscape, andSecurityViolationsources wrapped inPartialExtraction, for both bindings — asserting the specific type/code and the report attributes. This closes the coverage gap that let #216 regress in the first place (the prior test only covered aQuotaExceededsource).Breaking change
Python
PartialExtractionErroris removed. Code usingexcept PartialExtractionErrormust catch the specific exception type (orExtractionErroras the catch-all) and detect partial output viagetattr(e, "files_extracted", None) is not None. Documented in CHANGELOG.Verification
cargo +nightly fmt --all -- --check— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo nextest run --workspace --all-features --exclude exarch-python --exclude exarch-node— 842 passed, 3 skippedcargo test --doc --workspace --all-features --exclude exarch-python --exclude exarch-node— 114 passedNote: the new binding unit tests execute in the dedicated
pytest/npm testCI jobs; the Rust job type-checks them (cdylib extension modules cannot link as standalone test binaries — pre-existing project pattern).Closes #251