feat(security): add vulnerability intelligence review - #77
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a security-intel subsystem, dependency manifest parsers and OSV scanning, CLI commands for deps/intel, signal emission for CVE/dependency leads, risk scoring and aggregation services, processor/reporting/prompt wiring, tests, and README/CHANGELOG updates. ChangesVulnerability Intelligence & Dependency Scanning
Sequence Diagram(s) sequenceDiagram
participant Caller
participant Service as lookup_cve_bundle
participant Cache as IntelCache
participant Client as IntelHttpClient
participant NVD
participant EPSS
participant KEV
participant GitHub
participant Vendor
Caller->>Service: lookup_cve_bundle(cve_id,data_root,...)
Service->>Cache: get(nvd:cve:{normalized})
Service->>Client: request NVD (if miss)
Client->>NVD: HTTP GET /cves/2.0?cveId=...
Client-->>Service: NVD response
Service->>Client: request EPSS
Client->>EPSS: HTTP GET /epss?cve=...
Client-->>Service: EPSS response
Service->>Client: request KEV
Client->>KEV: HTTP GET KEV feed
Client-->>Service: KEV response
Service->>Client: (opt) GitHub search
Client->>GitHub: repos search q=CVE...
Client-->>Service: GitHub response
Service->>Client: (opt) vendor calls
Client->>Vendor: vendor advisory endpoints
Client-->>Service: vendor responses
Service->>Service: score_cve(...) -> RiskScore
Service-->>Caller: CVEIntelBundle (with models, source_errors, risk)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pythinker-review/src/pythinker_review/cli/security_scan.py`:
- Around line 159-167: The intel_cve command currently lets exceptions from
asyncio.run(lookup_cve_bundle(...)) bubble up; wrap the asyncio.run call in a
try/except that catches expected failures (HTTP/client library errors, pydantic
ValidationError, and a general Exception fallback) and handle them by printing a
concise CLI error via typer.echo (include the error message) and exit with a
non-zero code (raise typer.Exit(1) or sys.exit(1)); apply the same pattern to
the other intel command(s) that call lookup_* functions (e.g., the intel command
block around lines 170-181) and keep use of _data_root and lookup_cve_bundle
unchanged.
In `@packages/pythinker-review/src/pythinker_review/security_intel/client.py`:
- Around line 120-126: The code currently does
int(resp.headers.get("content-length")) which can raise ValueError and bypass
the IntelClientError handling; update the block around content_length, the int
conversion and the subsequent check (referencing resp.headers.get,
content_length, self.max_response_bytes, IntelClientError, resp.read, and
IntelResponse) to defensively handle malformed headers: catch ValueError when
parsing Content-Length and either treat it as absent (skip the size check) or
explicitly raise an IntelClientError with a clear message like "invalid
Content-Length header" so callers always receive IntelClientError instead of a
raw ValueError.
In
`@packages/pythinker-review/src/pythinker_review/security_intel/sources/nvd.py`:
- Around line 61-64: search_cves currently assumes the upstream response is a
dict and calls data.get(...), which will raise if data is not a dict; after the
await client.get_json(...) call in search_cves validate that data is a dict
(e.g., isinstance(data, dict)) and fall back to an empty dict if not, then
extract vulnerabilities = data.get("vulnerabilities", []) only if
vulnerabilities is a list (otherwise treat as []), and finally build records via
CVERecord.model_validate(item.get("cve", {})) for each item in that list so the
function returns a safe empty list instead of raising on non-dict payloads.
In
`@packages/pythinker-review/src/pythinker_review/security_scan/dependencies.py`:
- Around line 250-253: The current _find_line function returns any substring
match which can point at comments or unrelated tokens; update _find_line to
perform a whole-word match and ignore commented lines: read the file via
_read_text(path), iterate lines as before, skip lines where stripped startswith
comment markers (e.g., '#' or '//'), and use a regex search like re.search(r'\b'
+ re.escape(needle) + r'\b', line) to ensure only true token matches are
returned; if no safe match is found return None. Ensure you import re and
preserve the existing signature and use of _read_text.
- Around line 18-21: The current regexes _REQUIREMENT_RE and
_BARE_REQUIREMENT_RE reject package names containing extras like
requests[socks]; update both to accept optional extras in square brackets
(comma-separated identifiers) as part of the name token. Change the name capture
from ([A-Za-z0-9_.\-]+) to something like
([A-Za-z0-9_.\-]+(?:\[[A-Za-z0-9_,]+\])?) in _REQUIREMENT_RE and similarly allow
optional \[...\] in _BARE_REQUIREMENT_RE so entries like requests[socks]==2.31.0
or requests[socks] are matched.
In `@packages/pythinker-review/src/pythinker_review/security_scan/processor.py`:
- Around line 148-152: The code currently recomputes project_info inside each
batch by calling _with_dependency_context(read_info(...)) — move that call into
process_project so project_info is computed once per run (before spawning
workers) and pass the precomputed project_info into the worker/batch-processing
functions instead of recomputing; update any function signatures that currently
call read_info/_with_dependency_context to accept the prebuilt project_info
(refer to process_project, _with_dependency_context, read_info, and the batch
worker entry points) to ensure identical prompt context across concurrent
batches.
In `@packages/pythinker-review/src/pythinker_review/security_scan/reporting.py`:
- Around line 136-138: The report currently appends every entry from
dependency_report.source_errors to lines, which can bloat outputs; update the
code that builds the dependency intel section (the block referencing
dependency_report.source_errors and the lines list) to show only the first N
errors (choose a sensible default like N=10), add a trailing line like "- and X
more errors" when there are additional items, and ensure empty source_errors
still skips the section; use the existing variables
(dependency_report.source_errors, lines) so the change is local and reversible.
In `@packages/pythinker-review/src/pythinker_review/signals/scanner.py`:
- Around line 26-29: The _DEP_LINE_RE currently only matches a few operators and
X.Y versions; update its pattern to also detect other common version operators
and formats so dependency lines aren't missed. Expand the regex in _DEP_LINE_RE
to include operators like >=, <=, !=, >, < in addition to ==, ^, ~, allow
semantic versions with three segments (X.Y.Z) and optional pre-release/build
parts, and also match JSON-style pinned entries (e.g., "pkg": "1.2.3") and bare
numeric pins; keep the word anchors
(dependencies|devDependencies|requires|requirement|package) and
case-insensitivity. Ensure the final pattern still compiles as re.compile and is
assigned to _DEP_LINE_RE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f8896ed-c13c-486f-a5c7-1ee7e8038b9b
📒 Files selected for processing (28)
CHANGELOG.mdpackages/pythinker-review/README.mdpackages/pythinker-review/src/pythinker_review/cli/security_scan.pypackages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.mdpackages/pythinker-review/src/pythinker_review/security_intel/__init__.pypackages/pythinker-review/src/pythinker_review/security_intel/cache.pypackages/pythinker-review/src/pythinker_review/security_intel/client.pypackages/pythinker-review/src/pythinker_review/security_intel/models.pypackages/pythinker-review/src/pythinker_review/security_intel/risk.pypackages/pythinker-review/src/pythinker_review/security_intel/service.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/__init__.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/epss.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/github.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/kev.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/nvd.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/osv.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/vendor.pypackages/pythinker-review/src/pythinker_review/security_intel/validators.pypackages/pythinker-review/src/pythinker_review/security_scan/dependencies.pypackages/pythinker-review/src/pythinker_review/security_scan/processor.pypackages/pythinker-review/src/pythinker_review/security_scan/prompts/system.mdpackages/pythinker-review/src/pythinker_review/security_scan/reporting.pypackages/pythinker-review/src/pythinker_review/signals/advisor.pypackages/pythinker-review/src/pythinker_review/signals/models.pypackages/pythinker-review/src/pythinker_review/signals/scanner.pypackages/pythinker-review/tests/unit/test_security_intel.pypackages/pythinker-review/tests/unit/test_security_scan_dependencies.pypackages/pythinker-review/tests/unit/test_signals.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pythinker-review/tests/unit/test_security_intel.py`:
- Around line 36-46: The current test only exercises _NoRedirectHandler itself;
update test_intel_client_disables_implicit_redirects to instantiate
IntelHttpClient and assert the client's configured opener actually includes an
instance of _NoRedirectHandler (e.g., inspect the client's opener handlers or
registered handlers and assert any(isinstance(h, _NoRedirectHandler) for h in
opener.handlers)). This ensures the security handler is wired by IntelHttpClient
rather than just verifying the helper class alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0653a232-c1b8-40f7-86b0-0e4d01b5aa65
📒 Files selected for processing (2)
packages/pythinker-review/src/pythinker_review/security_intel/client.pypackages/pythinker-review/tests/unit/test_security_intel.py
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pythinker-review/README.md`:
- Line 59: Add a short inline comment next to the "pythinker-security-scan intel
cve CVE-2024-3094" command explaining what the command returns (e.g., that it
fetches intelligence/details related to the specified CVE such as affected
packages, severity, and references) to match the explanatory style used for the
"deps scan" comment; update the README line containing the command so the
comment is concise and consistent with the comment on the preceding "deps scan"
line.
In `@packages/pythinker-review/src/pythinker_review/security_intel/client.py`:
- Around line 110-119: The current call to urllib.request.urlopen(req,
timeout=self.timeout_s) only validates the initial URL via
validate_intel_url(url) and will follow 30x redirects without re-checking
allowlist; fix by creating a custom urllib.request.HTTPRedirectHandler that
overrides redirect_request to call validate_intel_url(newurl) (and raise an
HTTPError or return None to reject the redirect) and then use
urllib.request.build_opener(...your handler...) to open the Request (req)
instead of urlopen; update the code that constructs req and the open call
(referenced symbols: validate_intel_url, urllib.request.Request,
urllib.request.HTTPRedirectHandler, urllib.request.build_opener, req,
self.timeout_s) so every redirected Location is re-validated or redirects are
rejected.
In `@packages/pythinker-review/src/pythinker_review/security_intel/service.py`:
- Around line 57-77: When all enrichment inputs are None you must not synthesize
a LOW risk; before calling score_cve() in the block that builds the
CVEIntelBundle, check whether any scoring source succeeded (e.g. nvd_record,
epss_score (derived from epss_scores), kev_entry, or exploit are not None). If
none succeeded, set risk=None and skip calling score_cve(); otherwise call
score_cve(...) as currently done. Ensure the returned CVEIntelBundle uses that
risk (risk=None when no sources) and still includes source_errors,
vendor_result, and other fields unchanged.
In
`@packages/pythinker-review/src/pythinker_review/security_intel/sources/github.py`:
- Around line 55-63: check_exploit_availability currently assumes the GitHub
search response is a dict with "items" and unconditionally caches empty results;
change the flow so after calling client.get_json(GITHUB_REPO_SEARCH_URL, ...)
you explicitly validate the shape (isinstance(data, dict) and "items" in data
and isinstance(data["items"], list)); if the response is malformed, do not call
cache.set(..., TTL_EXPLOIT) and instead log the error/response (or raise) and
return confidence "NONE" without caching; only proceed to build refs/scores and
cache the result when the validated "items" list is present and processed.
In
`@packages/pythinker-review/src/pythinker_review/security_intel/sources/kev.py`:
- Around line 19-22: The current fetch_kev_catalog caches an empty/malformed
payload because it blindly uses data.get("vulnerabilities", []) before
validating the response shape; update fetch_kev_catalog to first verify that
data is a dict and that data["vulnerabilities"] exists and is a list (e.g.,
isinstance check) and only then build entries with KEVEntry.model_validate,
cache the serialized entries via cache.set("kev:catalog", ..., TTL_KEV) and
return them; if the shape check fails, raise an exception to avoid caching and
allow the caller to try the fallback source.
In
`@packages/pythinker-review/src/pythinker_review/security_intel/sources/nvd.py`:
- Around line 61-64: The current parsing after await client.get_json(NVD_BASE,
params=params, headers=_headers()) assumes shapes and will raise on malformed
responses; validate types before building CVERecord instances: ensure the
fetched value assigned to data is a dict, extract vulnerabilities =
data.get("vulnerabilities") and ensure it is a list, and for each element ensure
item is a dict before calling CVERecord.model_validate(item.get("cve", {})); if
any check fails raise a clear error (or return/raise to keep the fail-closed
behavior) referencing the variables/data and functions (client.get_json,
NVD_BASE, _headers, data, vulnerabilities, item, CVERecord) so readers can
locate the code.
In
`@packages/pythinker-review/src/pythinker_review/security_intel/sources/vendor.py`:
- Line 3: get_vendor_advisory currently awaits vendor lookups sequentially;
replace the tuple-literal awaits for _msrc/_redhat/_ubuntu with a single
asyncio.gather(...) call to run those coroutines concurrently. Also harden
per-item processing in the vendor-specific helpers: in _redhat, before using
release.get(...) inside the loop over affected_release, guard with
isinstance(release, dict) to skip non-dict items; apply the same pattern in
_msrc by checking each entry in value (e.g., if isinstance(item, dict)) before
accessing its keys/values so malformed entries do not raise during enrichment.
- Around line 62-71: The loop in _redhat() assumes data["affected_release"] is a
list of dicts and directly calls release.get(...), which can raise if the field
is malformed; guard by first retrieving affected_release =
data.get("affected_release") and checking isinstance(affected_release, list) (or
defaulting to []), then iterate over affected_release[:20] but skip or coerce
items that are not dicts (e.g., check isinstance(release, dict) before using
release.get) and use safe defaults for each field; update the block that builds
out.append(...) to only access release.get(...) when release is a dict so
malformed inputs won't raise TypeError/AttributeError.
In
`@packages/pythinker-review/src/pythinker_review/security_scan/dependencies.py`:
- Around line 34-49: parse_dependency_manifests currently only checks manifests
at the given root and thus misses manifests in subdirectories; update
parse_dependency_manifests to support recursive discovery (e.g., add optional
params like recurse: bool = False and include/exclude path globs or
allowlist/denylist) so it walks subdirs and calls the existing helpers
(_parse_requirements, _parse_package_json, _parse_pyproject, _parse_pom_xml) for
any discovered manifest files, then returns _dedupe(packages); alternatively, if
you prefer a smaller change, add a clear docstring to parse_dependency_manifests
calling out the root-only limitation and suggesting using a separate recursive
caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 749bb866-a71b-4653-be2e-2adefd0cb352
📒 Files selected for processing (27)
packages/pythinker-review/README.mdpackages/pythinker-review/src/pythinker_review/cli/security_scan.pypackages/pythinker-review/src/pythinker_review/reviewers/prompts/security_review.system.mdpackages/pythinker-review/src/pythinker_review/security_intel/__init__.pypackages/pythinker-review/src/pythinker_review/security_intel/cache.pypackages/pythinker-review/src/pythinker_review/security_intel/client.pypackages/pythinker-review/src/pythinker_review/security_intel/models.pypackages/pythinker-review/src/pythinker_review/security_intel/risk.pypackages/pythinker-review/src/pythinker_review/security_intel/service.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/__init__.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/epss.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/github.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/kev.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/nvd.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/osv.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/vendor.pypackages/pythinker-review/src/pythinker_review/security_intel/validators.pypackages/pythinker-review/src/pythinker_review/security_scan/dependencies.pypackages/pythinker-review/src/pythinker_review/security_scan/processor.pypackages/pythinker-review/src/pythinker_review/security_scan/prompts/system.mdpackages/pythinker-review/src/pythinker_review/security_scan/reporting.pypackages/pythinker-review/src/pythinker_review/signals/advisor.pypackages/pythinker-review/src/pythinker_review/signals/models.pypackages/pythinker-review/src/pythinker_review/signals/scanner.pypackages/pythinker-review/tests/unit/test_security_intel.pypackages/pythinker-review/tests/unit/test_security_scan_dependencies.pypackages/pythinker-review/tests/unit/test_signals.py
…eline - Wrap asyncio.run() calls in intel CLI commands with try/except; print error to stderr and exit 1 on failure (cve + package sub-commands) - Catch ValueError from malformed Content-Length header in IntelHttpClient and raise IntelClientError instead of leaking the raw exception - Skip score_cve() and set risk=None in lookup_cve_bundle when all four sources (NVD, EPSS, KEV, exploit) return None, preventing a synthetic LOW risk from being reported when no data is available - Validate NVD search_cves response is a dict with a list vulnerabilities field before building CVERecord instances; skip non-dict items - Validate KEV catalog response shape before building entries; raise on malformed payload so the fallback URL is tried instead of caching empty - Validate GitHub exploit response shape; return uncached NONE result when the response is not a dict with a list 'items' field; guard individual repo items with isinstance(repo, dict) - Run _msrc/_redhat/_ubuntu concurrently via asyncio.gather in get_vendor_advisory; add isinstance(entry/release, dict) guards in both helpers to skip malformed list items - Update _REQUIREMENT_RE and _BARE_REQUIREMENT_RE to accept package extras (e.g. requests[socks]==2.31.0) - Update _find_line to skip comment lines and use whole-word regex match to avoid false matches on comments or unrelated tokens - Compute project_info once before spawning batch workers in process_project to ensure identical prompt context across concurrent runs - Cap source_errors at 10 in markdown report; append "and N more errors" when truncated - Expand _DEP_LINE_RE to cover >=, <=, !=, ~=, >, < operators, three-part semver, and JSON-style pinned entries - Extend redirect-handler test to also assert IntelHttpClient wires _NoRedirectHandler into its opener - Add docstring to parse_dependency_manifests noting root-only limitation - Add inline comment to README intel cve example
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/pythinker-review/src/pythinker_review/security_scan/dependencies.py`:
- Around line 19-21: The regex _REQUIREMENT_RE currently leaves extras (e.g.
"requests[socks]") in match.group(1) and _parse_requirements() forwards that
into PackageRef(name=...), causing validation/OSV lookups to skip the package;
update _parse_requirements() to strip extras when building PackageRef (e.g.
derive base_name = match.group(1).split("[",1)[0] or prefer using
_BARE_REQUIREMENT_RE to capture the plain name) and pass base_name into
PackageRef(name=base_name); add/extend unit test in
tests/unit/test_security_scan_dependencies.py asserting that
"requests[socks]==..." yields PackageRef.name == "requests" so OSV querying is
not skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74982212-c758-4fef-8742-749fdcfe574c
📒 Files selected for processing (14)
CHANGELOG.mdpackages/pythinker-review/README.mdpackages/pythinker-review/src/pythinker_review/cli/security_scan.pypackages/pythinker-review/src/pythinker_review/security_intel/client.pypackages/pythinker-review/src/pythinker_review/security_intel/service.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/github.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/kev.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/nvd.pypackages/pythinker-review/src/pythinker_review/security_intel/sources/vendor.pypackages/pythinker-review/src/pythinker_review/security_scan/dependencies.pypackages/pythinker-review/src/pythinker_review/security_scan/processor.pypackages/pythinker-review/src/pythinker_review/security_scan/reporting.pypackages/pythinker-review/src/pythinker_review/signals/scanner.pypackages/pythinker-review/tests/unit/test_security_intel.py
…ement names - Break long asyncio.run() call lines in intel_cve and intel_package to stay under the 100-char limit (E501) - Add `from exc` to all three bare `raise` statements inside except blocks in cli/security_scan.py and client.py (B904) - Strip package extras (e.g. requests[socks]) from the name in _parse_requirements before constructing PackageRef so OSV lookups receive a plain package name; bare requirements use the same stripping - Add test asserting requests[socks]==2.31.0 and urllib3[secure] parse to PackageRef.name without brackets
Summary
security_intelpackage for CVE/package vulnerability enrichmentpythinker security-scanVerification
uv run --directory packages/pythinker-review ruff check src testsuv run --directory packages/pythinker-review ruff format --check src testsuv run --directory packages/pythinker-review pyright src tests/unit/test_security_intel.py tests/unit/test_security_scan_dependencies.py tests/unit/test_signals.pymake test-pythinker-review(168 passed, 3 skipped)Notes
make check-pythinker-reviewpasses ruff/format/pyright, then prints existing non-blockingtydiagnostics in unrelated files.Summary by CodeRabbit
New Features
Documentation
Tests