fix(suppression): separate local and transitive content caches

[yashrajp22]

A local file named like a transitive source path could have its cached contents overwritten by the child scan. This let an exact baseline bind to unchanged remote bytes when the local file had changed. Keep transitive content in an internal namespace that cannot be a filesystem path and require it for source-scoped baseline lookups. Display and coverage paths stay unchanged.

Validation: 506 tests passed against both source and a freshly installed wheel. Regressions drive the real transitive merge and report: changed local content remains active, an unchanged child is suppressed by its exact baseline, and changed child content becomes active again. The unusual normalized-path case is covered. Remote/provider responses are synthetic; these focused tests made no live provider calls.

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 moving transitive file content into a key namespace that no real path can occupy! The NUL-prefixed JSON key is a small, fail-closed change, and the new CLI test drives the real transitive merge and report node rather than a hand-built cache.

Value and readiness: The problem is real on main. _source_aware_file_cache re-keyed each child's file cache as external/<identity>/<path>, which is also a legal relative path inside the root skill. _bounded_cache_update then overwrote the root entry with the child's bytes, and partition_findings fingerprinted the remote content instead of the local file. I reproduced this on main with real --no-llm scans of a local child skill and a local root skill. The baseline was built the way skillspector baseline builds it, and then line 29 of the root's external/<identity>/payload.md was edited. P1 and YR4 on the changed file stayed suppressed, and the report recommended CAUTION (score 40). With my transcription of the PR's four changed functions, both findings are active and the recommendation is DO_NOT_INSTALL (score 60). One correction to the impact: in a real scan the same lookalike path also collides in the inspection ledger. Both main and this PR therefore mark that scan failed, with a fatal unaccounted_work exception at the lookalike path, and the CLI exits 2. The earlier reproductions mocked the child scan without ledger events and reported success. So on main the wrong suppression shows up inside a scan that already fails closed, and the PR makes the findings and score in that report correct. The fix is narrow, exact child baselines still round-trip through the real merge, and ordinary scans are unaffected. The two notes below are non-blocking. Ready for final maintainer review once CI has run.

Material findings

1. [Non-blocking] tests/unit/test_cli.py:102: no test covers a successful exact suppression of a transitive child finding through the real merge, or the new NUL-key skip at src/skillspector/suppression.py:204-205.
   • The new test only checks the negative case (a changed local file is not suppressed). The NUL-keyed tests in test_suppression.py build caches by hand with source_content_key(...) and never go through _cache_transitive_result -> _source_aware_file_cache -> _bounded_cache_update. The only other _scan_transitive test that passes a baseline (test_cli.py:4646) uses a glob rule.
   • The positive round trip works today (transcription): an unchanged child gives suppressed=1, and changed child bytes give active=1. In a simulated regression, scoped child findings carried the display path {identity}/payload.md, as if _scope_finding (cli.py:1298-1305) rewrote finding.file. The PR's new test assertions still all held, but a baseline built with correct code silently stopped matching (suppressed 0, active 1, no error).
   • This fails closed and is not a security hole. The baseline command has no --transitive option, so only baselines built by calling build_baseline_dict on a merged scan --transitive result are affected. The NUL skip only matters for contrived child names such as ..\..\..\x, whose key normalizes to x"].
   • Expected fix: add a sibling CLI test. It should run _scan_transitive once with a mocked child, build build_baseline_dict from the scoped child finding and the captured merged local_file_cache, then re-run and assert the finding is in suppressed_findings, and active again once the child bytes change. Optionally, add a unit test that a root finding ./x"] does not resolve to a child key for ..\..\..\x.
2. [Non-blocking] src/skillspector/cli.py:1741: the new keep-first conflict branch in _bounded_cache_update cannot be reached under the new keys, has no test, and would be reported as an output-limit truncation.
   • Child keys always start with \0 (suppression.py:175-181). Root keys come from filesystem paths or nested {outer}!/{name} keys, and _safe_member_name rejects names containing NUL. Within one traversal, each target reuses one cached result (cache_key=(target, source_local_only), cli.py:2126-2144), so a repeated key always carries identical content and takes the silent continue. No test calls _bounded_cache_update with conflicting values.
   • Forced with synthetic conflicting dicts (transcription), it keeps the first value, sets resource_limit_reached, and records local cache contains conflicting source content. That adds a PARTIAL LedgerReason.OUTPUT_LIMIT transitive_traversal event (cli.py:2361-2376) and shows the reason in transitive_truncation_reasons.
   • There is no runtime effect today. If a later key change makes collisions possible again, the scan would be marked partial for an output-limit reason, with an internal-sounding message.
   • Expected fix: either add a direct unit test of _bounded_cache_update with a conflicting key (keep-first plus the recorded reason), or drop the branch now that collisions cannot happen by construction. If it stays, word the reason in user terms and keep it out of the output-limit category.

