Repository navigation
fix(merge): reserve the first input's unresolved item ids from later streams (#463) - #500
entelecheia wants to merge 1 commit into
Conversation
…streams (#463) A primary picture whose `ItemRef` names an id the primary does not carry (no manifest entry, no stream) was silently bound to a later input's payload: the graft's namespace check only asked whether an existing name contains the candidate, while `Document::bin_stream_index` resolves a reference towards the streams by exact name, `/name` suffix or filename stem. Merging A (an unresolved `logo`) with B (`BinData/logo.png`) therefore made A's picture render B's bytes and suppressed the `BinaryAssetRemoved` evidence both writers would otherwise report. Collect the primary's unresolved `ItemRef` spellings before grafting and treat a candidate stream name as taken when it would answer one of them. The colliding stream is renamed on its file component (which also leaves the stem rule nothing to match), and a native payload is reminted by storage id instead. The primary's reference stays unresolved; nothing is rewritten or refused.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b29a88bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| a.sections[0].paragraphs.insert( | ||
| 0, | ||
| hwp_model::Paragraph { | ||
| controls: vec![Control::Picture(sample_picture("logo"))], | ||
| ..Default::default() |
There was a problem hiding this comment.
The new regression tests place the primary's unresolved picture only in a section-level paragraph, although unresolved_primary_spellings must also reserve references inside table cells, picture/table/shape captions, and generic paragraph lists. These tests would still pass if the new collector stopped visiting those nested paths. The existing shape-caption test exercises a valid reference in a later input, so it does not verify this primary-reference reservation behavior. Add nested primary cases that assert the reference remains unresolved and the later input retains its own payload. This is required by the regression-test guidance in AGENTS.md, which explicitly requires visiting every affected path.
AGENTS.md reference: AGENTS.md:L171-L175
Useful? React with 👍 / 👎.
Summary
mergenow reserves the first input's unresolved picture and shape-captionItemRefspellingsbefore it grafts any later input, so a later input's stream cannot take a name that would answer
one of them.
Closes #463.
The bug
Document::bin_stream_indexresolves an unlistedItemReftowards the streams: exact name,/{name}suffix, or filename stem. The graft's namespace check(
bin_namespace_name_taken) tested the opposite direction — whether an existing (or alreadycarried) name contains the candidate — so a later input's stream whose name answered an
unresolved primary spelling was appended unchanged.
A/B example:
ItemRef("logo"); no manifest entry, no stream, so A alone rendersnothing and reports
BinaryAssetRemoved.BinData/logo.png.Before: after
merge a b, A's picture resolved through the stem rule to B's payload, so Arendered B's image and the merged output reported no missing asset. After: B's stream is
renamed and A's reference stays unresolved, so both writers ledger it (
BinaryAssetRemovedinthe HWPX writer and in the merge loss report;
PictureControlRemovedin the HWP5 report).What changed
crates/hwp-convert/src/document_merge.rs:unresolved_primary_spellings(doc)collects every modeledItemRefspelling (body paragraphs,table cells, captions, generic paragraph lists) that the primary itself cannot resolve, once,
before grafting.
name_answers_spelling(name, spelling)mirrors the resolver's three unlisted-id rules;bin_namespace_name_takentreats a candidate as taken when it matches, in addition to theexisting containment check.
graft_bin_streamsfor every later input, and throughgraft_bin_data/bin_stream_name_taken, because a native payload's canonicalBIN####nameis a later stream name too: that path remints the storage id instead of the name.
renamed_bin_name). A prefix on thewhole
dir/filename leaves the stem unchanged, so the resolver's stem rule would match therename candidate again and the loop could not terminate.
Nothing is rewritten or refused: the primary's reference stays exactly as it was and the
writers continue to report it.
Document::bin_stream_indexis untouched.Tests
Red first, on
7719de2(currentmain), before the fix:document_merge::tests::병합_뒤_첫_입력의_미해결_id가_뒤_입력의_스트림으로_해석되지_않는다failed at its intended assertion,
left: Some([66, 45, 73, 77, 65, 71, 69])(B'sB-IMAGEbytes) vs
right: None, on both graft tiers.merge_missing_manifest's new cases failed atassert_binary_loss, with a report that heldonly metadata/package events and no
BinaryAssetRemoved, for both.hwpand.hwpxoutput.병합_뒤_원본_bin_이름도_첫_입력의_미해결_id를_대신하지_않는다failed at "the native payload mustbe reminted away from the reserved canonical name" when the remint reservation was dropped
from
graft_bin_dataalone.Added:
crates/hwp-convert/src/document_merge.rs: two unit tests (unlisted spelling through thestem rule on both tiers; canonical
BIN####remint) with fixture sanity checks that theprimary does not carry what it names and the later input does.
crates/hwp-cli/tests/merge_missing_manifest.rs:hwp_merge_unlisted_primary_id_reports_binary_asset_removedand
hwpx_merge_unlisted_primary_id_reports_binary_asset_removedbuild a real package whosepicture names an id with no manifest item and no payload, then assert the published output
reports
BinaryAssetRemoved, the primary's picture never resolves to the later input's bytes,and the later input's picture keeps its own bytes under its renamed stream.
Existing merge tests (
병합_뒤_입력의_그림은_자기_문서의_매니페스트_id로_자기_바이트를_가리킨다,병합_뒤_충돌로_이름이_바뀐_스트림도_id로_해석된다, the #417 publication cases) pass unchanged.Gate