fix(sc4): surface OSV.dev fallback warnings and add configurable timeout#120
Conversation
Three changes to improve SC4 (Known Vulnerable Dependencies) reliability: 1. Configurable timeout: Read SKILLSPECTOR_OSV_TIMEOUT env var (default 30s, was hardcoded 10s) so users in high-latency environments can increase it. 2. Increased default timeouts: Raised query timeout from 10s to 30s and is_available() check from 5s to 15s to reduce silent fallback rate. 3. Visible fallback warning: When OSV.dev is unreachable and static fallback finds nothing, emit a LOW-severity SC4 finding alerting users that results may be incomplete. Previously the fallback was only visible in --verbose logs. 4. Distinguish clean packages from failed lookups: Added INFO log when OSV.dev returns no vulnerabilities for a package (was silently cached). Added was_osv_reachable() helper so callers can detect API failures vs clean results. Fixes NVIDIA#102 Signed-off-by: Perseus Computing <51974392+tcconnally@users.noreply.github.com>
rng1995
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — the new fallback-warning feature does not actually fire, because the module-level _last_query_ok flag is never updated.
query_batch() assigns _last_query_ok = True / _last_query_ok = False without a global _last_query_ok declaration, so those assignments create function-local variables. The module-level _last_query_ok stays at its initial True, so was_osv_reachable() always returns True, and the new branch in static_patterns_supply_chain.py:
elif uncovered_packages and not osv_findings and not was_osv_reachable():is never taken — the "OSV.dev unreachable" warning will never be surfaced, even when the API actually fails. (Python turns any assignment in a function into a local binding unless global/nonlocal is declared, so there is no error, just a silent no-op.)
Required:
- Add
global _last_query_okat the top ofquery_batch()(or refactor the flag into a small mutable container or a return value) so the success/failure state is actually persisted. - Add a unit test that simulates an OSV failure and asserts
was_osv_reachable()returnsFalseafterward (and that the SC4 fallback warning is emitted). That would have caught this.
Other (non-blocking) notes:
float(os.environ.get("SKILLSPECTOR_OSV_TIMEOUT", 30.0))will raiseValueErrorat import time if the env var is set to a non-numeric value; consider validating/defaulting gracefully.- The fallback message hardcodes "(24 packages)"; if that count drifts from the actual static DB size it will be misleading — consider deriving it.
The raised timeouts and the is_available() change themselves look fine.
…rive fallback count Addresses review feedback from rng1995 on NVIDIA#120: 1. CRITICAL: Add to query_batch() so that assignments inside the function actually modify the module-level variable. Without this, Python creates function-local variables and was_osv_reachable() always returns True — the OSV.dev unreachable warning in static_patterns_supply_chain.py never fires. 2. Validate SKILLSPECTOR_OSV_TIMEOUT env var gracefully — catch ValueError on non-numeric values and log a warning instead of crashing at import time. 3. Derive fallback package count from len(fallback_db) instead of hardcoding (24 packages) — stays accurate when the static DBs are updated. Added unit tests: - test_was_osv_reachable_after_success (True after successful query) - test_was_osv_reachable_after_failure (False after failed query) - Patched _analyze_deps test helper to mock was_osv_reachable() to avoid test ordering fragility
|
Thanks for the thorough review, @rng1995! All three issues addressed: 1.
|
rng1995
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit and verified all three requested changes are now in place against the current head:
- The critical fix:
global _last_query_okis now declared insidequery_batch(), sowas_osv_reachable()correctly reflects the last query outcome instead of being a no-op. - The
SKILLSPECTOR_OSV_TIMEOUTparse is wrapped in try/except, so a non-numeric value logs a warning and falls back to the default instead of crashing at import time. - The fallback warning now derives the package count from
len(fallback_db)rather than a hardcoded number.
Also confirmed new unit tests assert was_osv_reachable() is True after a successful query and False after a failed one.
This approval supersedes my earlier change request. Thanks for the thorough follow-up!
Summary
Fixes #102: SC4 (Known Vulnerable Dependencies) silently falls back to a small static list when api.osv.dev is unreachable.
Changes
Testing
All 621 unit tests pass.