PIC tradeoffs:

• Programmatic callers of partition_findings or build_baseline_dict that built caches with the old {identity}::{path} or {identity}/{path} keys now get no suppression, or a source content missing ValueError. Only in-repo tests did this, and the PR updates them. No persisted data depends on those keys.
• Source-scoped lookups now need an exact finding.file match; ./SKILL.md no longer resolves to SKILL.md as it did on main. This fails closed. Across 14 real --no-llm scans, finding and occurrence paths always matched the cache keys exactly.
• A NUL-prefixed key separates the namespaces without changing the state shape. A separate source_file_cache dict would make the split explicit to readers and type checkers, at the cost of wider changes to the state and report signatures.
• Display, coverage and ledger keys keep the external/<identity>/<path> form, so output stays stable. A root file at that path still collides there. The ledger catches it, and the transitive scan is marked failed (unaccounted_work, exit 2) both before and after this PR. That is fail-closed. A follow-up could key those views on (source_identity, path) so such a scan is handled instead of failing.

Verification and gaps:

• Traced on the head: cli.py:1154-1160 (_source_aware_file_cache), cli.py:1731-1748 (_bounded_cache_update) and its only callers at cli.py:2278 and :2285, and suppression.py:175-208 (source_content_key, _component_content). _component_content is used only by partition_findings and build_baseline_dict, and partition_findings is called only from nodes/report.py:1797. Merged caches are not emitted in JSON, SARIF or Markdown output. The baseline file format is unchanged.
• Real-pipeline reproduction (my own script; it runs main's code with the four changed functions transcribed): on main the merged cache held the original remote bytes at the lookalike path and P1/YR4 were suppressed (CAUTION, 40). Under the transcription the cache held the changed local bytes, both findings were active (DO_NOT_INSTALL, 60), and there were no truncation reasons. Both runs ended with execution_successful=False from the ledger collision described above.
• Reviewer transcriptions driving _scan_transitive with a mocked child scan (the PR test's seam): the root finding is active for both the / and :: lookalike paths. An exact child baseline built from the merged NUL-keyed cache suppresses on re-scan and stops suppressing when the remote bytes change. An unchanged root lookalike stays suppressed, so the fix does not over-correct.
• main's suites with the transcription patched in: 704 passed in test_cli, test_suppression and test_transitive; the 1 failure is the old ::-key test this PR rewrites. A further 802 passed in the report, e2e, remediation, occurrence-columns and MCP-server suites.
• Tests: the new CLI test would fail on main (ImportError for source_content_key, and its first assertion is false on main). The new lookalike and key-injectivity tests in test_suppression.py would also fail on main. The CLI test's mocked child has no ledger events, so it does not exercise the ledger collision.
• ruff format --check passes on the four touched files. The two ruff check I001 results also appear on main's copies.
• CI: no checks have run on the head; the CI workflow run is action_required and needs maintainer approval.
• Conflicts: none with main (a0d489e). PR #798 also touches cli.py, suppression.py and tests/unit/test_cli.py, but its hunks (_write_result, multi-skill output, dump_baseline, imports, tests appended at the end of test_cli.py) do not touch cache keys, _component_content or the merge. It merges cleanly with no semantic interplay.
• Head update: I reviewed d7f8ef0cb00d1bc4538547449e378e609c64d33c. The current head 838f9e999ec353df90735bc907102076c2cc3e9a only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files. 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: all runs used Python 3.13 on macOS (CI uses 3.12); nothing here is version-specific. There was no real remote fetch and no full skillspector scan --transitive --baseline CLI invocation. All scans were --no-llm, so LLM-analyzer finding paths were checked only by code trace (llm_analyzer_base.py:617-630).

---

Decision: Approved (reviewed head d7f8ef0cb00d1bc4538547449e378e609c64d33c; current head 838f9e999ec353df90735bc907102076c2cc3e9a only adds merges of main)

[yashrajp22]

Thanks for checking this! I added a test that builds an exact baseline from the real transitive merge, confirms an unchanged child is suppressed, and confirms changed child content is active again. I also covered the unusual normalized-path case and removed the unreachable collision warning. All 506 focused tests pass against both source and a freshly built wheel.