[PR #3247] [CLOSED] fix(inventory): report duplicate ZIP entry paths instead of aborting inspection #5671

Closed
opened 2026-10-31 01:08:30 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3247
Author: @charan-rathore
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/3152-duplicate-zip-paths


📝 Commits (3)

  • 6843f9e fix(inventory): report duplicate ZIP entry paths instead of aborting inspection
  • 13fc5a5 Merge remote-tracking branch 'origin/main' into HEAD
  • be9f218 test(inventory): expect duplicate ZIP paths as separate occurrences

📊 Changes

8 files changed (+239 additions, -17 deletions)

View changed files

📝 src/artifacts/ArtifactPaths.test.ts (+29 -0)
📝 src/artifacts/ArtifactPaths.ts (+6 -1)
📝 src/artifacts/inventory/ArtifactGraphConstruction.ts (+13 -2)
📝 src/artifacts/inventory/ArtifactOccurrenceIdentity.test.ts (+119 -0)
📝 src/artifacts/inventory/scanReader.ts (+18 -3)
📝 src/domain/artifactIdentity.ts (+4 -0)
📝 src/domain/artifactInventoryEvidence.ts (+24 -7)
📝 tests/boundary/filesystem/artifactZipIntegrity.test.ts (+26 -4)

📄 Description

Summary

Fixes #3152. Inspecting an archive that lists the same entry path more than once aborted the whole scan with a path conflict. ZIP members can legally repeat a name, so one duplicate member hid the rest of the archive from inventory. The scan now reports every repeated same-kind path as its own occurrence and keeps going.

Problem and expected behavior

Smallest trigger: a ZIP whose central directory contains the same name twice, for example two different Chemin.txt members. ArtifactPathRegistry.add threw ArtifactReaderFailure(reason: "path") on the repeated terminal path, scanReader propagated it, and inspection produced nothing. A repeated name is data, not a structural conflict: inspection should report what the archive actually contains. Real conflicts are unchanged: the same path as file and directory, and path-prefix conflicts, still throw ArtifactReaderFailure(reason: "path").

Change and scope

  • ArtifactPathRegistry.add returns duplicate for an exact same-kind terminal repeat instead of throwing; kind mismatches and prefix conflicts keep throwing.
  • scanReader keeps digesting each entry. The first occurrence of a path stays the path owner for parent resolution; each later occurrence gets a Duplicate archive entry path; earlier occurrence of this path exists. limitation.
  • Occurrence identity: createOccurrence mixes a per-path duplicate ordinal into the scan-time id of later occurrences, so their ids no longer collide. rekeyOccurrences counts occurrences per logical path and passes the same ordinal into occurrenceIdForLocation, which accepts an optional duplicateOrdinal, so rekeyed ids stay distinct and stable. The Evidence validator applies the same per-path counting when it recomputes expected occurrence ids, keeping validated evidence consistent with scanned graphs.

Intentionally not included: no reconciliation or deduplication of duplicate members' content; each occurrence keeps its own digest, size, and limitations. This is an ordinary archive inspection fix inside the existing contribution scope.

Contract and boundary impact

  • Semantic owner and earliest changed stage: artifact inventory scan (path registration and occurrence identity).
  • Contribution scope fit and linked maintainer decision: not applicable; ordinary fix within existing archive inspection boundaries.
  • CLI and MCP/tool-catalog contract: unchanged; inventory_artifact now completes instead of failing on duplicate-path archives.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: occurrence ids for duplicate paths gain a duplicate-ordinal component. First-occurrence ids are unchanged, so inventories of archives without duplicates are bit-identical to before.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: ArtifactPaths.test.ts pins that an exact same-kind repeat returns duplicate while kind and prefix conflicts still throw. ArtifactOccurrenceIdentity.test.ts gains a duplicate archive entry paths describe that builds a stored ZIP with a repeated Chemin.txt member from raw bytes (zip.js ZipWriter refuses duplicate names) and asserts both occurrences appear, their ids are distinct and match the location-derived formula without and with the ordinal, the limitation lands only on the later occurrence, a normal member stays visible, and evidence creation over the duplicate inventory validates.
  • Base reproduction or other evidence: on the base revision with the source changes reverted, both new tests fail: the registry test because add throws, the archive test because inspection aborts on the repeated path.
  • User-visible CLI/MCP output (if applicable): not applicable.
  • Remaining proof gaps: none known.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run src/artifacts/ArtifactPaths.test.ts src/artifacts/inventory/ArtifactOccurrenceIdentity.test.ts - 2 files, 17 tests passed (17/17). With the source changes reverted on the base, both new tests fail (RED check).
  • npx vitest run src/artifacts src/domain - 183 files, 2993 tests passed (2993/2993).
  • npx oxlint on the changed files - 0 errors, 3 warnings, all pre-existing on the base (visitArtifactEntries and classifyArtifactBytes complexity/length).
  • npx oxfmt --check on the changed files - clean.
  • npx tsc --noEmit - attempted three times; the sandbox container was reclaimed before completion each time, so no local result. CI on this PR runs the full typecheck.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Archives without duplicate paths produce identical inventories, and duplicate-path archives previously failed outright, so no consumer could depend on their output.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; archive parsing path only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new target interaction; duplicate entries are still digested and validated individually.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3247 **Author:** [@charan-rathore](https://github.com/charan-rathore) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3152-duplicate-zip-paths` --- ### 📝 Commits (3) - [`6843f9e`](https://github.com/morluto/rea/commit/6843f9e64f0bb117f9ddb37b66dcb15b860e6378) fix(inventory): report duplicate ZIP entry paths instead of aborting inspection - [`13fc5a5`](https://github.com/morluto/rea/commit/13fc5a51e27b87aa41fe34036c11a9ae779a3d37) Merge remote-tracking branch 'origin/main' into HEAD - [`be9f218`](https://github.com/morluto/rea/commit/be9f218cc37524d9cb46760c72ae43db35c8c51c) test(inventory): expect duplicate ZIP paths as separate occurrences ### 📊 Changes **8 files changed** (+239 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/ArtifactPaths.test.ts` (+29 -0) 📝 `src/artifacts/ArtifactPaths.ts` (+6 -1) 📝 `src/artifacts/inventory/ArtifactGraphConstruction.ts` (+13 -2) 📝 `src/artifacts/inventory/ArtifactOccurrenceIdentity.test.ts` (+119 -0) 📝 `src/artifacts/inventory/scanReader.ts` (+18 -3) 📝 `src/domain/artifactIdentity.ts` (+4 -0) 📝 `src/domain/artifactInventoryEvidence.ts` (+24 -7) 📝 `tests/boundary/filesystem/artifactZipIntegrity.test.ts` (+26 -4) </details> ### 📄 Description ## Summary Fixes #3152. Inspecting an archive that lists the same entry path more than once aborted the whole scan with a path conflict. ZIP members can legally repeat a name, so one duplicate member hid the rest of the archive from inventory. The scan now reports every repeated same-kind path as its own occurrence and keeps going. ## Problem and expected behavior Smallest trigger: a ZIP whose central directory contains the same name twice, for example two different `Chemin.txt` members. `ArtifactPathRegistry.add` threw `ArtifactReaderFailure(reason: "path")` on the repeated terminal path, `scanReader` propagated it, and inspection produced nothing. A repeated name is data, not a structural conflict: inspection should report what the archive actually contains. Real conflicts are unchanged: the same path as file and directory, and path-prefix conflicts, still throw `ArtifactReaderFailure(reason: "path")`. ## Change and scope - `ArtifactPathRegistry.add` returns `duplicate` for an exact same-kind terminal repeat instead of throwing; kind mismatches and prefix conflicts keep throwing. - `scanReader` keeps digesting each entry. The first occurrence of a path stays the path owner for parent resolution; each later occurrence gets a `Duplicate archive entry path; earlier occurrence of this path exists.` limitation. - Occurrence identity: `createOccurrence` mixes a per-path duplicate ordinal into the scan-time id of later occurrences, so their ids no longer collide. `rekeyOccurrences` counts occurrences per logical path and passes the same ordinal into `occurrenceIdForLocation`, which accepts an optional `duplicateOrdinal`, so rekeyed ids stay distinct and stable. The Evidence validator applies the same per-path counting when it recomputes expected occurrence ids, keeping validated evidence consistent with scanned graphs. Intentionally not included: no reconciliation or deduplication of duplicate members' content; each occurrence keeps its own digest, size, and limitations. This is an ordinary archive inspection fix inside the existing contribution scope. ## Contract and boundary impact - Semantic owner and earliest changed stage: artifact inventory scan (path registration and occurrence identity). - Contribution scope fit and linked maintainer decision: not applicable; ordinary fix within existing archive inspection boundaries. - CLI and MCP/tool-catalog contract: unchanged; `inventory_artifact` now completes instead of failing on duplicate-path archives. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: occurrence ids for duplicate paths gain a duplicate-ordinal component. First-occurrence ids are unchanged, so inventories of archives without duplicates are bit-identical to before. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `ArtifactPaths.test.ts` pins that an exact same-kind repeat returns `duplicate` while kind and prefix conflicts still throw. `ArtifactOccurrenceIdentity.test.ts` gains a `duplicate archive entry paths` describe that builds a stored ZIP with a repeated `Chemin.txt` member from raw bytes (zip.js `ZipWriter` refuses duplicate names) and asserts both occurrences appear, their ids are distinct and match the location-derived formula without and with the ordinal, the limitation lands only on the later occurrence, a normal member stays visible, and evidence creation over the duplicate inventory validates. - Base reproduction or other evidence: on the base revision with the source changes reverted, both new tests fail: the registry test because `add` throws, the archive test because inspection aborts on the repeated path. - User-visible CLI/MCP output (if applicable): not applicable. - Remaining proof gaps: none known. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run src/artifacts/ArtifactPaths.test.ts src/artifacts/inventory/ArtifactOccurrenceIdentity.test.ts` - 2 files, 17 tests passed (17/17). With the source changes reverted on the base, both new tests fail (RED check). - `npx vitest run src/artifacts src/domain` - 183 files, 2993 tests passed (2993/2993). - `npx oxlint` on the changed files - 0 errors, 3 warnings, all pre-existing on the base (`visitArtifactEntries` and `classifyArtifactBytes` complexity/length). - `npx oxfmt --check` on the changed files - clean. - `npx tsc --noEmit` - attempted three times; the sandbox container was reclaimed before completion each time, so no local result. CI on this PR runs the full typecheck. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Archives without duplicate paths produce identical inventories, and duplicate-path archives previously failed outright, so no consumer could depend on their output. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; archive parsing path only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new target interaction; duplicate entries are still digested and validated individually. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:08:30 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#5671
No description provided.