fix(artifacts): prevent disk paths from colliding with archive members

[yashrajp22]

A real file below a directory ending in ! could share a cache key with an archive member. Merging the archive could replace the disk file's bytes and hide its findings. Detect actual disk/member collisions, retain the disk bytes, and record a fatal attribution failure. Non-colliding !/ paths keep their normal behavior.

Withheld members and their existing exceptions are attributed to the archive container, with original reason and limit values preserved. Real disk containers expand first so nested virtual paths cannot displace their children. Later overrides cannot downgrade a collision from failed to partial.

Validation: 744 tests passed against source and a freshly installed wheel, including reversed/depth-two collisions, exclusions, ordinary delimiter paths, and Python source classification. Three focused synthetic skills also matched across source and wheel. Withheld archive metadata cannot overwrite a real file's Python classification. Verification was offline.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for finding a real collision between disk paths and archive virtual paths and fixing it at the one place where nested results are merged into the caches! Keeping the bytes read from disk and making the collision fail closed means the layout you reported can no longer end complete/SAFE with exit 0.

Value and readiness: The problem is real on main. With main's code (graph.invoke, use_llm=False, and the CLI), a disk file bundle.zip!/payload.txt holding an injection payload next to a bundle.zip whose member payload.txt is benign gives two analyzed inventory rows for the same path, raw_file_cache holding the benign archive bytes, no P1, a complete SAFE result and CLI exit 0. The hidden .bundle.zip variant behaves the same way. With the PR's two source hunks transcribed onto main, that layout gives one failed/artifact_path_collision row, keeps the disk bytes, reports P1 and YR4, and ends with status failed, execution_successful=false and exit 2. An ordinary ZIP with no collision is unchanged. No other route to a collision exists, so every collision now fails closed. The change is not ready for final maintainer review yet because of finding 1: the reservation applies to every real path containing !/, not just to colliding ones. A benign skill with any directory whose name ends in ! now fails with exit 2, an executable in such a directory gets a false SC9 HIGH and DO_NOT_INSTALL, --exclude cannot avoid it, and a new test locks this in. The remaining findings are non-blocking. The most important of them is that the reverse layout (benign disk decoy, malicious archive member) now drops the member's findings without recording that the member was withheld. Once the failure is limited to real collisions, the change should be close to ready.

Material findings

