Repository navigation
Conversation
Reviewer's GuideRecommendation status resolution is now isolated per winner to prevent vulnerability metadata leaking between version streams, with a direct database fallback for synthetic advisory-derived winners lacking versioned_purl rows. New tests cover both multi-stream isolation and advisory-only vulnerability restoration. Sequence diagram for per-winner vulnerability status resolutionsequenceDiagram
participant Recommend as recommend_purls()
participant PurlService
participant DB as Database
loop Each recommendation winner
Recommend->>PurlService: fetch_vulnerability_statuses(base_purl_id, versioned_purl_id)
PurlService->>DB: Query statuses joined to versioned_purl
DB-->>PurlService: StatusInfo for this winner
alt No statuses and synthetic winner
PurlService->>PurlService: Uuid::new_v5(base_purl_id, version)
PurlService->>PurlService: fetch_vulnerability_statuses_by_version(base_purl_id, version)
PurlService->>DB: Query purl_status by base PURL and version range
DB-->>PurlService: StatusInfo for synthetic winner
end
end
Recommend->>PurlService: assemble_recommend_entry(winner, statuses_by_vp)
PurlService-->>Recommend: Recommendation with isolated vulnerability metadata
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/fundamental/src/purl/service/mod.rs" line_range="646-653" />
<code_context>
+ // winner. Isolating each winner to its own query ensures version-range matching is
+ // scoped to that winner's specific versioned PURL.
+ let mut statuses_by_vp: HashMap<Uuid, Vec<StatusInfo>> = HashMap::new();
+ for winner in &winners {
+ let per_winner = Self::fetch_vulnerability_statuses(
+ std::iter::once(winner.base.id),
+ std::iter::once(winner.versioned_purl.id),
+ connection,
+ )
+ .await?;
+ if let Some(infos) = per_winner.into_values().next() {
+ statuses_by_vp.insert(winner.versioned_purl.id, infos);
+ } else {
</code_context>
<issue_to_address>
**Bulk recommendations stall or mix statuses**
When a request produces many winners, or an advisory update commits between winner fetches, `recommend_purls` calls `fetch_vulnerability_statuses` serially per winner, repeating status and association queries. Large requests hold database capacity and time out, while read-committed statements can return statuses from different advisory states in one response.
Fetch statuses for all winners in one batched operation under a consistent database snapshot.
</issue_to_address>
### Comment 2
<location path="modules/fundamental/src/purl/endpoints/test.rs" line_range="455-459" />
<code_context>
+
+ // And each recommendation has non-empty vulnerability metadata
+ assert!(
+ !rec_1[0]["vulnerabilities"].as_array().unwrap().is_empty(),
+ "stream 1 should have vulnerability metadata"
+ );
+ assert!(
+ !rec_2[0]["vulnerabilities"].as_array().unwrap().is_empty(),
+ "stream 2 should have vulnerability metadata"
+ );
</code_context>
<issue_to_address>
**Wrong stream statuses pass**
When both version streams’ status rows are returned in one base-PURL batch, `get_recommendations_multi_stream` lines 455–460 only require non-empty vulnerability arrays. Since both fixtures use the same CVE with `fixed` status (lines 411–412 and 421–422), a batched lookup that assigns one stream’s status to the other still passes the test.
Give the two streams distinguishable statuses and assert the expected status for each recommendation.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if version-range matching selects the wrong advisory status, the recommendation API can label a vendor rebuild incorrectly—for example, appearing fixed when it is still affected—and users may make remediation decisions based on that result. Reverting stops future incorrect responses, but cannot undo decisions already made.
Blocking findings: modules/fundamental/src/purl/service/mod.rs:653, modules/fundamental/src/purl/endpoints/test.rs:459
…commend winners Advisory-synthesized winners (from extend_with_advisory_versions) have deterministic v5 UUIDs with no backing versioned_purl row. The fetch_vulnerability_statuses JOIN on versioned_purl excluded them, producing empty vulnerability arrays even when purl_status data existed. Add fetch_vulnerability_statuses_by_version fallback that queries purl_status directly by base_purl_id and version string, bypassing the versioned_purl JOIN. Synthetic winners are detected via v5 UUID comparison in the per-winner loop. Implements TC-6902 Assisted-by: Claude Code
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/fundamental/src/purl/service/mod.rs" line_range="659-664" />
<code_context>
+ let is_synthetic = winner.versioned_purl.id
+ == Uuid::new_v5(&winner.base.id, winner.versioned_purl.version.as_bytes());
+ if is_synthetic {
+ let infos = Self::fetch_vulnerability_statuses_by_version(
+ winner.base.id,
+ &winner.versioned_purl.version,
+ connection,
+ )
+ .await?;
+ if !infos.is_empty() {
+ statuses_by_vp.insert(winner.versioned_purl.id, infos);
</code_context>
<issue_to_address>
**Advisory-only recommendations disappear**
When an SBOM package's winning vendor version is synthesized from an advisory and has no backing `versioned_purl` row, `report_for_sboms` calls only `fetch_vulnerability_statuses`, which cannot resolve the synthetic ID, so the status list stays empty and the report skips the package and its vulnerability counts.
Route synthetic winners through the advisory-derived status lookup instead of relying only on `fetch_vulnerability_statuses`.
</issue_to_address>
### Comment 2
<location path="modules/fundamental/src/purl/endpoints/test.rs" line_range="487-488" />
<code_context>
+ use sea_orm::ConnectionTrait;
+ ctx.db
+ .execute_unprepared(
+ "DELETE FROM versioned_purl WHERE version = '1.0.0.redhat-00001' \
+ AND base_purl_id = (SELECT id FROM base_purl WHERE name = 'advlib')",
+ )
+ .await?;
</code_context>
<issue_to_address>
**Test deletes unrelated package versions**
When the test runs against an external database containing another base PURL named `advlib` with version `1.0.0.redhat-00001`, the DELETE subquery matches only `base_purl.name`; when another package with that name has the same version, the test deletes its version row too and leaves the external test database modified.
Constrain the subquery by the full PURL identity and restore the deleted row or run the test against an isolated database.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: modules/fundamental/src/purl/service/mod.rs:664, modules/fundamental/src/purl/endpoints/test.rs:488
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review (5505884499) — Classified as nit — summary of inline comments already classified individually (4260515881: suggestion, 4260515908: code change request addressed). No sub-task created. [sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review (5544849852) — Classified as nit — summary of inline comments already classified individually (4292342724: suggestion, 4292342739: suggestion). No sub-task created. |
Verification Report for TC-6902 (commit e36fe08)
Overall: WARNCI checks still pending (bench, windows-2025, test). Compilation and clippy pass on both macOS and Ubuntu. All 5 acceptance criteria verified against the code:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.10. |
mrizzi
left a comment
mrizzi
left a comment
There was a problem hiding this comment.
These comments don't repeat the existing sourcery threads. Comments on is_synthetic and the by-version query are about an assumption in the TC-6902 spec itself, and the PR implements that spec faithfully. The rest are CONVENTIONS.md items, and each names its section.
| if let Some(infos) = per_winner.into_values().next() { | ||
| statuses_by_vp.insert(winner.versioned_purl.id, infos); | ||
| } else { | ||
| let is_synthetic = winner.versioned_purl.id |
There was a problem hiding this comment.
This matches the TC-6902 spec, but the spec's assumption doesn't hold. Real versioned_purl rows are also created as Uuid::new_v5(package_uuid, effective_version) (purl.version_uuid(), modules/ingestor/src/graph/purl/mod.rs:267 → common/src/purl.rs:130-132), and winner.base.id is package_uuid. So for non-epoch purls, is_synthetic is true for every winner. The new test shows this: after it deletes the real row, the synthetic winner from extend_with_advisory_versions has the same ID that row had.
The results are still correct, because for a real winner with no statuses the fallback also returns nothing. But every real "clean" winner pays for a second query that can't return anything, synthetic winners always run a first query that can't match, and the name and comments promise a distinction the code doesn't make. If it stays, please at least document that it is a "statuses were empty" gate, not real synthetic detection.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — UUIDv5 equality is also used for persisted version rows, so it does not uniquely establish that a winner is synthetic. Sub-task TC-7121 created to resolve and document the fallback eligibility.
| /// literal against `version_range`, bypassing the `versioned_purl` JOIN that fails for | ||
| /// advisory-synthesized winners. | ||
| #[instrument(skip_all, err(level = tracing::Level::INFO))] | ||
| async fn fetch_vulnerability_statuses_by_version<C: ConnectionTrait>( |
There was a problem hiding this comment.
Non-blocking, possibly a follow-up: running fetch_vulnerability_statuses_by_version for every winner gives the same results for real rows (same VersionMatches filter, and the version is already in winner.versioned_purl.version). It would remove the detection branch and the double query, and report_for_sboms could reuse it. It keeps the per-winner isolation from TC-6899. I realise this departs from acceptance criteria 2 and 3 as written, so it may be better tracked separately.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — querying every winner by version may avoid the UUID branch, but it broadens the change beyond the stated acceptance criteria and is framed as a possible follow-up. No sub-task created.
| .instrument(info_span!("querying purl statuses for synthetic winner")) | ||
| .await?; | ||
|
|
||
| let vulns = all_statuses |
There was a problem hiding this comment.
CONVENTIONS.md § Code Duplication
Lines 806-846 copy about 40 lines of fetch_vulnerability_statuses (lines 721-770): the four loaders, the slug map, the four-way zip, and building each StatusInfo. The task's "Files to Modify" already anticipates "possibly extract a new helper function", and the Reuse Candidates list suggests adapting fetch_vulnerability_statuses rather than copying it. CONVENTIONS.md § Code Duplication asks to consolidate blocks longer than ~20 lines. Please pull this into a shared helper, for example load_status_infos(statuses: Vec<purl_status::Model>, connection) -> Result<Vec<(Uuid, StatusInfo)>, Error>, so that changes to StatusInfo (such as advisory_date precedence or slug defaults) only happen in one place.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — this matches CONVENTIONS.md § Code Duplication; the new status mapping duplicates the existing loader logic. Sub-task TC-7122 created to extract shared mapping.
| .await?; | ||
|
|
||
| let vulns = all_statuses | ||
| .load_one(vulnerability::Entity, connection) |
There was a problem hiding this comment.
CONVENTIONS.md § Tracing spans on sub-operations
The load_one calls (vulnerability, advisory, status) and the load_many_to_many call (remediations) at lines 806-822 have no .instrument(info_span!(...)). The original function wraps the same calls (lines 724-745: "loading vulnerabilities", "loading advisories", …). Under § Tracing spans on sub-operations, SeaORM loaders have no built-in instrumentation and should be wrapped. Right now a slow advisory-only request shows one opaque span. A shared helper (see the Code Duplication comment) would fix this too.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — this matches CONVENTIONS.md § Tracing spans on sub-operations for the SeaORM association loaders. Sub-task TC-7125 created to add the spans.
| "pkg:maven/com.example/advlib@1.0.0.redhat-00001" | ||
| ); | ||
| assert!( | ||
| !rec[0]["vulnerabilities"].as_array().unwrap().is_empty(), |
There was a problem hiding this comment.
This meets the criterion literally, but bug TC-6887's Expected Result is "vulnerability metadata (CVE, status, remediation)". A wrong version-range match, a status from another advisory, or a slug that falls back to unknown would all still pass. This is the same weakness fixed in get_recommendations_multi_stream (e36fe08). Please assert CVE-2099-20001 with status fixed.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — a non-empty vulnerability array can still contain the wrong CVE or status. Sub-task TC-7123 created to assert the expected CVE and fixed status.
| .left_join(version_range::Entity) | ||
| .filter(purl_status::Column::BasePurlId.eq(base_purl_id)) | ||
| .filter(SimpleExpr::FunctionCall( | ||
| Func::cust(trustify_common::db::VersionMatches) |
There was a problem hiding this comment.
A few small CONVENTIONS.md items in the new function:
- line 798 (§ Import Style):
trustify_common::db::VersionMatchesshould be imported in the existinguse trustify_common::{db::{...}}block. Line 714 has the same problem, so it's worth fixing both. - line 811 (§ Type inference):
let status_slug_map: HashMap<_, _> = ….collect()→.collect::<HashMap<_, _>>(). Line 734 has the same pattern. - line 824 (§ Pre-allocating collections):
Vec::new()→Vec::with_capacity(all_statuses.len()), taking the length before thezipconsumesall_statuses. - line 841 (§ Iterator ownership):
advisoryis owned here (the loop consumesadvisories_loaded), soadvisory.identifiercan be moved instead of.clone()d.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — the cited import, type inference, preallocation, and iterator ownership practices are documented in CONVENTIONS.md. Sub-task TC-7124 created to apply them.
| .await?; | ||
|
|
||
| // And the real versioned_purl row is removed, forcing the synthetic path | ||
| use sea_orm::ConnectionTrait; |
There was a problem hiding this comment.
CONVENTIONS.md § Logging framework, § Import Nesting
- line 497 (§ Logging framework): new code should use
tracing::rather thanlog::info!. The convention also asks to migrate a file'slog::calls when you touch it. This file already usestracing::debug!elsewhere. - line 485 (§ Import Nesting / § Import Style): move
use sea_orm::ConnectionTrait;from inside the function body into the file's import block at the top.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — this matches CONVENTIONS.md § Logging framework and § Import Nesting. Sub-task TC-7126 created to update the logging and import style.
|
[sdlc-workflow/verify-pr] Re: @mrizzi review — Classified as suggestion — this review summarizes the spec-assumption concern and convention-backed inline feedback; the actionable items are tracked in the inline threads. No separate sub-task created. |
Verification Report for TC-6902 (commit dd8b97e)
Overall: FAILThe UUIDv5 predicate in The advisory-only reproducer passes but asserts only non-empty vulnerability metadata; exact CVE/status assertions are tracked in TC-7123. The local precommit result is user-confirmed; this session earlier lacked the Podman socket configuration. Review follow-ups: TC-7121 (fallback eligibility), TC-7122 (shared status mapping), TC-7123 (exact test assertions), TC-7124 (service conventions), TC-7125 (loader instrumentation), TC-7126 (test conventions). Root-cause follow-ups: TC-7127 (semantic-value assertions), TC-7128 (conventions audit), TC-7129 (validate predicates against target and control paths). This comment was AI-generated by sdlc-workflow/verify-pr v0.13.10. |
Track synthesized version IDs explicitly because persisted rows use the same UUIDv5 scheme. Keep advisory-only fallback status lookup without querying for persisted winners. Implements TC-7121 Assisted-by: Claude Code
Summary
versioned_purlrowfetch_vulnerability_statuses_by_version()queriespurl_statusdirectly by base PURL ID + version string, bypassing theversioned_purlJOIN that excluded synthetic winnersUuid::new_v5comparison in the per-winner loopextend_with_advisory_versionsNote: This PR includes TC-6899's commit (per-winner isolation fix) as a prerequisite — rebase onto main after #2761 merges.
Test plan
get_recommendations_advisory_onlytest passes (deletes real versioned_purl row to force synthetic path, asserts non-empty vulnerabilities)get_recommendations*tests pass (including TC-6899'sget_recommendations_multi_stream)recommend_report*tests passcargo clippyclean,cargo fmtcleanImplements TC-6902
🤖 Generated with Claude Code
Summary by Sourcery
Resolve vulnerability statuses for advisory-synthesized recommendation winners.
Bug Fixes:
Enhancements:
Tests: