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