feat(scan): add opt-in transitive reference scanning#225
Conversation
rng1995
left a comment
There was a problem hiding this comment.
Requesting changes. Transitive prefix controls can be escaped through non-normalized URL paths, traversal has no fan-out budget, and shared visited state causes sibling skills to omit shared dependency findings. The merged report also misreports dependency coverage, several report tests are no longer collected, and the branch conflicts with current main. Please address the inline blockers and rebase while preserving current CLI, input-handler, and reporting behavior.
47dfb07 to
4fb451c
Compare
|
Addressed the blockers on the transitive traversal path rather than patching each edge case in isolation. The branch now normalizes target identities before allow or deny checks, applies an explicit traversal budget, reuses cached child results per referring root instead of suppressing sibling findings behind one shared visited set, keeps dependency coverage source-aware in reporting, restores the report tests that had fallen out of pytest collection, and replays the whole change onto current |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review: still requesting changes. The prior path-normalization, per-root cache/visited, source-aware coverage, and test-collection issues are addressed. Remaining blockers are that byte/time limits are checked only after a child scan, recursive roots do not share a command-level budget, child LLM degradation is dropped, baseline-suppressed transitive findings are overcounted, and the branch still conflicts with current main.
5dc441a to
cb5ffa1
Compare
|
Rebased onto current |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review of cb5ffa1 (rebased single commit). The PR adds opt-in --transitive scanning with canonicalized allow/deny prefix controls, a command-level target/byte/time budget shared across recursive roots, per-root visited state with a shared dependency result cache, cached child LLM telemetry, and source-aware reporting.
Prior-issue resolution checklist
| Prior blocker (review 2026-07-09) | Status | Evidence |
|---|---|---|
| Byte/time limits checked only after a child scan | Resolved | Downloads are now streamed against remaining_bytes with mid-stream abort; git clones get --filter=blob:limit=N, a remaining_seconds subprocess timeout, and a post-clone tree-size check; file reads in build_context._read_file_cache are capped; LLM calls take per-call dynamic timeouts (LLMAnalyzerBase._model_for_call). Tests: test_input_handler_rejects_oversized_transitive_git_clone, TestDynamicTimeout, test_dispatches_timeout_to_cli_provider_complete. |
| Recursive roots do not share a command-level budget | Resolved | _scan_multi_skill creates one _TransitiveTraversalState (shared cache + counters + deadline) passed to every root; visited sets stay per-root. Tests: test_recursive_transitive_root_graphs_share_one_traversal, test_scan_transitive_root_graph_uses_shared_traversal. |
Cached child llm_call_log dropped |
Resolved | _CachedTransitiveResult.llm_call_log is stored and merged into the final report state; degraded metadata verified by test_scan_transitive_preserves_cached_child_llm_telemetry. |
transitive_finding_count counted pre-baseline |
Resolved | Count now derives from report_result["active_findings"] after baseline partitioning; test_scan_transitive_counts_only_active_post_baseline_findings covers suppression. |
Branch conflicts with main |
Resolved | Rebased to a single commit cb5ffa1; GitHub reports the PR mergeable. |
New blockers
- Root-scan byte-budget truncation silently zeroes coverage. When
--transitiveis set, the root skill's file reads consume the shared 10 MB budget (build_context._read_file_cachebreaks onremaining_bytes), and every unread file is dropped fromcomponents/component_metadataentirely._build_analysis_completenessthen computes totals fromcomponent_metadata(i.e., only the files that were read), so a truncated scan reports 100% coverage, the dropped files never reach any analyzer, risk stays LOW, and the CLI exits 0 — only a truncation string in metadata hints anything was skipped. A skill padded with one >10 MB file that sorts first produces an empty root scan (file_cache={}, 0 findings) under--transitive; in recursive mode, sibling roots after budget exhaustion are scanned as empty too. Enabling the deeper-scan flag must not weaken the direct scan: exempt the directly requested root(s) from the byte cap (budget only transitive ingest), and/or keep unread files in the completeness totals as unscanned. - Budget truncation does not fail closed. Semantic analyzers skipped on deadline (
semantic_*,meta_analyzer, TP4) record a truncation reason but nollm_call_record, so_llm_runtime_statussees no failures and the degraded-scan CAUTION floor never engages; combined with (1), a truncated deep scan can still yield a SAFE recommendation. Mirror the existing degraded-LLM behavior: floorrisk_recommendationat CAUTION whenevertransitive_truncatedis true (or when analyzers were budget-skipped), with a regression test.
Non-blocking
finding_fingerprintnow unconditionally appendssource_url/transitive_depth, which changes the hash for every existing baseline entry (root findings now hash...||0) — all previously recorded baselines stop matching after upgrade. Provenance scoping is right for transitive findings, but append the extra fields only whensource_urlis set to keep existing baselines valid._download_with_redirect_validationruns before the budget checks and usesself._remaining_seconds() or 30, so an expired deadline (0.0) becomes a 30 s timeout across up to 5 validation GETs; it also fetches the final URL twice (validation GET + content GET).- Dead code:
_CachedTransitiveResult.bytes_scanned/_estimate_scan_bytesare computed but never read;_URL_PATTERNand_EXCLUDED_PATH_MARKERSintransitive.pyare unused. _scan_skill'stry/except Exception: raiseis a no-op, and_scan_state_with_baselineis a pure pass-through wrapper.- Budgets are hardcoded (32 targets / 10 MB / 60 s) with no CLI override, and the 60 s clock starts before the root scan's own LLM calls —
--transitiveplus LLM will routinely truncate on realistic scans. Consider CLI-configurable budgets or scoping the time budget to transitive work. - An invalid
--transitive-allow-prefix/--transitive-deny-prefixvalue raisesValueErrorfrom_normalize_prefixonly after the root scan has already run; validate prefixes at CLI parse time. - The
transitiveCLI parameter shadows thetransitivemodule import insidescan()(style).
Test coverage for the transitive machinery itself is thorough (traversal sharing, prefix normalization/boundaries, cache reuse, truncation reporting, SARIF provenance).
Signed-off-by: Rod Boev <[email protected]>
cb5ffa1 to
60024ce
Compare
|
The remaining failures came from letting the transitive budget leak into the direct root scan.
|
Summary
Adds opt-in transitive scanning for source references inside scanned skill content while keeping directly requested roots complete, preserving baseline compatibility for direct findings, and surfacing child-budget truncation as incomplete analysis instead of silent coverage loss.
Closes #97
Root cause
The transitive traversal budget was attached too early in the scan pipeline. Direct roots could spend the shared child target, byte, and time budget during input resolution, context loading, graph execution, and recursive entrypoint setup. When a child budget later truncated reads,
build_context()reduced the component inventory to the cached file subset, andreport()treated truncation as display metadata instead of a completeness or verdict input. That combination let incomplete scans still look complete and sometimes report SAFE.Diff Notes
file_cache, filters out adjacent non-source URLs, canonicalizes source identities with dot-segment and unreserved percent-decoding normalization, enforces allow or deny prefix checks on normalized path boundaries, and owns per-root visited-state mutation.--transitive,--transitive-depth, and repeated allow or deny prefix controls toskillspector scan, then route both single-skill and recursive multi-skill entrypoints through a cached traversal helper.file_cachepartial, and compute completeness from discovered components instead of only cached content.InputHandlerfor approved external targets so existing host allowlists, SSRF checks, clone or download handling, and archive protections stay authoritative, and collapse redirect validation plus final streaming into one deadline-aware request path.llm_call_logrecords and merge them into the final report, so degraded transitive semantic calls stay visible even when a dependency result is reused.transitive_depthandsource_urltoFinding, preserve direct baseline fingerprints by adding provenance only for transitive findings, and keep source-aware component coverage so same-named dependency files do not collapse or appear skipped.main.Scope
This change stays on source types already supported by
InputHandler, normalized trust-prefix checks, traversal depth plus hard fan-out limits, allow or deny prefix controls, cache reuse across recursive siblings, and provenance in reports. It does not add a web crawler, new allowed hosts, MCP behavior, or any default behavior change when--transitiveis absent.Verification
pytest tests\\unit\\test_cli.py tests\\unit\\test_transitive.py tests\\unit\\test_input_handler_ssrf.py- pass,71 passedpytest tests\\nodes\\test_analysis_completeness.py tests\\nodes\\test_report.py tests\\nodes\\test_sarif_rules_and_empty_findings.py- pass,86 passedpytest tests\\unit\\test_suppression.py tests\\nodes\\test_llm_analyzer_base.py tests\\unit\\test_llm_utils.py- pass,170 passedruff check src testsruff format --check src tests69 rows, state domains checkedLint & Test (Python 3.12),Lint & Test (Python 3.13), andDCO Check- pending after push