feat: populate ExtraFields with dependency chains, CVE, fixVersion#296
Conversation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high (1 false positive) |
🟢 Metrics 19 complexity · 0 duplication
Metric Results Complexity 19 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request updates the codacy-engine-golang-seed dependency and introduces dependency chain tracking for identified vulnerabilities, adding this information to the issue's extra fields. The review feedback highlights critical issues in the buildDependencyChains implementation, including traversing dependencies in the wrong direction, a key mismatch when looking up packages, and a potential nil pointer dereference. Suggestions are provided to refactor the DFS traversal by passing the full package list directly and building an inverted parent map.
There was a problem hiding this comment.
Pull Request Overview
The PR is currently not up to standards due to critical logic errors and potential runtime panics. While it introduces the structure for populating ExtraFields, the implementation of buildDependencyChains traverses the dependency graph in the wrong direction (following children instead of parents), failing the requirement to show paths from the project root to the vulnerable package. Additionally, there is a significant risk of a nil pointer dereference when looking up packages by PURL.
Most of the acceptance criteria regarding chain limits, truncation, and cycle detection are implemented but remain unverified by tests. The cyclomatic complexity of the DFS implementation also exceeds quality thresholds, suggesting a need for refactoring into more modular, testable components.
About this PR
- The PR lacks specific unit tests for helper functions like
buildDependencyChainsandtrimChainTail. Given the complexity of DFS with cycle detection and truncation, these functions should be tested in isolation with various graph scenarios (e.g., multi-node chains, cycles, and chains exceeding the 20-node limit) before merging. - Variable naming in the traversal loop (e.g., using 'parent' for items in
pkg.DependsOn) is misleading because the code currently treats child dependencies as parents. This should be corrected alongside the traversal logic refactor.
Test suggestions
- Direct dependency results in a single-element chain in ExtraFields
- Transitive dependency chain correctly identifies path from root to vulnerable package
- Limit the number of returned dependency chains to 10
- Truncate dependency chains to 20 nodes while retaining the tail (vulnerable package)
- Prevent infinite recursion when encountering cycles in the dependency graph
- Correct population of CVE and fixVersion when a fix is available
- Unit test for buildDependencyChains coverage
- Unit test for trimChainTail coverage
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Transitive dependency chain correctly identifies path from root to vulnerable package
2. Limit the number of returned dependency chains to 10
3. Truncate dependency chains to 20 nodes while retaining the tail (vulnerable package)
4. Prevent infinite recursion when encountering cycles in the dependency graph
5. Unit test for buildDependencyChains coverage
6. Unit test for trimChainTail coverage
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
There was a problem hiding this comment.
Pull Request Overview
This PR is currently not up to standards due to a combination of performance bottlenecks and edge-case logic flaws in the new dependency chain implementation.
While the core functionality for populating ExtraFields is present, the implementation performs O(N*M) work by rebuilding dependency maps for every vulnerability found. Additionally, there is a logic gap in the cycle-detection fallback: it can exceed the maxDependencyChains limit (10), which contradicts the acceptance criteria. Finally, the complexity of the internal dfs function has increased since the last review, indicating a need for refactoring into smaller, testable units.
About this PR
- The dependency graph traversal logic should be optimized. Currently, the maps used to navigate the graph are rebuilt for every single vulnerability. Since the project's dependency structure is static during the scan, these should be precomputed once.
Test suggestions
- Direct dependency produces a single-element chain
- Transitive dependency produces root-first chains
- Multiple paths to root produce multiple chains (up to 10)
- Chain length capped at 20 nodes, keeping the tail of the chain
- Cycles in dependency graph are handled gracefully without infinite loops
- ExtraFields contains expected JSON keys (CVE, fixVersion, dependenciesChains) in tool output
- Package without PURL falls back to using UID as the node name in the chain
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Address PR review comments on #296: - Precompute pkgByUID and parentsByUID once per result via buildPackageGraph, avoiding O(N*M) reconstruction inside the vulnerability loop. - buildDependencyChains now takes a targetUID + packageGraph; the call site uses vuln.PkgIdentifier.UID (with PURL fallback). - Extract the DFS into a chainBuilder struct with nodeName/tryRecord helpers, dropping the closure's cyclomatic complexity below the threshold. - tryRecord enforces maxDependencyChains for the cycle-fallback branch too, preventing one extra chain past the cap. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
For each vulnerability found by Trivy, emit extraFields containing: - dependenciesChains: up to 10 DFS paths root-first to the vulnerable pkg, each capped at 20 nodes (tail kept); single-element chain = direct dep - CVE: vulnerability ID - fixVersion: least disruptive fix version, or empty string Bumps codacy-engine-golang-seed to v8.0.4 (ExtraFields on Issue). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Three bugs in buildDependencyChains: 1. DependsOn lists children (what a pkg depends on), not parents. Fix: build inverted parentsByUID map (childUID -> []parentUIDs) and DFS upward to roots. 2. pkgByPurl was keyed by PURL but DependsOn contains UIDs — always missed. Fix: key by pkg.Identifier.UID and pass packages slice. 3. Nil dereference: PURL dereferenced before checking ok. Fix: check ok before calling purlPrettyPrint. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Covers: direct dep, single transitive chain, deep chain, multiple paths, target not found, missing PURL fallback to UID, max chains limit (10), chain length trim (20), and cycle termination. Also fixes buildDependencyChains: when all parent branches are blocked by the cycle guard, treat current node as root and emit the chain rather than silently discarding it. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
4c82892 to
be14768
Compare
Trivy's pkg.DependsOn references Package.ID (name@version), while Identifier.UID is an unrelated calculated hash. buildDependencyChains keyed the inverted parent map by UID, so lookups always missed and every chain collapsed to a single element on real scan data. - buildPackageGraph: precompute pkgByID + inverted parentsByID once per result (keyed by Package.ID), fixing both the keying bug and the O(vulns*packages) per-vuln rebuild. - target the vulnerable package by vuln.PkgID (Trivy's graph key). - fold the maxDependencyChains cap into a single record helper. - tests: makePkg now sets Identifier.UID distinct from Package.ID so the suite fails if the graph is keyed by UID; add regression test and realistic PkgID/SBOM PkgID-property fixtures in TestRun. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
codacy-engine-golang-seedtov8.0.4(which addsExtraFields json.RawMessagetoIssue)ExtraFieldswith a JSON object containing:dependenciesChains: up to 10 DFS chains from root to vulnerable package (root-first ordering); each chain capped at 20 nodes (tail kept, vulnerable package last); single-element chain = direct dependencyCVE: vulnerability ID (same asSourceID)fixVersion: least disruptive fix version, or""if none availablebuildDependencyChains+trimChainTailhelpersExample output
{ "extraFields": { "dependenciesChains": [ ["root-app", "@codacy/codacy-mcp", "@modelcontextprotocol/sdk"], ["root-app", "express-rate-limit", "express", "path-to-regexp"] ], "CVE": "CVE-2024-12345", "fixVersion": "1.2.4" } }Part of
Dependency chain propagation initiative — TASK 2 of the SRM dependency chains plan.
Test plan
Tested locally ✅
internal/tooltests pass (updatedTestRunto include expectedExtraFields)TestRunInvalidExecutionConfiguration,TestRunNewRunnerError, etc.) unchanged🤖 Generated with Claude Code