morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #3152] [Feature] Report duplicate ZIP entry paths as inventory facts instead of aborting inspect_artifact #1100

New issue

Closed

opened 2026-10-15 18:31:03 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-15 18:31:03 +00:00

Owner

Originally created by @molako2 on GitHub (Oct 14, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/3152

Before submitting

I searched the open and closed issues and pull requests for a related proposal.
I understand the contribution scope must be reviewed before integration; open questions are welcome here.
This request does not include credentials, private samples, or sensitive target data.

Area

Evidence, artifacts, or reconstruction

Current limitation

One repeated name in a ZIP central directory hides the whole archive from REA. On 2026-10-14, a ZIP extracted from a commercial installer had Chemin.txt twice in its central directory. inspect_artifact failed, and none of the archive's other members were reported.

Reproduction (synthetic, no vendor file)

Environment: Windows 11, Node.js 22.20.0, rea-agents 6.4.0 MCP server. The source was read at commit 583008852.

make_dup_zip.py (under Verification details) builds a stored ZIP with three central-directory records, Chemin.txt (6 bytes), Autre.txt and Chemin.txt (7 bytes, different content). Python's zipfile only warns about the repeated name.

open_binary   { "path": "<abs>/duplicate-path.zip" }  → ok, format "zip", kind "archive"
inspect_artifact {}                                      → artifact_operation_failed
inspect_artifact { "integrity_policy": "record-and-continue" } → artifact_operation_failed
extract_artifact {}                                      → artifact_operation_failed
  details: { "reason": "path", "detail": "Artifact path collision: Chemin.txt" }

Autre.txt is never reported, and neither Chemin.txt occurrence is hashed.

Owning code

The reader is not the cause. ZipArtifactReader keeps physical member identity: adapterKey: zip-entry:<index> at src/artifacts/ZipArtifactReader.ts:271. Its comment at lines 188–189 says that "Logical path uniqueness belongs to the artifact inventory boundary." #1860 and commit bcd350c36 made that choice.
The inventory boundary throws. ArtifactPathRegistry.add (src/artifacts/ArtifactPaths.ts:45-78) throws ArtifactReaderFailure("path", "Artifact path collision: …") at line 64. Its callers are:
- inventory: src/artifacts/inventory/scanReader.ts:175
- extraction: src/artifacts/extraction/ArtifactExtraction.ts:297-300
- JavaScript application files: src/artifacts/javascript/JavaScriptArtifactFiles.ts:208
Identity is derived from the path. This is why removing the throw alone is not enough. createOccurrence builds occurrence_id from { path, kind } (src/artifacts/inventory/ArtifactGraphConstruction.ts:86). rekeyOccurrences then rebuilds it from occurrenceIdForLocation({ rootArtifactId, logicalPath, entryKind }) (:247-269, src/domain/artifactIdentity.ts:37). Two members with the same name would share one occurrence_id.
Consumers keyed by logical_path. Each of these would drop or misattribute a duplicate:
- scanReader.ts:214 occurrenceByPath.set
- ArtifactGraphConstruction.ts:57-61 indexOccurrencesByPath: keeps the first occurrence and silently skips later ones
- scanCanonical.ts:234,301: integrity contradictions are looked up by path
- ArtifactExtraction.ts:218
- JavaScriptArtifactFiles.ts:407-437
- src/domain/artifactComparison.ts:202-205: compare_artifacts pairs occurrences by path
- src/domain/artifactInventoryEvidence.ts:185: mergeExact on the logical path
The behaviour is tested. tests/boundary/filesystem/artifactZipIntegrity.test.ts:264-319 expects inventory_artifact to fail with reason: "path". #1725 stated that "exact duplicate names should be rejected". This proposal asks maintainers to revisit that decision for inventory only.

Use cases

No response

Desired outcome and success criteria

Inventory reports what the archive declares. Extraction keeps refusing to choose between the copies.

Inventory completes. Each central-directory record remains one occurrence with its own hash, sizes and flags. Coverage stays complete when every record was examined.
Distinct identity. Occurrences that share a logical path get distinct occurrence_id values. I propose that the disambiguator (the 1-based duplicate ordinal in central-directory order) enters the identity only when a collision exists. Then occurrence_id, graph_sha256 and manifest_id stay byte-identical for every archive without collisions, and retained Evidence and comparisons remain valid. Maintainers decide the exact encoding.
An explicit fact. Add a new top-level array next to integrity_contradictions, for example path_collisions, with one record per logical path. Fields:
- logical_path
- kind: exact, or prefix for a file named a next to a/b
- each member's occurrence_id, central-directory ordinal, sha256, declared_size and compressed_size
- bytes_identical: true or false
- trust: "observed" and provenance: "container-directory"
I avoid reusing integrity_contradictions, because its schema means "declared hash contradicts observed bytes". This is a different relation.
Extraction stays fail-closed. extract_artifact keeps returning reason: "path". Its message names the colliding ordinals and points to the inventory fact. Extractors resolve duplicates differently (overwrite, rename or prompt), so REA must not pick one silently. Letting the caller choose one occurrence is out of scope.
Consumers are explicit. Every consumer in the list under "Owning code" either handles all occurrences of a path or reports the ambiguity. compare_artifacts must not pair a duplicate silently.

The case-folding collisions recorded by annotateEnglishUnicodeCaseCollisions stay as they are. Exact collisions are a stronger fact and get their own record.

Expected interaction with targets

Read existing files or captured data

Target and scope details (optional)

Authority and side effects

Read-only. No new write, mount or execution path. Extraction authority does not change.
Main risk: a downstream Map keyed by logical_path that silently keeps one occurrence (indexOccurrencesByPath does this). The consumer audit is part of the change, not follow-up work.
Evidence compatibility: inventories without collisions keep their identities, so retained Evidence and comparisons are unaffected. The artifactInventoryResultSchema gains a field, which needs a default for older Evidence, like integrity_contradictions.

Open scope questions

Should prefix conflicts (Artifact prefix conflict, ArtifactPaths.ts:52-56 and :69-73) get the same treatment?
Should the same rule apply to ASAR and directory readers, or only to ZIP-family formats (zip, apk, ipa, msix, appx, hap, hsp, app-pack)?
Where does the disambiguator live: in occurrenceIdForLocation, or in a separate physical-ordinal field?

Verification available so far

I have a prototype or observed results

Verification details (optional)

Update the pinned test. Change artifactZipIntegrity.test.ts:264-319 so that inventory succeeds with two occurrences, two distinct IDs and one path_collisions record. Each occurrence's sha256 must match its own bytes. Extraction still fails with reason: "path".
Fixtures. Reuse that test's byte-patching technique (rename y.txt to x.txt in the local and central headers), so no Python is needed in tests. make_dup_zip.py is an equivalent generator.
Cases.
- identical bytes and different bytes
- three copies of one name
- a repeated dir/ entry
- a file and a directory with the same name (prefix)
- a duplicate inside a nested ASAR
- a duplicate plus a case-fold collision
- an encrypted duplicate
- a duplicate plus an integrity mismatch under record-and-continue
Golden. For archives without collisions, manifest_id and graph_sha256 must be unchanged before and after the change.
Workflows. CLI and MCP inspect_artifact, extract_artifact, and compare_artifacts between a clean archive and a duplicated one.
Unverified. How other extractors resolve duplicates. This proposal does not depend on it.

Fixture generators (synthetic, no vendor file)

make_dup_zip.py

import sys, zipfile, warnings
out = sys.argv[1]
warnings.simplefilter("ignore")  # zipfile warns "Duplicate name" but writes both records
with zipfile.ZipFile(out, "w", zipfile.ZIP_STORED) as z:
    z.writestr("Chemin.txt", "first\n")
    z.writestr("Autre.txt", "unrelated\n")
    z.writestr("Chemin.txt", "second\n")
with zipfile.ZipFile(out) as z:
    print([(i.filename, i.header_offset, i.file_size) for i in z.infolist()])

Alternatives considered

Keep failing (the behaviour at 583008852). This loses every unrelated member and gives the analyst no partial facts. It conflicts with the AGENTS.md rule to "preserve partial facts and meaningful failure reasons".
Last-wins or first-wins deduplication. This hides a contradiction and invents a choice the archive does not make.
Treating duplicates as a limitation string only, like case collisions. Too weak: the occurrences need distinct identities, and the bytes may differ.

Possible direction

The plan under Desired outcome and success criteria is a proposal, not a requirement. No implementation is intended before a maintainer scope decision.

References

#1725, #1860 and commit bcd350c36: physical ZIP member identity; duplicate rejection kept at inventory
tests/boundary/filesystem/artifactZipIntegrity.test.ts#L264-L319: the test that pins the current behaviour

Originally created by @molako2 on GitHub (Oct 14, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/3152 ### Before submitting - [X] I searched the open and closed issues and pull requests for a related proposal. - [X] I understand the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope) must be reviewed before integration; open questions are welcome here. - [X] This request does not include credentials, private samples, or sensitive target data. ### Area Evidence, artifacts, or reconstruction ### Current limitation One repeated name in a ZIP central directory hides the whole archive from REA. On 2026-10-14, a ZIP extracted from a commercial installer had `Chemin.txt` twice in its central directory. `inspect_artifact` failed, and none of the archive's other members were reported. #### Reproduction (synthetic, no vendor file) Environment: Windows 11, Node.js 22.20.0, `rea-agents` 6.4.0 MCP server. The source was read at commit 583008852. `make_dup_zip.py` (under *Verification details*) builds a stored ZIP with three central-directory records, `Chemin.txt` (6 bytes), `Autre.txt` and `Chemin.txt` (7 bytes, different content). Python's `zipfile` only warns about the repeated name. ```text open_binary { "path": "<abs>/duplicate-path.zip" } → ok, format "zip", kind "archive" inspect_artifact {} → artifact_operation_failed inspect_artifact { "integrity_policy": "record-and-continue" } → artifact_operation_failed extract_artifact {} → artifact_operation_failed details: { "reason": "path", "detail": "Artifact path collision: Chemin.txt" } ``` `Autre.txt` is never reported, and neither `Chemin.txt` occurrence is hashed. #### Owning code - **The reader is not the cause.** `ZipArtifactReader` keeps physical member identity: `adapterKey: zip-entry:<index>` at [`src/artifacts/ZipArtifactReader.ts:271`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/ZipArtifactReader.ts#L271). Its comment at lines 188–189 says that "Logical path uniqueness belongs to the artifact inventory boundary." #1860 and commit bcd350c36 made that choice. - **The inventory boundary throws.** `ArtifactPathRegistry.add` ([`src/artifacts/ArtifactPaths.ts:45-78`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/ArtifactPaths.ts#L45-L78)) throws `ArtifactReaderFailure("path", "Artifact path collision: …")` at line 64. Its callers are: - inventory: [`src/artifacts/inventory/scanReader.ts:175`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/inventory/scanReader.ts#L175) - extraction: [`src/artifacts/extraction/ArtifactExtraction.ts:297-300`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/extraction/ArtifactExtraction.ts#L297-L300) - JavaScript application files: [`src/artifacts/javascript/JavaScriptArtifactFiles.ts:208`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/javascript/JavaScriptArtifactFiles.ts#L208) - **Identity is derived from the path.** This is why removing the `throw` alone is not enough. `createOccurrence` builds `occurrence_id` from `{ path, kind }` ([`src/artifacts/inventory/ArtifactGraphConstruction.ts:86`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/artifacts/inventory/ArtifactGraphConstruction.ts#L86)). `rekeyOccurrences` then rebuilds it from `occurrenceIdForLocation({ rootArtifactId, logicalPath, entryKind })` (`:247-269`, [`src/domain/artifactIdentity.ts:37`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/domain/artifactIdentity.ts#L37)). Two members with the same name would share one `occurrence_id`. - **Consumers keyed by `logical_path`.** Each of these would drop or misattribute a duplicate: - `scanReader.ts:214` `occurrenceByPath.set` - `ArtifactGraphConstruction.ts:57-61` `indexOccurrencesByPath`: keeps the first occurrence and silently skips later ones - `scanCanonical.ts:234,301`: integrity contradictions are looked up by path - `ArtifactExtraction.ts:218` - `JavaScriptArtifactFiles.ts:407-437` - [`src/domain/artifactComparison.ts:202-205`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/domain/artifactComparison.ts#L202-L205): `compare_artifacts` pairs occurrences by path - [`src/domain/artifactInventoryEvidence.ts:185`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/src/domain/artifactInventoryEvidence.ts#L185): `mergeExact` on the logical path - **The behaviour is tested.** [`tests/boundary/filesystem/artifactZipIntegrity.test.ts:264-319`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/tests/boundary/filesystem/artifactZipIntegrity.test.ts#L264-L319) expects `inventory_artifact` to fail with `reason: "path"`. #1725 stated that "exact duplicate names should be rejected". This proposal asks maintainers to revisit that decision for **inventory only**. ### Use cases _No response_ ### Desired outcome and success criteria Inventory reports what the archive declares. Extraction keeps refusing to choose between the copies. 1. **Inventory completes.** Each central-directory record remains one occurrence with its own hash, sizes and flags. Coverage stays `complete` when every record was examined. 2. **Distinct identity.** Occurrences that share a logical path get distinct `occurrence_id` values. I propose that the disambiguator (the 1-based duplicate ordinal in central-directory order) enters the identity only when a collision exists. Then `occurrence_id`, `graph_sha256` and `manifest_id` stay byte-identical for every archive without collisions, and retained Evidence and comparisons remain valid. Maintainers decide the exact encoding. 3. **An explicit fact.** Add a new top-level array next to `integrity_contradictions`, for example `path_collisions`, with one record per logical path. Fields: - `logical_path` - `kind`: `exact`, or `prefix` for a file named `a` next to `a/b` - each member's `occurrence_id`, central-directory ordinal, `sha256`, `declared_size` and `compressed_size` - `bytes_identical`: true or false - `trust: "observed"` and `provenance: "container-directory"` I avoid reusing `integrity_contradictions`, because its schema means "declared hash contradicts observed bytes". This is a different relation. 4. **Extraction stays fail-closed.** `extract_artifact` keeps returning `reason: "path"`. Its message names the colliding ordinals and points to the inventory fact. Extractors resolve duplicates differently (overwrite, rename or prompt), so REA must not pick one silently. Letting the caller choose one occurrence is out of scope. 5. **Consumers are explicit.** Every consumer in the list under "Owning code" either handles all occurrences of a path or reports the ambiguity. `compare_artifacts` must not pair a duplicate silently. The case-folding collisions recorded by `annotateEnglishUnicodeCaseCollisions` stay as they are. Exact collisions are a stronger fact and get their own record. ### Expected interaction with targets Read existing files or captured data ### Target and scope details (optional) #### Authority and side effects - Read-only. No new write, mount or execution path. Extraction authority does not change. - Main risk: a downstream `Map` keyed by `logical_path` that silently keeps one occurrence (`indexOccurrencesByPath` does this). The consumer audit is part of the change, not follow-up work. - Evidence compatibility: inventories without collisions keep their identities, so retained Evidence and comparisons are unaffected. The `artifactInventoryResultSchema` gains a field, which needs a default for older Evidence, like `integrity_contradictions`. #### Open scope questions 1. Should prefix conflicts (`Artifact prefix conflict`, `ArtifactPaths.ts:52-56` and `:69-73`) get the same treatment? 2. Should the same rule apply to ASAR and directory readers, or only to ZIP-family formats (`zip`, `apk`, `ipa`, `msix`, `appx`, `hap`, `hsp`, `app-pack`)? 3. Where does the disambiguator live: in `occurrenceIdForLocation`, or in a separate physical-ordinal field? ### Verification available so far I have a prototype or observed results ### Verification details (optional) - **Update the pinned test.** Change `artifactZipIntegrity.test.ts:264-319` so that inventory succeeds with two occurrences, two distinct IDs and one `path_collisions` record. Each occurrence's `sha256` must match its own bytes. Extraction still fails with `reason: "path"`. - **Fixtures.** Reuse that test's byte-patching technique (rename `y.txt` to `x.txt` in the local and central headers), so no Python is needed in tests. `make_dup_zip.py` is an equivalent generator. - **Cases.** - identical bytes and different bytes - three copies of one name - a repeated `dir/` entry - a file and a directory with the same name (prefix) - a duplicate inside a nested ASAR - a duplicate plus a case-fold collision - an encrypted duplicate - a duplicate plus an integrity mismatch under `record-and-continue` - **Golden.** For archives without collisions, `manifest_id` and `graph_sha256` must be unchanged before and after the change. - **Workflows.** CLI and MCP `inspect_artifact`, `extract_artifact`, and `compare_artifacts` between a clean archive and a duplicated one. - **Unverified.** How other extractors resolve duplicates. This proposal does not depend on it. #### Fixture generators (synthetic, no vendor file) <details><summary><code>make_dup_zip.py</code></summary> ```python import sys, zipfile, warnings out = sys.argv[1] warnings.simplefilter("ignore") # zipfile warns "Duplicate name" but writes both records with zipfile.ZipFile(out, "w", zipfile.ZIP_STORED) as z: z.writestr("Chemin.txt", "first\n") z.writestr("Autre.txt", "unrelated\n") z.writestr("Chemin.txt", "second\n") with zipfile.ZipFile(out) as z: print([(i.filename, i.header_offset, i.file_size) for i in z.infolist()]) ``` </details> ### Alternatives considered - **Keep failing** (the behaviour at 583008852). This loses every unrelated member and gives the analyst no partial facts. It conflicts with the `AGENTS.md` rule to "preserve partial facts and meaningful failure reasons". - **Last-wins or first-wins deduplication.** This hides a contradiction and invents a choice the archive does not make. - **Treating duplicates as a `limitation` string only**, like case collisions. Too weak: the occurrences need distinct identities, and the bytes may differ. ### Possible direction The plan under *Desired outcome and success criteria* is a proposal, not a requirement. No implementation is intended before a maintainer scope decision. ### References - #1725, #1860 and commit bcd350c36: physical ZIP member identity; duplicate rejection kept at inventory - [`tests/boundary/filesystem/artifactZipIntegrity.test.ts#L264-L319`](https://github.com/morluto/rea/blob/58300885207133422af235e0f2b10658cf958b03/tests/boundary/filesystem/artifactZipIntegrity.test.ts#L264-L319): the test that pins the current behaviour

bsakaguchi

2026-10-15 18:31:03 +00:00

closed this issue
added the
enhancement
label

bsakaguchi commented

2026-10-15 18:31:04 +00:00

Author

Owner

@charan-rathore commented on GitHub (Oct 14, 2026):

Taking this one. The registry treating a repeated same-kind entry name as a structural conflict is what aborts the scan; I have it reporting each repeat as its own occurrence with the path/type conflict errors intact. PR coming up shortly.

@charan-rathore commented on GitHub (Oct 14, 2026): Taking this one. The registry treating a repeated same-kind entry name as a structural conflict is what aborts the scan; I have it reporting each repeat as its own occurrence with the path/type conflict errors intact. PR coming up shortly.

bsakaguchi referenced this issue

2026-10-16 21:09:19 +00:00

[GH-ISSUE #4307] [Feature] Inspect the dyld shared cache and resolve system library loads #1711

bsakaguchi referenced this issue

2026-10-17 21:08:36 +00:00

[PR #1100] [MERGED] fix(apple): validate dylib candidates and preserve search uncertainty #2648

bsakaguchi referenced this issue

2026-10-17 21:08:43 +00:00

[PR #1110] [MERGED] fix(apple): classify empty dyld settings by consumer semantics #2658

bsakaguchi added

enhancement

and removed