Skip to content

feat(scan): add opt-in transitive reference scanning - #225

Open
rodboev wants to merge 1 commit into
NVIDIA:mainfrom
rodboev:pr/transitive-external-reference-scanning
Open

feat(scan): add opt-in transitive reference scanning#225
rodboev wants to merge 1 commit into
NVIDIA:mainfrom
rodboev:pr/transitive-external-reference-scanning

Conversation

@rodboev

@rodboev rodboev commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Rebuilds the opt-in transitive scanning feature onto current main while preserving the resolved review fixes and current state, input, analyzer, and report contracts.

Closes #97

Root cause

The previous head was based on a pre-ledger state shape and stale provider call contracts. Rebasing it mechanically conflicted with current ownership of inspection completeness, effective finding selection, LLM telemetry, secure input handling, and report rendering. The branch also retained the current-head semantic analyzer failures from that integration drift.

Diff Notes

  • Port the transitive traversal through current SkillspectorState, inspection_ledger, analyzer_status_events, effective_finding_ids, analysis_completeness, execution_successful, and llm_call_log contracts.
  • Namespace and merge child ledger/status/completeness facts so coverage and execution status describe root and dependency components together.
  • Keep --transitive opt-in, with canonical source identities, normalized allow or deny prefixes, per-root visited sets, shared child-result caching, depth limits, target limits, byte limits, and time limits.
  • Keep direct root scans outside the child traversal budget. Child input uses the existing InputHandler host, SSRF, redirect, clone, archive, and secure-file boundaries.
  • Keep child failures, incomplete child reports, and traversal truncation visible through the current completeness path, and floor an otherwise SAFE result to CAUTION without changing the score or severity.
  • Keep the shared deadline scoped to child work, continue after an individual child failure, and account for child file reads against the byte budget while retaining all inventoried components.
  • Preserve effective child findings when merging graph results, including source-aware component coverage, finding provenance, cached LLM telemetry, and baseline suppression for repeated dependency paths.
  • Surface dependency source and depth in JSON, Markdown, and SARIF output, while keeping default scans, MCP behavior, and existing direct baseline fingerprints unchanged.
  • Add focused regressions for current analyzer/provider compatibility, child finding selection, input budgets and redirects, source-aware suppression, provenance, cache reuse, and fail-closed reports.

Scope

The change remains limited to source types already supported by InputHandler, canonical trust-prefix handling, opt-in traversal, bounded child scanning, provenance, and fail-closed reporting. It does not add new hosts, a web crawler, MCP traversal, or default behavior changes when --transitive is absent.

Verification

  • Focused CLI, transitive, input-budget/SSRF, LLM utility, report, completeness, and analyzer-base suites, 363 passed
  • Focused suppression, deduplication, CLI, transitive, input-budget/SSRF, and report suites, 245 passed
  • Focused semantic developer-intent, quality-policy, security-discovery, and MCP poisoning suites, 136 passed, 11 deselected
  • uv run ruff check src/ tests/
  • uv run ruff format --check src/ tests/
  • git diff --check
  • Invariant enumeration gate, 222 rows, state domains checked
  • GitHub CI, pending after push

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/skillspector/transitive.py Outdated
Comment thread src/skillspector/cli.py
Comment thread src/skillspector/cli.py Outdated
Comment thread src/skillspector/cli.py
Comment thread tests/nodes/test_report.py Outdated
@rodboev
rodboev force-pushed the pr/transitive-external-reference-scanning branch from 47dfb07 to 4fb451c Compare June 30, 2026 11:15
@rodboev

rodboev commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

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 main.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/skillspector/cli.py
Comment thread src/skillspector/cli.py
Comment thread src/skillspector/cli.py
Comment thread src/skillspector/cli.py Outdated
@rodboev
rodboev force-pushed the pr/transitive-external-reference-scanning branch from 5dc441a to cb5ffa1 Compare July 9, 2026 18:16
@rodboev

rodboev commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main as cb5ffa1. The update preserves the review-thread fixes already described above and removes the remaining merge conflict.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

  1. Root-scan byte-budget truncation silently zeroes coverage. When --transitive is set, the root skill's file reads consume the shared 10 MB budget (build_context._read_file_cache breaks on remaining_bytes), and every unread file is dropped from components/component_metadata entirely. _build_analysis_completeness then computes totals from component_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.
  2. Budget truncation does not fail closed. Semantic analyzers skipped on deadline (semantic_*, meta_analyzer, TP4) record a truncation reason but no llm_call_record, so _llm_runtime_status sees 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: floor risk_recommendation at CAUTION whenever transitive_truncated is true (or when analyzers were budget-skipped), with a regression test.

