Conversation
…bom_ai.properties Extract structured fields from CycloneDX ModelCard into sbom_ai.properties as frontend-friendly keys: model_parameters.task → primaryPurpose, model_parameters.approach.type → typeOfModel, and considerations.technical_limitations → limitation (joined with "; "). Generic model_card.properties overlay on top, preserving precedence. Implements TC-4516 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tions Extract 5 additional CycloneDX 1.6 ModelCardConsiderations fields into sbom_ai.properties during AIBOM ingestion: ethicalConsiderations as safetyRiskAssessment (JSON array), fairnessAssessments (JSON array), performanceTradeoffs, useCases, and users (joined strings). Implements TC-4796 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Assisted-by: Claude Code
Contributor
Reviewer's GuideExtends AI CycloneDX AIBOM ingestion to derive additional structured model card considerations into sbom_ai.properties, while ensuring generic model card properties override structured fields, and adds targeted tests/fixtures to validate precedence and new mappings. Flow diagram for building ModelCard properties from CycloneDX model_cardflowchart TD
A[Component]
A --> B[model_card]
B --> C[model_parameters]
C --> C1[primaryPurpose from task]
C --> C2[typeOfModel from approach.type_]
B --> D[considerations]
D --> D1[limitation from technical_limitations join]
D --> D2[safetyRiskAssessment from ethical_considerations]
D --> D3[fairnessAssessments from fairness_assessments]
D --> D4[performanceTradeoffs from performance_tradeoffs join]
D --> D5[useCases from use_cases join]
D --> D6[users from users join]
B --> E[properties]
E -->|overrides structured fields| F[final properties map]
C1 --> F
C2 --> F
D1 --> F
D2 --> F
D3 --> F
D4 --> F
D5 --> F
D6 --> F
F --> G[ModelCard.properties in sbom_ai.Entity]
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
ModelCardconversion now returnsValue::Nullfor cards that only have an emptypropertieslist (and no structured fields), whereas previously this resulted in an empty object; consider whether this behavioral change is intentional and, if not, preserve the prior semantics for emptyproperties. - The structured-field-to-
propertiesmapping logic inimpl From<&Component> for ModelCardhas grown quite large; consider extracting this into a dedicated helper (e.g.,fn build_properties(card: &ModelCardType) -> Map<String, Value>) to improve readability and make the mapping easier to unit-test in isolation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ModelCard` conversion now returns `Value::Null` for cards that only have an empty `properties` list (and no structured fields), whereas previously this resulted in an empty object; consider whether this behavioral change is intentional and, if not, preserve the prior semantics for empty `properties`.
- The structured-field-to-`properties` mapping logic in `impl From<&Component> for ModelCard` has grown quite large; consider extracting this into a dedicated helper (e.g., `fn build_properties(card: &ModelCardType) -> Map<String, Value>`) to improve readability and make the mapping easier to unit-test in isolation.
## Individual Comments
### Comment 1
<location path="modules/ingestor/src/graph/sbom/common/machine_learning_model.rs" line_range="124-125" />
<code_context>
+ }
+ }
+
+ if map.is_empty() {
+ Value::Null
+ } else {
+ Value::Object(map)
</code_context>
<issue_to_address>
**issue (bug_risk):** Revisit behavior change for model cards with only empty generic properties
Previously, an empty `card.properties` vector resulted in `Some(Map::new())` and thus `Value::Object({})`. With the new check, the same case now yields `Value::Null`. This changes observable behavior for model cards that exist but have no effective properties and may break callers that distinguish between an empty object and `null`. If that distinction matters, consider still returning `Value::Object(map)` when `card.properties` was `Some(_)`, even if the map is empty.
</issue_to_address>
### Comment 2
<location path="modules/ingestor/src/graph/sbom/common/machine_learning_model.rs" line_range="45-54" />
<code_context>
+ if let Some(risks) = &considerations.ethical_considerations
</code_context>
<issue_to_address>
**suggestion:** Consider skipping fully-empty risk objects when all fields are `None`
Right now each `ethical_considerations` entry becomes an object even when both `name` and `mitigation_strategy` are `None`, producing `{}` entries. This may be unexpected for consumers. Consider either dropping objects that end up empty or only inserting when `name` is set, whichever best matches your schema.
Suggested implementation:
```rust
if let Some(risks) = &considerations.ethical_considerations
&& !risks.is_empty()
{
let arr: Vec<Value> = risks
.iter()
.filter_map(|r| {
let mut obj = Map::new();
if let Some(name) = &r.name {
obj.insert("name".to_string(), Value::String(name.clone()));
}
if let Some(ms) = &r.mitigation_strategy {
```
To fully implement the behavior of skipping fully-empty risk objects, adjust the rest of the closure so that instead of unconditionally returning `Value::Object(obj)`, it returns `None` when `obj.is_empty()`:
```rust
if let Some(ms) = &r.mitigation_strategy {
obj.insert("mitigation_strategy".to_string(), Value::String(ms.clone()));
}
if obj.is_empty() {
None
} else {
Some(Value::Object(obj))
}
```
This should replace the previous `Value::Object(obj)` expression at the end of the closure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+124
to
+125
| if map.is_empty() { | ||
| Value::Null |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Revisit behavior change for model cards with only empty generic properties
Previously, an empty card.properties vector resulted in Some(Map::new()) and thus Value::Object({}). With the new check, the same case now yields Value::Null. This changes observable behavior for model cards that exist but have no effective properties and may break callers that distinguish between an empty object and null. If that distinction matters, consider still returning Value::Object(map) when card.properties was Some(_), even if the map is empty.
Comment on lines
+45
to
+54
| if let Some(risks) = &considerations.ethical_considerations | ||
| && !risks.is_empty() | ||
| { | ||
| let arr: Vec<Value> = risks | ||
| .iter() | ||
| .map(|r| { | ||
| let mut obj = Map::new(); | ||
| if let Some(name) = &r.name { | ||
| obj.insert("name".to_string(), Value::String(name.clone())); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Consider skipping fully-empty risk objects when all fields are None
Right now each ethical_considerations entry becomes an object even when both name and mitigation_strategy are None, producing {} entries. This may be unexpected for consumers. Consider either dropping objects that end up empty or only inserting when name is set, whichever best matches your schema.
Suggested implementation:
if let Some(risks) = &considerations.ethical_considerations
&& !risks.is_empty()
{
let arr: Vec<Value> = risks
.iter()
.filter_map(|r| {
let mut obj = Map::new();
if let Some(name) = &r.name {
obj.insert("name".to_string(), Value::String(name.clone()));
}
if let Some(ms) = &r.mitigation_strategy {To fully implement the behavior of skipping fully-empty risk objects, adjust the rest of the closure so that instead of unconditionally returning Value::Object(obj), it returns None when obj.is_empty():
if let Some(ms) = &r.mitigation_strategy {
obj.insert("mitigation_strategy".to_string(), Value::String(ms.clone()));
}
if obj.is_empty() {
None
} else {
Some(Value::Object(obj))
}This should replace the previous Value::Object(obj) expression at the end of the closure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2399 +/- ##
==========================================
+ Coverage 71.37% 71.50% +0.12%
==========================================
Files 441 441
Lines 26747 26886 +139
Branches 26747 26886 +139
==========================================
+ Hits 19092 19224 +132
+ Misses 6541 6533 -8
- Partials 1114 1129 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
It feels a bit weird taking some fields from the original structure, stuffing then in some JSON column, renaming part of the fields in the process. I could see value in just taking in the while model card as JSONB (depending on how big they can get). Or using dedicated columns in the database. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ModelCardConsiderationsfields intosbom_ai.propertiesduring AIBOM ingestionethicalConsiderations→safetyRiskAssessment(JSON array of{name, mitigationStrategy}objects)fairnessAssessments→fairnessAssessments(JSON array of{groupAtRisk, benefits, harms, mitigationStrategy}objects)performanceTradeoffs→performanceTradeoffs(semicolon-joined string)useCases→useCases(semicolon-joined string)users→users(semicolon-joined string)Test plan
ingest_ai_cyclonedx_structured_fieldstest with assertions for all 5 new fieldstest_structured_model_card.jsonfixture with CycloneDX 1.6 considerations dataImplements TC-4796
🤖 Generated with Claude Code
Summary by Sourcery
Map CycloneDX AI model card structured fields into sbom_ai properties and validate ingestion across multiple vendor fixtures.
New Features:
Enhancements:
Tests: