fix: use deepcopy instead of shallow copy when stripping orig_elements - #4402
Open
chuenchen309 wants to merge 2 commits into
Open
fix: use deepcopy instead of shallow copy when stripping orig_elements#4402chuenchen309 wants to merge 2 commits into
chuenchen309 wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
No issues found across 4 files
Shadow auto-approve: would auto-approve. Fixes a shallow-copy bug that mutated the caller's element metadata. The change is bounded to one line, matches the already-correct sibling implementation, and adds a regression test. No operational, security, or product tradeoffs.
Re-trigger cubic
_Chunker._orig_elements copies each element before clearing its metadata.orig_elements, with an explicit comment stating this is because "these elements don't belong to us (the user may have downstream purposes for them)." But copy.copy() is a shallow copy -- the copy's `.metadata` is the same object as the original's `.metadata` -- so `orig_element.metadata.orig_elements = None` mutates the caller's own element in place, exactly contradicting the stated intent. Sibling `_TableChunker._orig_elements` already does this correctly with `copy.deepcopy(self._table)`. Added a regression test confirming a caller's element still has its original `.metadata.orig_elements` value after `_Chunker._orig_elements` is computed. Confirmed it fails against the pre-fix shallow copy and passes with deepcopy. Ran the full test_unstructured/chunking/ suite (334 passed) plus ruff check/format (clean). Disclosure: I used an AI coding assistant (Claude) to help identify this bug and draft the fix. I independently reproduced the caller mutation with a throwaway script before writing the test, cross-checked against the already-correct sibling _TableChunker implementation, and ran the full local test suite before preparing this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the CHANGELOG entry and version bump required by CONTRIBUTING.md's PR checklist for the orig_elements shallow-copy fix in the previous commit.
chuenchen309
force-pushed
the
fix/chunking-orig-elements-shallow-copy
branch
from
July 15, 2026 22:05
c3d6478 to
ef6eed0
Compare
|
@/tmp/gh_comment_unstructured_4402.md |
Author
|
Heads up — your last comment came through as just a literal |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_Chunker._orig_elementscopies each element before clearing itsmetadata.orig_elements, with an explicit comment stating this is because "these elements don't belong to us (the user may have downstream purposes for them)." Butcopy.copy()is a shallow copy — the copy's.metadatais the same object as the original's.metadata— soorig_element.metadata.orig_elements = Nonemutates the caller's own element in place, exactly contradicting the stated intent.Sibling
_TableChunker._orig_elementsalready does this correctly withcopy.deepcopy(self._table).Fix
Use
copy.deepcopyinstead ofcopy.copy, matching the already-correct sibling implementation.Testing
.metadata.orig_elementsvalue after_Chunker._orig_elementsis computed.base.pyreproduces the caller mutation (assert None == [inner]); reapplying passes.test_unstructured/chunking/suite: 334 passed.ruff check— clean.include_orig_elementsmatches, a different parameter name).AI-Generated disclosure
I used an AI coding assistant (Claude) to help identify this bug and draft the fix. I independently reproduced the caller mutation with a throwaway script before writing the test, cross-checked against the already-correct sibling
_TableChunkerimplementation, and ran the full local test suite before preparing this change.Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com