Non-blocking

  • finding_fingerprint now unconditionally appends source_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 when source_url is set to keep existing baselines valid.
  • _download_with_redirect_validation runs before the budget checks and uses self._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_bytes are computed but never read; _URL_PATTERN and _EXCLUDED_PATH_MARKERS in transitive.py are unused.
  • _scan_skill's try/except Exception: raise is a no-op, and _scan_state_with_baseline is 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 — --transitive plus 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-prefix value raises ValueError from _normalize_prefix only after the root scan has already run; validate prefixes at CLI parse time.
  • The transitive CLI parameter shadows the transitive module import inside scan() (style).

Test coverage for the transitive machinery itself is thorough (traversal sharing, prefix normalization/boundaries, cache reuse, truncation reporting, SARIF provenance).

Comment thread src/skillspector/nodes/build_context.py
Comment thread src/skillspector/nodes/report.py Outdated
Comment thread src/skillspector/cli.py
Comment thread src/skillspector/suppression.py Outdated
Comment thread src/skillspector/input_handler.py Outdated
Comment thread src/skillspector/cli.py Outdated
@rodboev
rodboev force-pushed the pr/transitive-external-reference-scanning branch from cb5ffa1 to 60024ce Compare July 14, 2026 14:28
@rodboev

rodboev commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

The remaining failures came from letting the transitive budget leak into the direct root scan.

  1. Directly requested roots now run without traversal budget state. The shared target, byte, and time limits start with the first transitive child scan and stay shared across recursive roots, so --transitive no longer reduces root coverage.

  2. build_context() now keeps the full discovered component list even when a child hits the byte cap. file_cache still stays partial, so unread files show up as skipped instead of disappearing from coverage.

  3. Any traversal truncation now marks the scan incomplete and lifts SAFE to CAUTION, even when the skipped analyzer never ran long enough to record its own LLM call.

  4. Direct finding fingerprints stay backward compatible. Source and depth are added only for transitive findings, and redirect validation now stops immediately on an exhausted deadline while streaming the final response once.

  5. The dead cached-byte plumbing and other unused helpers are gone, and allow or deny prefixes are validated before the root scan starts.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Re-review of head 60024ce. The two blockers from the previous review are resolved: direct/root scans no longer consume the child traversal budget, unread transitive files remain represented in completeness metadata, and any recorded budget truncation now floors a SAFE recommendation to CAUTION. The baseline fingerprint compatibility concern was also fixed by adding provenance only for transitive findings.

Blocker

  1. A transitive child-scan exception still fails open. _scan_transitive() catches every exception from _run_graph_scan() and only logs a warning. The failed target is omitted from findings, components, sources, and truncation reasons, so JSON output can still report 100% completeness and SAFE after a referenced dependency fails to download, resolve, parse, or scan. For a security scanner, an unscanned requested dependency must be surfaced as incomplete/degraded and must trigger the same CAUTION floor as budget truncation. Record the failure on the traversal (without leaking secrets from exception text), expose it in completeness metadata, and add a regression where _run_graph_scan raises and the final report is incomplete/CAUTION.

No other new correctness or security blockers found in the post-review changes.

Comment thread src/skillspector/cli.py Outdated
rng1995
rng1995 previously approved these changes Jul 31, 2026

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Re-review: approved. The remaining fail-open path is resolved. A transitive child exception now keeps the target in transitive_sources, records a sanitized traversal limitation without exception text, marks analysis incomplete/truncated, and floors an otherwise SAFE result to CAUTION. The dedicated child-failure regression covers visibility, completeness, recommendation, and secret non-disclosure. The transitive/CLI/report/LLM suites passed; four pre-existing DNS-dependent SSRF tests could not resolve public hosts in this restricted sandbox and were not failures in the changed traversal logic.

@rng1995

rng1995 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@rodboev - Please address review comments and resolve merge conflicts.

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev
rodboev force-pushed the pr/transitive-external-reference-scanning branch from 5eb519d to a4e317b Compare August 15, 2026 20:24
@rodboev

rodboev commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt the transitive-scanning branch onto current main at a4e317b. The update preserves the resolved review fixes and integrates current ledger, analyzer-status, effective-finding, input-budget, provider-timeout, suppression, and report contracts. Child failures remain visible and fail closed, dependency provenance is preserved across report formats, and the focused suites are green. All existing review threads remain resolved; the updated head is ready for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: transitive link scanning — follow external repos/URLs referenced inside skill files

2 participants