Conversation
Signed-off-by: Roaa Sakr <romoh@microsoft.com>
romoh
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request adds support for mutating the diff_ids field in OCI image configurations to test edge cases in container runtimes. The field is changed from Vec<String> to Option<Vec<String>> to enable fuzzing scenarios where this field can be null or missing.
Changes:
- Modified
diff_idsfield inRootFsstruct to be optional, enabling null/omitted serialization - Added four new extreme mutations to test different
diff_idsedge cases - Updated existing code to wrap
diff_idsinSome()
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/confuzzled_lib/src/oci.rs | Changed diff_ids field from Vec<String> to Option<Vec<String>> with skip_serializing attribute, and updated minimal image initialization |
| crates/confuzzled_lib/src/mutations/mod.rs | Registered four new diff_ids-related mutations in the extreme category |
| crates/confuzzled_lib/src/mutations/extreme.rs | Implemented four new mutation structs for testing diff_ids edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub diff_ids: Vec<String>, | ||
| /// Layer diff IDs. When None, serializes as null which can trigger | ||
| /// edge cases in container runtimes that expect this field. | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
The documentation states 'serializes as null' but with skip_serializing_if = \"Option::is_none\", a None value will be omitted from serialization entirely, not serialized as null. The comment should clarify this behavior or the attribute should be changed to serialize as explicit null if that's the intended behavior.
| #[serde(skip_serializing_if = "Option::is_none")] |
| } | ||
| } | ||
|
|
||
| /// Empty layers with null diff_ids |
There was a problem hiding this comment.
The documentation comment says 'null diff_ids' but with the skip_serializing_if attribute on the field, setting diff_ids to None will omit the field from serialization rather than serialize it as null. Update the comment to reflect that the field will be 'omitted' rather than 'null'.
| // Clear all layers from manifest | ||
| image.layers.clear(); | ||
|
|
||
| // Set diff_ids to None (serializes as null or omitted) |
There was a problem hiding this comment.
The comment says 'serializes as null or omitted' but should be more precise. With skip_serializing_if = \"Option::is_none\", it will be omitted, not serialized as null. Either update the comment to say 'omitted' or clarify the actual behavior intended.
| // Set diff_ids to None (serializes as null or omitted) | |
| // Set diff_ids to None (will be omitted from serialization if using `skip_serializing_if = "Option::is_none"`) |
| /// Null diff_ids with layers intact | ||
| /// | ||
| /// Sets diff_ids to null while keeping the actual layers. |
There was a problem hiding this comment.
The documentation says 'Sets diff_ids to null' but with the skip_serializing_if attribute, setting to None will omit the field rather than serialize it as null. Update the documentation to accurately describe the behavior as 'omits diff_ids' or 'removes diff_ids field'.
| } | ||
|
|
||
| fn apply(&self, mut image: Image, _rng: &mut StdRng) -> Result<Image> { | ||
| // Keep layers but null out diff_ids |
There was a problem hiding this comment.
The comment says 'null out diff_ids' but the field will be omitted during serialization due to skip_serializing_if. Consider changing to 'omit diff_ids' or 'remove diff_ids' for accuracy.
1 participant
Summary
Add support for mutating diffids
Validation
Ran locally and validated different mutations generated.