1. [Blocker] src/skillspector/nodes/build_context.py:3181: every real path containing !/ becomes a fatal FAILED artifact, even when no archive member collides with it.

   • reserved_paths is built from every artifact_inventory row whose path contains !/. That includes user-excluded and out-of-scope rows (merged at 2678-2694), and nothing checks whether nested inspection produced the same path. Lines 3182-3194 then set FAILED/artifact_path_collision and add a FAILED ledger event, while the bytes stay in raw_file_cache and are still analyzed (2931-2941).
   • Downstream, inspection_ledger.py:914-929 turns any failed inventory row into a fatal exception, so execution_successful is false and the CLI exits 2 (cli.py:858-859). inspection_ledger.py:985-992 counts the file as entirely uninspected, and _mark_unanalyzed_executables (build_context.py:1000) emits SC9 for a FAILED executable. The archive-side equivalents, ARCHIVE_UNSAFE_MEMBER_PATH and ARCHIVE_AMBIGUOUS_MEMBER_PATH, are in _PARTIAL_INVENTORY_REASONS (nested_artifacts.py:167-178) and are non-fatal.
   • Measured with scan --no-llm --format json through CliRunner, main against a transcription of the PR hunks (two reviewers, independently):
     • SKILL.md + tools!/run.sh (#!/bin/bash, echo hello, mode 755), no archive: main exit 0, complete, SAFE, score 0. PR exit 2, failed, SC9 HIGH, score 51, DO_NOT_INSTALL, entirely_uninspected=1.
     • Notes!/readme.txt, no archive: main exit 0, SAFE. PR exit 2, failed, CAUTION.
     • The same with --exclude 'Notes!/*': main exit 0 with a non-fatal user_exclusion. PR exit 2, with a fatal artifact_path_collision added.
     • node_modules/pkg/wow!/a.txt, which is out of scope and never read: main exit 0, complete, SAFE. PR exit 2.
     • A real bundle.zip with member other.txt plus a disk bundle.zip!/payload.txt (no shared key): main exit 0, complete. PR exit 2.
     • A malicious tools!/run.sh (curl piped to bash plus SSH-key exfiltration): main reports E1, PE3, SC2 and TM2. The PR reports the same four findings from the same bytes and also SC9 saying the content is outside analyzer coverage (score 97 to 100). The FAILED/uninspected accounting is therefore wrong, not just strict.
     • A control with tools/run.sh (no !) is identical on both.
   • test_reserved_disk_path_fails_even_without_an_archive (tests/nodes/test_nested_artifacts.py:1255) asserts this behaviour, and the PR body describes it, so it is deliberate policy rather than an oversight. It is still the wrong outcome: README.md:784 documents exit 2 as "Error (bad input, unreadable source, internal failure)", and here the input is benign and fully read. Renaming the directory is the only way out. Prevalence is probably low (no path in this repository contains !), but any skill that hits it gets the same signal as a scanner crash.
   • Expected fix:
     • Treat a disk !/ path as reserved only when nested inspection produced the same path for an archive member, for example by intersecting with {i["path"] for i in nested.artifact_inventory} | set(nested.file_cache) | set(nested.raw_file_cache) | set(nested.components). Do not use nested.inventory_overrides or nested ledger events on their own for this: they can also be keyed on the outer disk container (_exception(path=container_virtual_path, ...) at nested_artifacts.py:834-836), which would flag a real ZIP under a ! directory against itself.
     • Keep the rest of the hunk (FAILED mark, ledger event, blocking the nested merge, the override guard) for real collisions, whatever the disk row's prior disposition. Leave rows with no collision unchanged, or add at most a non-fatal note.
     • A reviewer prototype of this restriction (reviewer code, not the PR's) kept exit 2 with the disk finding for both the visible and hidden attack layouts, and returned every non-colliding case above to main's results.
     • Replace the no-archive test with tests asserting that real!/notes.txt with no archive stays analyzed and complete with exit 0, that an archive with no matching member stays complete, and that --exclude 'x!/*' stays non-fatal.

2. [Non-blocking] src/skillspector/nodes/build_context.py:3195: in the reverse layout (benign disk decoy, malicious archive member) the member is dropped and its findings disappear, with no record that archive content was withheld.

   • blocked_nested_paths includes reserved_paths, so the colliding member is removed from nested.metadata (3196), the nested components (3197-3199), local_file_cache (3201-3203), raw_file_cache (3204-3210) and the nested inventory (3211-3213). llm_file_cache is built from disk files only, so the member's bytes are not analyzed anywhere. The only record is the FAILED artifact_path_collision row and event on the path, and that message is about a filesystem path.
   • Measured (two reviewers, transcription):
     • payload.txt with an injection string, DEFLATE, benign disk decoy: main reports P1 HIGH and YR4 HIGH on bundle.zip!/payload.txt, score 40, CAUTION, complete, exit 0. The PR reports no findings, score 0, CAUTION, status failed, exit 2; the raw cache holds the decoy bytes and the member bytes are in neither cache.
     • The same with ZIP_STORED, which is what the test helper _write_archive writes: main score 50. The PR keeps only YR4 on the outer bundle.zip, score 20.
     • notes.md with prompt-injection and exfiltration text: main reports E1, P1 and PE3 (score 65 to 79 depending on the payload), DO_NOT_INSTALL, exit 1. The PR reports no findings, score 0, CAUTION, exit 2.
   • This is not a new attacker capability. Every collision now fails closed (exit 2, safe_to_install false at mcp_server.py:213-219), and main already lets an author hide a member with duplicate member names inside one archive (first wins at nested_artifacts.py:841-847; 0 findings, CAUTION, exit 0 on both main and the PR). The cost is in reporting: reports, SARIF and batch summaries (which sort by score) show score 0 and CAUTION where main showed HIGH findings and DO_NOT_INSTALL, and nothing tells the reader that an archive member was not analyzed. The PR tests cover only the malicious-disk orientation (tests/nodes/test_nested_artifacts.py:1229-1234).
   • Expected fix:
     • When a member is withheld because of a real collision, record it, for example with a ledger event that names the member, on the path or on its container, with a member-specific reason. ARCHIVE_AMBIGUOUS_MEMBER_PATH is the existing first-wins precedent.
     • Add a reverse-orientation regression test (malicious member, benign disk file at bundle.zip!/payload.txt) that asserts execution_successful is False, the result is incomplete and not SAFE, and the withheld-member record is present.
     • Analyzing both sides under distinct identities would keep both sets of findings. That is a larger change across findings, SARIF and reference resolution and fits better as a follow-up.

3. [Non-blocking] src/skillspector/nodes/build_context.py:3196: nested ledger events for the withheld member are still merged, unfiltered, at line 3489 and are reported against the disk file.

   • Lines 3181-3212 filter metadata, components, caches and inventory, but *nested.ledger_events is spread into the final ledger at 3489 as-is. Nested events carry the member's virtual path (_exception at nested_artifacts.py:594-630), which is the same string as the disk path, and the finalizer projects them into ledger_exceptions by path.
   • Measured (transcription): disk bundle.zip!/payload.txt is the 12-byte Plain note.; the member is 400,000 zero bytes, DEFLATE. The path's ledger_exceptions are artifact_path_collision (fatal) and archive_compression_ratio (non-fatal, observed_bytes=400000, limit_bytes=40400). The inventory has one row, failed, size_bytes 12, and the only metadata row is text, size 12, with no outer_path. An encrypted member adds archive_encrypted the same way; a benign member adds nothing.
   • Main was worse here (two inventory and two metadata rows, status partial), so this is an unfinished separation rather than a new problem. The verdict and exit code do not change, but the report says a 12-byte text file failed an archive compression-ratio check, which is the mixed provenance this PR sets out to remove.
   • Expected fix: re-attribute nested events whose path is in reserved_paths to the archive container that produced the member, with a note that the member was withheld (this fits finding 2). Plain filtering is weaker, because some member events are halting events (ARCHIVE_TIME_LIMIT at nested_artifacts.py:1048-1049) whose loss would hide that the rest of the archive was not inspected. Extend the collision test with a high-ratio member and assert that no archive_* reason appears for the disk path.

4. [Non-blocking] src/skillspector/nodes/build_context.py:3216: no test fails if the guard that keeps inventory overrides from downgrading FAILED is removed.

   • nested.inventory_overrides is not filtered by reserved_paths, so this guard is the only thing stopping a member override from rewriting the disk row. It only matters for reserved-path collisions: failed disk rows are never passed to nested inspection, and nested rows already carry their overrides.
   • Mutation (two reviewers, independently, transcription with the guard removed): on the PR test's shape (a 20-byte STORED member) the results are identical with and without the guard. With a colliding DEFLATE member that trips ARCHIVE_COMPRESSION_RATIO (400,000 zero bytes, or 200 KB of A), the guarded copy keeps failed/artifact_path_collision and the mutant gives partial/archive_compression_ratio. Main's own build_context sets both rows to partial/archive_compression_ratio in that case, which confirms that the override reaches the disk path.
   • inspection_ledger.py:914-929 restores fatal exceptions only from failed inventory rows when detail events are truncated, so without the guard the collision would survive truncation only through the separate prework event (reasoned from the code, not run end to end).
   • Expected fix: add a test whose colliding member triggers a PARTIAL-class override and assert the row stays failed/artifact_path_collision. Build the archive with _zip_bytes({"payload.txt": b"\x00" * 400_000}, compression=zipfile.ZIP_DEFLATED), since _write_archive uses ZIP_STORED and triggers no override. An encrypted member is not a good trigger: ARCHIVE_ENCRYPTED is FAILED-class, so the disposition would stay FAILED without the guard.

5. [Non-blocking] tests/nodes/test_nested_artifacts.py:1245: the hidden-archive case skips the retention and P1 checks, and the != checks at 1243-1244 also pass when the path is missing.

   • Line 1245 limits the == checks (1246-1247) and the P1 check (1248-1250) to bundle.zip. With the transcription, .bundle.zip also keeps the malicious disk bytes and reports P1 and YR4 on .bundle.zip!/payload.txt, so the skip is unnecessary.
   • A simulated regression that deletes the colliding path from both caches fails the bundle.zip case but passes every assertion of the .bundle.zip case, because None != benign is true.
   • The fatal status and exit 2 are already covered: the disposition check at 1241, together with inspection_ledger.py:914-929 and cli.py:858, forces execution_successful=false.
   • Expected fix: remove the startswith(".") guard, replace the two != checks with == malicious and == malicious.decode(), and assert the P1 finding for both parametrizations.

6. [Non-blocking] src/skillspector/inspection_ledger.py:167: the reason code says "collision" when there is none, and the message gives no remediation.

   • With Notes!/readme.txt and no archive, main's terminal renderer fed the PR's code and message (transcribed strings) prints artifact_path_collision Notes!/readme.txt: A filesystem path uses the reserved archive delimiter and cannot be attributed safely. The message names the cause class, but the code name points to an archive that does not exist, and nothing says how to fix it. Renaming the directory is the only remedy: user-excluded rows are re-added to the inventory (build_context.py:2678-2694) and then escalated at 3181-3194. The archive-side equivalent is named after the property (ARCHIVE_UNSAFE_MEMBER_PATH).
   • Expected fix: once the reason fires only for real collisions (finding 1), the name becomes accurate. Make the message actionable: quote !/, say that a directory name ends in !, suggest renaming it, and say that the archive member at that path was not analyzed (finding 2). If a note for non-colliding !/ paths is kept, give it its own code. The code is unreleased, so naming is free now.

7. [Non-blocking] src/skillspector/inspection_ledger.py:78: the new fatal reason and the reservation of !/ in real paths are not documented, and the PR changes no docs.

   • docs/NESTED_ARTIFACT_INSPECTION.md:8-12 documents the outer-file!/nested.zip!/... virtual path syntax, and its "Failure and completeness behavior" section (53-64) lists member-level failures only. Nothing in docs/ or the README mentions artifact_path_collision or says what happens to a real path containing !/. The PR that added the archive-side !/ reservation (#382) updated NESTED_ARTIFACT_INSPECTION.md, the CHANGELOG and the README.
   • Users and integrators who get exit 2 for this have nothing to explain why or how to avoid it.
   • Expected fix: add a short paragraph to docs/NESTED_ARTIFACT_INSPECTION.md (after the syntax example or in the failure section) saying that !/ is reserved, that a real file colliding with an archive member is reported as artifact_path_collision and the member is not merged, what that does to the result (exit 2, safe_to_install=false), and that renaming the directory fixes it. Word it to match the outcome of finding 1, and optionally add a CHANGELOG line. docs/scan-completeness.md is the design note for #563 and does not need a row.

8. [Non-blocking] src/skillspector/nodes/build_context.py:3208: one level deeper, archive members still shadow the members of a real container under a ! directory (pre-existing, untested).

   • The unchanged nested input list (build_context.py:2963-2969) includes a reserved disk container such as bundle.zip!/inner.zip. bundle.zip sorts first and is expanded first, so its own inner.zip!/x.txt claims bundle.zip!/inner.zip!/x.txt. When the disk container is expanded later, its x.txt is skipped as ARCHIVE_AMBIGUOUS_MEMBER_PATH (nested_artifacts.py:841-847). The exact-path filters at 3195-3213 drop the archive member bundle.zip!/inner.zip but not its child, which passes this filter.
   • Measured (transcription; malicious x.txt in the disk inner.zip, benign in the archive's): the cache holds the benign bytes, x.txt is partial/archive_ambiguous_member_path, and there is no P1 (no findings with DEFLATE; only YR4 on bundle.zip!/inner.zip with STORED). The result is CAUTION with execution_successful=false. Main gives the same shadowing (CAUTION, incomplete), so this is not a regression, and the verdict fails closed. Without a colliding archive, both main and the PR report P1 and YR4 at that path.
   • Expected fix (low priority, can be a follow-up): order the inspect_nested_artifacts inputs at 2963-2969 so disk paths containing !/ are inspected first. A reviewer prototype with a stable sorted(inputs, key=lambda p: "!/" not in p) restored P1 at depth 2 and left depth 1 and the no-collision case unchanged. Excluding reserved disk paths from nested inspection would make it worse, since it loses the P1 reported today when no archive collides. Add a depth-2 regression test, or at least one that pins today's fail-closed behaviour.

PIC tradeoffs:

• A collision leaves one cache key for two sources, so one side has to yield or be renamed. Keeping the disk bytes and failing closed is the simplest choice and fixes the reported layout, but moves the lost findings to the reverse layout (finding 2). Giving both sides distinct identities keeps all evidence at the cost of changes across findings, SARIF and reference resolution.
• FAILED or PARTIAL for a real collision: FAILED (fatal, exit 2, immune to later overrides) is defensible for a genuine collision. The archive-side analogues ARCHIVE_UNSAFE_MEMBER_PATH and ARCHIVE_AMBIGUOUS_MEMBER_PATH are PARTIAL, which already blocks SAFE and safe_to_install. Either is reasonable if chosen deliberately; neither fits a directory name with no collision (finding 1).
• Reserving !/ for real paths has a case beyond collisions: other code reads !/ as archive provenance (static_patterns_supply_chain.py:3472, static_runner.py:2408 and 2498). A non-fatal note on non-colliding !/ paths would keep the reservation visible without turning benign skills into failed scans.
• Filtering each store separately (metadata, components, caches, inventory) rather than through one provenance-keyed merge already missed nested.ledger_events (finding 3). Any future nested store would need the same filter.

Verification and gaps:

• Code traces on head 2d6defd: the hunks at build_context.py:3179-3217 and inspection_ledger.py:78 and 167-169, and their consumers at inspection_ledger.py:914-929, 985-992 and 1046, cli.py:858-859, mcp_server.py:213-219, build_context.py:1000 and 3489, and nested_artifacts.py:167-187, 362 and 834-847.
• Baseline runs of main (graph.invoke with use_llm=False, and CliRunner scan --no-llm --format json) reproduced the bug for the visible and hidden archive names: two analyzed rows, benign bytes cached, no P1, complete, SAFE, exit 0.
• PR behaviour comes from reviewers' own transcriptions of the two source hunks onto copies of main 3c8e4b9, run with main's venv. Each copy differs from the head only by the two comment lines at build_context.py:3179-3180, or not at all. Results: the reported layout is fixed (P1 and YR4 kept, failed, exit 2), an ordinary ZIP and a control skill without ! are unchanged, and the numbers in findings 1-5 and 8.
• No other collision route: virtual paths are always container!/member (nested_artifacts.py:840), _safe_member_name rejects member names containing !/ (nested_artifacts.py:362), inspect_nested_artifacts has a single caller (build_context.py:2964), and symlinked disk entries are never cached. The new REASON_MESSAGES entry means the message lookups cannot raise KeyError, and the broadened guard at 3216 changes behaviour only for rows that are already FAILED.
• main's test suite run against a transcription: 10,533 passed; the 3 failures were batch_scan worker-timing tests that pass in isolation and are unrelated. A second reviewer ran about 880 tests in 9 related modules with the same outcome.
• Tests: both new tests would fail on main (the core test at len(inventory) == 1, because main has two rows; the no-archive test at disposition == FAILED). They use production code paths without mocks. None covers the reverse layout, the override guard, depth 2, or a non-colliding !/ path keeping its result; the second test asserts the opposite of that last case.
• CI: no checks have run on the head; the workflow run is action_required and waiting for maintainer approval.
• Conflicts: none with main. This PR shares build_context.py with #794 and #795 and inspection_ledger.py with #795; all merge cleanly and no semantic conflict was found. #794 renames primary_content_events and adds OPAQUE_CONTENT PARTIAL events, so a reserved readable-binary path would get both events and FAILED still dominates. #795 changes OMS signature handling for the root skill.oms.sig, which cannot contain !/.
• Head update: I reviewed 2d6defd4e821fb861866b8b836e70283e08ba548. The current head 201a6ec48df626d45f0643bb6a6c6b44c17c784e only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files, so line references below are to the current head. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Gaps: local runs used Python 3.13 on macOS (CI uses 3.12). Windows and case-insensitive filesystems were not exercised; cache keys are exact as_posix strings. use_llm=True was not tested. MCP, SARIF and Markdown output were checked by code reading and through the shared ledger_exceptions path, not end to end, and the batch scanner and Pi/OpenCode extensions were not run. The real-world prevalence of directories ending in ! is unmeasured.

---

Decision: Changes Requested (reviewed head 2d6defd4e821fb861866b8b836e70283e08ba548; current head 201a6ec48df626d45f0643bb6a6c6b44c17c784e only adds merges of main)

already resolved

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for the careful follow-up. Three parts of it stand out:

• The collision failure now applies only to paths that nested inspection actually produced, and a parametrized test covers the plain, excluded, out-of-scope, different-member and container layouts.
• While merging #579, you extended the reserved-path filters to its new python_source_classifications store and added a test for both orientations.
• The new tests for the override guard, the input sort and the classification filter each fail when that piece of code is removed.

Value and readiness: The earlier blocker is fixed, and the change is ready for maintainer review.

• Attack layout. The reported layout is a malicious disk file bundle.zip!/payload.txt next to a bundle.zip whose member payload.txt is benign. On main it ends complete, SAFE, exit 0. With this PR it still fails closed: one failed/artifact_path_collision row, disk bytes kept, P1 and YR4 reported, exit 2.
• Ordinary ! directories. Directories ending in ! that have no collision give main's results in all six layouts I measured.
• Earlier items. All eight earlier findings and the #579 impact item are resolved, except finding 3, which is partly resolved.
• Remaining issues. Two non-blocking provenance issues remain (findings 1 and 2 below). Both occur only in collision layouts that already fail with exit 2. Neither changes a verdict or loses a finding compared with main, so they can be fixed here or in a follow-up.

All 6 CI checks pass on this head. GitHub reports it MERGEABLE/CLEAN, and every earlier review thread is resolved.

Previous findings:

The thread replies cite 2660d977. That is a merge commit. The fixes are in 89de1080 and e17d593d, and the python_source_classifications filter arrived with the merge b9db2f0c.

1. Resolved. Finding 1 [Blocker]: every real path containing !/ became a fatal FAILED artifact.

   • Fix: a path is reserved only when a disk inventory path equals a path that nested inspection produced, through its artifact_inventory, components, file_cache or raw_file_cache (src/skillspector/nodes/build_context.py:3252-3259). The no-archive test was replaced by test_noncolliding_disk_delimiter_paths_remain_complete (tests/nodes/test_nested_artifacts.py:1409-1437).

   • Evidence (transcription): six layouts give main's status, verdict, score, findings and exceptions, with no SC9 and no artifact_path_collision:

     • tools!/run.sh (mode 755);
     • Notes!/readme.txt;
     • --exclude 'Notes!/*' (partial/CAUTION on both);
     • node_modules/pkg/wow!/a.txt;
     • bundle.zip{other.txt} plus a disk bundle.zip!/payload.txt;
     • real!/bundle.zip.

     Through the CLI, tools!/run.sh and the excluded case exit 0 on both main and the PR.

   • Author's reply: accurate.

2. Resolved. Finding 2: the reverse layout dropped the archive member with no record.
   • Fix: for each colliding path, a PARTIAL archive_ambiguous_member_path event is recorded on the member's container, with the member path appended to its message (build_context.py:3265-3277). test_withheld_member_events_keep_archive_provenance (tests/nodes/test_nested_artifacts.py:1439-1479) covers the benign-disk, malicious-member orientation.
   • Evidence (transcription):
     • DEFLATE injection member: status failed, execution_successful false, CAUTION, exit 2, with bundle.zip archive_ambiguous_member_path (non-fatal) and a fatal artifact_path_collision on the disk path.
     • notes.md member: main gives DO_NOT_INSTALL, score 59, exit 1. The PR gives failed, CAUTION, exit 2, with the same two records.
   • Author's reply: accurate, with one caveat. The member name appears only in the internal state["inspection_ledger"] event, which is what the test asserts on. CLI JSON, SARIF, Markdown, terminal and MCP output build the message from REASON_MESSAGES (src/skillspector/inspection_ledger.py:622, via _exception_from_event at 643), so they show the generic text. A search of the CLI JSON for the appended text found nothing. Users can still identify the member: its path is the disk path, and that row's artifact_path_collision message says "the colliding archive member was not analyzed". No change is needed.
3. Partly resolved. Finding 3: nested events for the withheld member were reported against the disk file.
   • Fix: nested events whose path is reserved are moved to the member's container, with a note appended (build_context.py:3278-3287). The new work_id uses the same formula as ledger_event (inspection_ledger.py:464-466).
   • Evidence (transcription): in the 400,000-zero DEFLATE case, bundle.zip gets archive_compression_ratio (the internal event keeps observed_bytes 400000), and the disk path gets no archive_* exception.
   • What remains:
     • The loop cannot tell member events apart from events that nested inspection recorded about the disk file itself, so it moves both (finding 1 below).
     • Events from the withheld member's descendants are not moved (finding 2 below).
     • Part of the over-reach traces to my earlier request to assert that "no archive_* reason appears for the disk path". The loop now enforces that for every event. The request should have been limited to events produced by the member.
   • Author's reply: accurate for events at the member's own path: their reason and limit values are preserved. The note naming the member is internal only, as in item 2.
4. Resolved. Finding 4: no test failed if the FAILED override guard was removed.
   • Fix: the provenance test is parametrized with b"\x00" * 400_000 DEFLATE and asserts the row stays failed/artifact_path_collision.
   • Evidence (transcription, mutation): with the guard removed, the row becomes partial/archive_compression_ratio, so the test fails.
   • Author's reply: accurate.
5. Resolved. Finding 5: the hidden-archive case skipped the retention and P1 checks.
   • Fix: tests/nodes/test_nested_artifacts.py:1382-1406 asserts the exact malicious bytes in raw_file_cache and local_file_cache, and P1, for both bundle.zip and .bundle.zip, with no startswith(".") guard.
   • Evidence (transcription): the .bundle.zip layout gives one failed row, the malicious bytes retained, P1 and YR4 on .bundle.zip!/payload.txt, and exit 2.
   • Author's reply: accurate.
6. Resolved. Finding 6: the reason fired without a collision, and the message gave no remedy.
   • Fix: the reason now fires only on real collisions. The message (inspection_ledger.py:178-181) says the path collides with an archive member, tells the user to rename the directory ending in !, and says the colliding member was not analyzed.
   • Evidence: artifact_path_collision appears in none of the six non-colliding layouts in item 1.
   • Author's reply: accurate.
7. Resolved. Finding 7: the new fatal reason was undocumented.
   • Fix: docs/NESTED_ARTIFACT_INSPECTION.md:66-70 covers real collisions, retention of the disk bytes, exit 2 and safe_to_install=false, the rename remedy, and that non-colliding ! directories are allowed. The optional CHANGELOG line was not added; that is fine.
   • What remains: line 70 ("Archive exceptions for a withheld member are reported against its container") needs a small wording change once findings 1 and 2 below are settled.
   • Author's reply: accurate.
8. Resolved. Finding 8: depth-2 shadowing of a real disk container's members.
   • Fix: nested inputs are stably sorted by -path.count("!/"), so real containers under a ! directory expand first (build_context.py:3035-3042). test_disk_container_children_take_precedence_over_colliding_archive (tests/nodes/test_nested_artifacts.py:1481-1497) asserts the malicious disk bytes, P1, and execution_successful false.
   • Evidence (transcription):
     • With the sort, the disk child keeps its malicious bytes and P1, with status failed and score 50.
     • Without the sort (mutation), the cache holds the archive's A plain note., P1 is gone and the score is 20, so the test pins the sort.
     • The ordering argument holds: an archive can produce P!/c only after producing P, so a disk container at P has more !/ segments and always expands first.
   • What remains: finding 8 also noted that the exact-path filters do not cover the member's descendants. You did what that finding asked: the sort and a depth-2 test. That residual is finding 2 below.
   • Author's reply: accurate.
9. Resolved. Impact check: #579's NestedInspectionResult.python_source_classifications was not covered by the reserved-path filters.
   • Fix: the store is filtered by blocked_nested_paths (build_context.py:3303-3307) before it is used at 3349 and 3407. test_collision_keeps_python_identity_bound_to_disk_bytes (tests/nodes/test_nested_artifacts.py:1357-1379) covers both orientations.
   • Evidence (transcription):
     • Dangerous disk Python runner next to a text member: AST2, AST4, E1, PE3, TT4, TT5 and SC9, DO_NOT_INSTALL, and the file is classified python. On main: SAFE with 0 findings.
     • The same layout without the filter (mutation): the file is classified non_python, and AST2, AST4, TT4 and TT5 are lost.
     • Python member next to a disk text file: the disk file stays non_python.
   • Author's reply: not applicable. The PR body states this behaviour.

Material findings

1. [Non-blocking] src/skillspector/nodes/build_context.py:3278: the event re-attribution also moves exceptions that belong to the disk file, and blames the valid outer archive for them.

   • Code:
     • The loop at 3278-3287 moves every nested event at a reserved path to the archive container and appends "This applies to withheld archive member ..., not the disk file." It never checks where the event came from.
     • In src/skillspector/nested_artifacts.py (unchanged by this PR), archive_format_mismatch is recorded only in the top-level loop over disk inputs (1287-1291 and 1308-1312). archive_malformed is recorded only at depth 1 (735-740); deeper members get archive_truncated. Archive members are never format-checked.
     • So when either reason appears on a reserved path, it can only belong to the disk file. The disk path is always a top-level input, so this happens whenever the colliding disk file is a mistyped or broken container.
   • Measured (transcription, graph.invoke, use_llm=False):
     • Disk bundle.zip!/doc.docx (plain text) next to bundle.zip{doc.docx: text}:
       • Main: archive_format_mismatch on the disk path.
       • PR: bundle.zip archive_ambiguous_member_path, bundle.zip archive_format_mismatch, and a fatal artifact_path_collision on the disk path. Nothing archive-related stays on the disk path.
       • Without the colliding member, both main and the PR keep the mismatch on the disk path.
     • Disk bundle.zip!/inner.zip (a PK header followed by garbage) next to a text member inner.zip:
       • Main: a fatal archive_malformed on the disk path.
       • PR: a non-fatal archive_malformed on bundle.zip, which is a well-formed archive.
     • Plain-text disk bundle.zip!/inner.zip next to a member inner.zip that is a valid ZIP: the disk file's archive_format_mismatch moves to bundle.zip. The internal note says it applies to the withheld member, which is the valid ZIP, so the note has the attribution backwards.
   • Consequence: the verdict does not change, because the collision is still fatal. But reports show bundle.zip as mismatched or malformed, with the standard reason text, and the disk path loses its own container problem. This is the mixed provenance the fix for finding 3 set out to remove, now in the other direction. docs/NESTED_ARTIFACT_INSPECTION.md:70 does not describe it.
   • Expected fix:
     • Narrow fix: in the loop, leave archive_format_mismatch and archive_malformed events on the disk path when that path is in nested.outer_metadata. That map records only top-level inputs (nested_artifacts.py:821-828 at depth 1, and 1265, 1280, 1301 and 1317).
     • Add a test with a non-ZIP disk bundle.zip!/doc.docx and a colliding member. Assert that archive_format_mismatch stays on the disk path and does not appear on bundle.zip. The existing assertion at tests/nodes/test_nested_artifacts.py:1468-1471 still holds, because that disk file is a .txt.
     • A general fix needs each event's origin. One option is to skip the colliding member inside _inspect_zip_bytes. A reviewer prototype of that (reviewer code, about 30 lines, not the PR's) records the ambiguity on the container and keeps these exceptions on the disk path in all three layouts. It also withholds the member's whole subtree, though, which is a policy change with a cost (see finding 2).
     • Align the docs sentence at line 70 with whichever behaviour you keep.

2. [Non-blocking] src/skillspector/nodes/build_context.py:3301: the filters and the event loop match reserved paths exactly, so the withheld member's descendants are still handled under the disk path's namespace. In the PR's own depth-2 layout, the fully analyzed disk child is labelled partial and ambiguous.

   • Code:
     • The filters at 3301-3324 and the loop at 3278-3287 use exact membership in reserved_paths.
     • When the colliding member is itself a ZIP, nested inspection has already expanded it (nested_artifacts.py:1166-1177).
     • Because of the sort, the disk container expands first. The archive's copy of the same child path then hits the duplicate check (nested_artifacts.py:856-861), which records archive_ambiguous_member_path on that path and sets inventory_overrides for it (598).
     • The override loop at build_context.py:3325-3328 then applies to the disk child's row.
   • Measured (transcription):
     • Depth-2 test layout: the disk child bundle.zip!/inner.zip!/payload.txt keeps its malicious bytes and P1, and its metadata outer_path is the disk container bundle.zip!/inner.zip. But its row is partial/archive_ambiguous_member_path, and the public exceptions include archive_ambiguous_member_path on that path. The PR introduced this. On main, the row was also partial, but there the disk child really was the copy that was skipped.
     • Distinct child (disk inner.zip{a.txt}, archive inner.zip{b.txt} with an injection): bundle.zip!/inner.zip!/b.txt is analyzed, P1 and YR4 are reported on it, and its outer_path correctly says bundle.zip. Meanwhile, the internal container note says member bundle.zip!/inner.zip "was not analyzed". Main also analyzes b.txt (complete, execution_successful true), so only the contradictory note is new.
     • Plain-text disk bundle.zip!/inner.zip with a ZIP member holding payload.txt: P1 and YR4 are reported at bundle.zip!/inner.zip!/payload.txt (outer_path bundle.zip). Main reports the same P1.
     • All three end with status failed, execution_successful false and exit 2.
   • Consequence: a fully analyzed disk file is reported as partial with ambiguous provenance, and the note says the member was not analyzed when its children were. docs/NESTED_ARTIFACT_INSPECTION.md:70 says more than the code does. This is not a bypass, and no finding is lost compared with main.
   • Expected fix:
     • At minimum, stop the archive side's duplicate-skip from marking disk rows. For a path under <reserved>!/ whose row came from the disk container (its metadata outer_path is the reserved disk path), move the archive_ambiguous_member_path event to the archive container and do not apply the override.
     • Extend the depth-2 test to assert that the disk child stays analyzed, with no archive_ambiguous_member_path on its path.
     • Then choose and document a policy for the member's own descendants, and add a distinct-child test that pins it. Either option still fails closed.
       • (a) Keep analyzing them, which is the current behaviour and main's. Then word the note and docs line 70 to say only the member file itself was not analyzed.
       • (b) Withhold the whole subtree, as the direct reverse layout already does. The source-side skip from finding 1 gives (b), but it drops findings that both main and the PR report today. In the prototype, the distinct-child case goes from P1 and YR4 on b.txt (score 50) to YR4 on bundle.zip only (score 20). The plain-text case goes from DO_NOT_INSTALL (score 59) to CAUTION (score 29), and P1 is lost.

PIC tradeoffs:

• Post-hoc filtering or a source-side skip.
  • The PR runs nested inspection unchanged, then filters each store by exact path and moves events. That keeps the change local, but it cannot tell an event's origin (finding 1) or a descendant's (finding 2). Every new nested store also needs its own filter; you added one for #579's store.
  • Skipping the colliding member in _inspect_zip_bytes removes the sort, the filters and the event loop. It changes the descendant policy, though, and drops findings in the layouts in finding 2.
  • Either is acceptable if the docs match.
• FAILED for a real collision. This remains a sound choice. One side effect: a colliding executable disk file also gets SC9 HIGH and excluded_executable_content, even though its bytes were analyzed. In the disk Python layout, AST4 and TM1 are reported alongside SC9. I accept this because it only affects genuine collisions, which already exit 2.
• Recording withheld members, or analyzing both sides. The PR records them. Analyzing both sides under distinct identities would keep all evidence, but it remains a larger follow-up across findings, SARIF and reference resolution.

Verification and gaps:

• Head: 3d0d9cc642fd1f62dbcf6d68f3c9ba8106b615f8 is the live head (re-checked with gh pr view just before writing). Its merge-base is current main 5edc347c, and the diff touches only the four PR files. The author's merge commits add nothing outside them.
• CI: changes, lint, test-unit, OpenCode TypeScript Tests, DCO Check and docker-smoke all completed with SUCCESS on this head. GitHub reports MERGEABLE/CLEAN, and all 8 earlier review threads are resolved.
• Conflicts: none with main; git merge-tree is clean. Against the other open PRs:
  • Merge-tree is also clean with #794, #795 and #741. #796, #797 and #798 touch disjoint files.
  • #794's readable-binary block and its renamed content_interpretation_events run after this hunk and only upgrade ANALYZED rows, so FAILED still dominates.
  • #795's OMS projection reads local_file_cache, which keeps the disk bytes for colliding paths.
• Method:
  • PR behaviour comes from my transcription of the live-head source hunks onto main 5edc347c. Checked against git show of the head, it differs only in local variable names, and inspection_ledger.py is byte-identical.
  • I ran it with main's venv through graph.invoke (use_llm=False) and CliRunner scan --no-llm --format json. Main baselines and the mutation runs used the same harness.
  • The recomputed work_id cannot collide with analyzer work: SYSTEM events never match analyzer planned_work, and _merge_exception_projection merges public rows with the same key by design.
• Tests: I ran main's tests from 9 related modules against the transcription: test_nested_artifacts, test_build_context, test_explicit_exclusions, test_finalize_inspection_ledger, test_analysis_completeness, integration/test_graph, test_primary_input_completeness, test_shared_python_ast and test_opaque_reference_reporting. 447 passed and 103 were deselected by markers. The full suite was not run.
• I did not run the PR's tests or code, per policy.
• Gaps:
  • Local runs used Python 3.13.14 on macOS; CI uses 3.12. Windows, case-insensitive filesystems and use_llm=True were not exercised.
  • SARIF, Markdown, terminal and MCP output were checked by reading code only (report.py:910, 1050 and 1530, and mcp_server.py:214-219).
  • The PR body's figures could not be checked independently: 744 tests against source and wheel, 6,243 combined regression tests, 19 sample pairs, and the live NVIDIA Build checks.
  • nested.recognized_zip_paths is still unfiltered for withheld ZIP members (build_context.py:3343). The impact is limited because the colliding disk row is already FAILED; I did not measure it.
  • The source-side prototype mentioned in findings 1 and 2 is reviewer code. I checked it only against the layouts listed here and the same 447-test subset.

---

Decision: Approved (reviewed head 3d0d9cc642fd1f62dbcf6d68f3c9ba8106b615f8)