WIP: Add #[must_use] to parse_suppression functions#1534
WIP: Add #[must_use] to parse_suppression functions#1534EffortlessSteven wants to merge 7 commits intomainfrom
Conversation
Work item: work-a140ddf6 Fixes clippy::unnecessary_wraps on cmd_doctor() which returns Result<i32> but never produces Err. The function always returns Ok(0) or Ok(1).
Add detailed docstring explaining what validate_config_rules checks: - Duplicate rule IDs - Empty pattern lists - Invalid regex patterns - Invalid multiline_window values - Unknown rule dependencies - Invalid path globs
Work item: work-3d8d9b32
- Remove unused doc comments inside proptest! blocks (clippy warning) - Fix formatting in property_test_string_syntax_invariant.rs - Add missing insta dependency to diffguard-analytics dev-dependencies These are pre-existing issues on the branch that block CI.
…duration_overflow test
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Summary by CodeRabbit
WalkthroughThis change introduces two new configuration options ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #[test] | ||
| fn language_from_str_yaml_toml_json_aliases() { | ||
| // YAML has 'yml' alias | ||
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); |
| // YAML has 'yml' alias | ||
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); | ||
| // JSON has 'jsonc' and 'json5' aliases (JSON with comments) | ||
| assert_eq!("jsonc".parse::<Language>().unwrap(), Language::Json); |
| assert_eq!("yml".parse::<Language>().unwrap(), Language::Yaml); | ||
| // JSON has 'jsonc' and 'json5' aliases (JSON with comments) | ||
| assert_eq!("jsonc".parse::<Language>().unwrap(), Language::Json); | ||
| assert_eq!("json5".parse::<Language>().unwrap(), Language::Json); |
| "Yaml", | ||
| "Yml", | ||
| ]) { | ||
| let lang: Language = alias.parse().unwrap(); |
| "Jsonc", | ||
| "Json5", | ||
| ]) { | ||
| let lang: Language = alias.parse().unwrap(); |
|
|
||
| #[test] | ||
| fn toml_parsed_returns_cstyle() { | ||
| let lang: Language = "toml".parse().unwrap(); |
BUILT Gate — Documentation ReviewReviewed the Explanatory comments added
Other findings
The documentation changes are minimal and do not affect logic or behavior. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/diffguard-types/src/lib.rs (1)
290-303: 🧹 Nitpick | 🔵 TrivialDocument why new fields default to
Nonewhile existing fields default toSome(...).Within
Defaults::default(),base/head/scope/fail_on/max_findings/diff_contextcarry concreteSomevalues, butignore_comments/ignore_stringsareNone. That's defensible (None = "inherit" inmerge_configs), but it silently changes the meaning of..Default::default()for the new fields vs. existing ones — a future maintainer is likely to "fix" this inconsistency or be tripped up by it (notably tests liketest_merge_defaults_field_wiserely on..Default::default()propagating concreteSomevalues).A short comment explaining the intent (inherit semantics in
merge_configs) would prevent that.📝 Suggested doc comment
+ // NOTE: Unlike the other Defaults fields, these intentionally default to + // None so that omitted values inherit through `merge_configs` rather than + // overriding parent configs with a concrete bool. Keep them as None here. ignore_comments: None, ignore_strings: None, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-types/src/lib.rs` around lines 290 - 303, Add a short doc comment to Defaults::default explaining why some fields are initialized to Some(...) while new fields ignore_comments and ignore_strings are set to None: state that None indicates "inherit" semantics used by merge_configs (so ..Default::default() keeps existing behavior and tests like test_merge_defaults_field_wise continue to pass) and mention that new fields defaulting to None is intentional to avoid breaking merges. Reference Defaults::default, ignore_comments, ignore_strings, merge_configs, and test_merge_defaults_field_wise in the comment so future maintainers understand the rationale.crates/diffguard-testkit/src/arb.rs (1)
357-378: 🧹 Nitpick | 🔵 TrivialGenerate optional booleans instead of hardcoding
Noneto keep proptest coverage.
ignore_commentsandignore_stringsare pinned toNone, so the new fields are never exercised byarb_defaults-driven properties (e.g., TOML/JSON round-trip inproperties.rs, defaults merging if covered by future tests). The other optional fields useprop::option::of(...)— match that pattern so all three states (None/Some(true)/Some(false)) are explored.♻️ Proposed refactor
pub fn arb_defaults() -> impl Strategy<Value = Defaults> { ( arb_optional_string(), arb_optional_string(), prop::option::of(arb_scope()), prop::option::of(arb_fail_on()), prop::option::of(0u32..1000), prop::option::of(0u32..10), + prop::option::of(any::<bool>()), + prop::option::of(any::<bool>()), ) .prop_map( - |(base, head, scope, fail_on, max_findings, diff_context)| Defaults { + |(base, head, scope, fail_on, max_findings, diff_context, ignore_comments, ignore_strings)| Defaults { base, head, scope, fail_on, max_findings, diff_context, - ignore_comments: None, - ignore_strings: None, + ignore_comments, + ignore_strings, }, ) }As per coding guidelines: "Proptest strategy functions should use prop_oneof!, prop_compose!, or combinators, avoiding prop_filter when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-testkit/src/arb.rs` around lines 357 - 378, arb_defaults currently hardcodes Defaults.ignore_comments and Defaults.ignore_strings to None, preventing proptest from exploring Some(true/false); update the tuple strategy in arb_defaults to include optional booleans for those fields (e.g., use prop::option::of(any::<bool>()) or a helper arb_optional_bool()) and map them into Defaults.ignore_comments and Defaults.ignore_strings instead of None so all three states (None/Some(true)/Some(false)) are exercised by the strategy.crates/diffguard-types/tests/properties.rs (1)
79-100: 🧹 Nitpick | 🔵 TrivialSame gap as in
diffguard-testkit/src/arb.rs: generateOption<bool>instead of hardcodingNone.Properties like
config_file_toml_round_trip,defaults_field_names_are_snake_case, andconfig_file_validates_against_schemarely on this generator. With both new fields fixed toNone, the schema/serde paths forSome(true)/Some(false)are never exercised by proptests — exactly the cases most likely to surface a regression.♻️ Proposed refactor
fn arb_defaults() -> impl Strategy<Value = Defaults> { ( arb_optional_string(), arb_optional_string(), prop::option::of(arb_scope()), prop::option::of(arb_fail_on()), prop::option::of(0u32..1000), prop::option::of(0u32..10), + prop::option::of(any::<bool>()), + prop::option::of(any::<bool>()), ) .prop_map( - |(base, head, scope, fail_on, max_findings, diff_context)| Defaults { + |(base, head, scope, fail_on, max_findings, diff_context, ignore_comments, ignore_strings)| Defaults { base, head, scope, fail_on, max_findings, diff_context, - ignore_comments: None, - ignore_strings: None, + ignore_comments, + ignore_strings, }, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-types/tests/properties.rs` around lines 79 - 100, The arb_defaults() generator currently always sets Defaults.ignore_comments and Defaults.ignore_strings to None, so property tests never exercise Some(true)/Some(false); update arb_defaults (the function named arb_defaults) to generate prop::option::of(prop::bool::ANY) (i.e., Option<bool>) for those two fields and map them into Defaults.ignore_comments and Defaults.ignore_strings instead of hardcoding None so serde/schema paths for both Some(true) and Some(false) are exercised by tests.schemas/diffguard.config.schema.json (1)
245-252:⚠️ Potential issue | 🟠 MajorSchema now allows
patterns-less rules that the runtime rejects — schema/runtime mismatch.Removing
patternsfromrequiredand assigningdefault: []creates a runtime incompatibility:
crates/diffguard-domain/src/rules.rs(lines 106–110) returnsRuleCompileError::MissingPatternswhenpatterns.is_empty()crates/diffguard/src/main.rs(lines 797–799) validates and rejects empty patterns with"Rule '<id>': no patterns defined"- Multiple test suites (properties, built-in, testkit) explicitly assert
!patterns.is_empty()- Fuzzing harnesses skip rules with empty patterns, noting they "would fail compilation"
A valid schema config with no patterns will still fail at runtime. External tooling (IDE completion, code generators) will be misled.
Fix: Restore
patternstorequiredand removedefault: []. This keeps the schema aligned with the runtime contract.🛠️ Proposed schema fix
"patterns": { "description": "One or more regex patterns.\nAlso accepts `match` as a TOML shorthand.", "type": "array", - "default": [], "items": { "type": "string" } }, ... "required": [ "id", - "severity" + "severity", + "patterns" ]Also applies to: lines 278–281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/diffguard.config.schema.json` around lines 245 - 252, The JSON schema change made the "patterns" array optional by adding "default": [] and removing it from required, but the runtime expects non-empty patterns (see RuleCompileError::MissingPatterns and the runtime validation that emits "Rule '<id>': no patterns defined"); revert the schema to require "patterns" and remove the default so the property is listed in "required" and has no default value, ensuring the "patterns" property remains mandatory and schema-level validation matches runtime checks (apply the same fix for the other occurrence referenced in the diff).fuzz/fuzz_targets/config_parser.rs (1)
65-76:⚠️ Potential issue | 🔴 CriticalInvalid struct field syntax at lines 65-76.
The struct definition contains invalid Rust syntax. Struct fields must declare types, not initialization expressions:
// Invalid (current code): match_mode: Default::default(), multiline: false, context_patterns: vec![], // Valid: match_mode: MatchMode, multiline: bool, context_patterns: Vec<Pattern>,All fields from lines 65-73 (
match_modethroughdepends_on) lack type annotations. Additionally, the#[allow(dead_code)]attribute on the struct (line 54) suggests these fields are unused, which is correct—theto_toml_stringmethod does not reference any of these fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fuzz/fuzz_targets/config_parser.rs` around lines 65 - 76, The struct currently defines fields with initializer expressions instead of types (e.g., match_mode: Default::default(), multiline: false, context_patterns: vec![]) which is invalid; change each field declaration in the struct (the block annotated with #[allow(dead_code)]) to use proper type annotations (for example match_mode: MatchMode, multiline: bool, context_patterns: Vec<Pattern> or Vec<String>, multiline_window: Option<...>, escalate_window: Option<...>, escalate_to: Option<String>, depends_on: Vec<String>, help: Option<String>, url: Option<String>) so the compiler sees types not values, and make sure these types align with how the struct is used by to_toml_string and other methods in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.hermes/conveyor/work-3d8d9b32/adr.md:
- Around line 1-60: The PR includes an unrelated ADR artifact (ADR-0013 in
work-3d8d9b32) that belongs to a different work item than the parse_suppression
#[must_use] change; either remove or move the ADR file(s) out of this branch and
into a separate PR/branch for work-3d8d9b32, or update the PR description to
explicitly state why ADR-0013 is intentionally included; look for the ADR
identifier "ADR-0013" and the work marker "work-3d8d9b32" and the
parse_suppression/must_use changes to decide whether to revert the ADR commit
from this branch or create a new branch/PR containing the ADR alone.
In `@adr.md`:
- Line 16: The ADR documents the wrong parameter type for
parse_suppression_in_comments; update the ADR entry to match the actual function
signature in crates/diffguard-domain/src/suppression.rs by changing the
documented parameter from masked_comments: &MaskedComments to masked_comments:
&str (i.e., document the signature as parse_suppression_in_comments(line: &str,
masked_comments: &str) -> Option<Suppression>), and ensure any surrounding prose
that references MaskedComments is adjusted to reflect that the function takes a
&str.
In `@CHANGELOG.md`:
- Line 24: The CHANGELOG entry incorrectly documents #[must_use] on
parse_unified_diff (issue `#329`) while this PR implements #[must_use] for
parse_suppression / parse_suppression_in_comments (issue `#364` / PR `#543`); update
the entry to reference parse_suppression and parse_suppression_in_comments and
the correct issue/PR numbers (or remove the parse_unified_diff line if it
belongs to another PR), and fix the markdown indentation so the bullet sits at
the correct nesting level consistent with the surrounding "### Changed" / "###
Refactored" sections.
In `@crates/diffguard-analytics/tests/snapshot_tests.rs`:
- Around line 249-265: Update the tests to snapshot full normalized/merged
baseline JSON instead of boolean/length-only values: in
snapshot_normalize_idempotent call
normalize_false_positive_baseline(baseline.clone()), serialize or format the
resulting FalsePositiveBaseline (type FalsePositiveBaseline) and assert that
snapshot matches that JSON, and keep idempotence as a separate
assert_eq!(normalized1, normalized2); similarly in snapshot_merge_deduplication
snapshot the full merged baseline (not just merged.entries.len()) so the
snapshot will catch wrong-winner or ordering regressions, while any length
assertions can remain as explicit asserts if needed.
- Around line 436-473: The test snapshot_append_trend_run_with_limit currently
only snapshots history.runs.len(), which won't catch an eviction-order
regression; update the test to snapshot the retained runs themselves (e.g., the
full history.runs or at minimum the sequence of history.runs[*].started_at) so
that append_trend_run's retention policy is asserted; locate the test and
replace the insta::assert_snapshot! call to snapshot the runs collection (or map
to started_at strings) to lock in the eviction order implemented by
append_trend_run.
In `@crates/diffguard-domain/src/property_test_string_syntax.rs`:
- Around line 49-60: The empty match over StringSyntax in the property test is a
no-op; remove the match block from the string_syntax() property test (or replace
it with a real invariant) so the test does meaningful work. If you want to keep
a runtime check, add an actual assertion using the StringSyntax type returned by
string_syntax(), e.g. compare the value to an expected/equatable value or assert
a real property (like that formatting/debug representation is non-empty or that
cloning yields an equal value) instead of matching with empty arms.
- Around line 1-198: Move property_test_string_syntax.rs out of the src/ tree
into the crate-level tests folder (crates/diffguard-domain/tests/) so it becomes
an integration test; keep the extern import use
diffguard_domain::preprocess::{Language, StringSyntax}; (no crate:: import
needed after move), remove any src module declarations (e.g., mod
property_test_string_syntax) referencing it, and ensure proptest remains a
dev-dependency in Cargo.toml (no #[cfg(test)] gating required once in tests/).
In `@crates/diffguard-domain/tests/property_test_string_syntax_invariant.rs`:
- Around line 46-71: The three unit tests yaml_returns_cstyle,
toml_returns_cstyle, and json_returns_cstyle duplicate assertions already
covered by the proptest cstyle_languages_return_cstyle; consolidate by replacing
them with a single table-driven unit test (or remove them) that iterates over
the languages [Language::Yaml, Language::Toml, Language::Json] and asserts
string_syntax() == StringSyntax::CStyle, or keep only the existing
cstyle_languages_return_cstyle property test; update or remove the duplicated
test functions (yaml_returns_cstyle, toml_returns_cstyle, json_returns_cstyle)
accordingly to avoid redundant coverage.
- Around line 13-41: The test uses proptest! with prop_oneof![Just(...)] over a
fixed list of Language::... variants (in the
all_languages_return_valid_string_syntax test), which isn't true property
testing; replace it by either iterating over the known enum variants in a plain
unit test loop (call Language::variants or an explicit table and invoke
lang.string_syntax() for each) or implement/derive
proptest::arbitrary::Arbitrary for Language (e.g., via proptest-derive) and
change the strategy to any::<Language>() so new variants are covered
automatically; apply the same change to the other two similar blocks (the tests
around lines 116-136 and 138-162) to ensure real property-based sampling or
deterministic enumeration.
- Around line 75-78: The test case function yaml_aliases_return_cstyle includes
an invalid alias "YAM" which maps to Language::Unknown; update the test data by
removing "YAM" and replacing it with "yML" so Language::from_str("yML")
lowercases to "yml" and yields Language::Yaml, ensuring the assertion that
recognized YAML aliases return StringSyntax::CStyle is actually exercised.
In `@crates/diffguard-domain/tests/snapshot_tests_work_65ff3da7.rs`:
- Around line 19-25: The snapshot labels are hardcoded and misleading for
Language::TypeScript and Language::Ruby; update the test so the second tuple
element is derived from the Language value instead of a hardcoded string—e.g.,
produce the label using the Language's Debug representation (format!("{:?}",
lang)) or otherwise derive it consistently (do not call string_syntax() if
earlier comments prohibit it) so that the languages array entries (the tuple
pairs around the variable named languages and the variants Language::TypeScript
and Language::Ruby) reflect the correct, non-misleading labels.
- Around line 13-48: The test test_all_language_string_syntax currently only
formats the hardcoded (lang, expected) pairs instead of invoking the actual
implementation; update the loop to call Language::string_syntax() for each lang
and compare/record that result in the snapshot (e.g., use lang.string_syntax()
or Language::string_syntax(lang) when building snapshot) so the snapshot asserts
the real outputs from string_syntax(), keeping the languages array of expected
values for reference and leaving the
assert_snapshot!("all_language_string_syntax", snapshot) as the final assertion.
In `@crates/diffguard-types/tests/red_tests_work_a98db3d3.rs`:
- Around line 102-117: The test test_defaults_explicit_none_for_ignore_flags is
using a full Defaults struct literal which is brittle; change it to build the
struct using the spread-default pattern (e.g., Defaults { ignore_comments: None,
ignore_strings: None, ..Defaults::default() }) so only the two fields under test
are set explicitly and all other fields come from Defaults::default(); after
making this change remove the now-unused FailOn and Scope imports if they become
unused.
In `@crates/diffguard/tests/duration_overflow_work_3010cb68.rs`:
- Around line 17-79: The tests that grep src/main.rs
(test_duration_instant_conversion_uses_saturation,
test_duration_datetime_conversion_uses_saturation,
test_only_expected_duration_casts_exist and helper find_line_with_pattern) are
fragile; remove those three scraping tests and instead either (A) keep only the
pure-math tests (test_u128_to_u64_saturation_logic and
test_i64_to_u64_saturation_logic) or (B) refactor the runtime logic into small
helpers (e.g., saturating_millis_from_instant and saturating_millis_from_chrono)
called from main.rs and add unit tests for those helpers; update the test file
to delete the scraping tests and add focused unit tests for the new helper(s) so
assertions check behavior not source text.
- Around line 39-79: The tests fail because two unchecked casts truncate large
durations; update the sites that set duration_ms so they saturate before
casting: change the Instant path that uses start_time.elapsed().as_millis() to
call .min(u128::from(u64::MAX)) before the final as u64 (i.e.
start_time.elapsed().as_millis().min(u128::from(u64::MAX)) as u64), and change
the DateTime path that computes (ended_at -
*started_at).num_milliseconds().max(0) to also call .min(i64::MAX) before the as
u64 (i.e. (ended_at - *started_at).num_milliseconds().max(0).min(i64::MAX) as
u64); locate both by the duration_ms variable assignments and the expressions
start_time.elapsed().as_millis() and (ended_at - *started_at).num_milliseconds()
and apply these saturation calls.
In `@schemas/diffguard.check.schema.json`:
- Line 195: Update the "description" text for the "reasons" field so it no
longer calls the entries "human-readable tokens" but instead describes them as
stable token identifiers for programmatic consumption; specifically change the
phrase around `reasons` in the schema (referencing the examples `no_diff_input`
and `truncated`) to something like "stable token identifiers" or "machine-stable
tokens" and keep the rest of the context about `status` and `counts` intact.
In `@schemas/diffguard.config.schema.json`:
- Around line 220-223: The "message" property in the JSON schema currently has a
default of "" which is allowed at runtime but triggers --strict warnings; update
the "message" property in schemas/diffguard.config.schema.json to either remove
the default or, preferably, augment its "description" to state that "message" is
recommended (not required) and may be empty in runtime (matching behavior in
crates/diffguard/src/main.rs around the strict-mode handling), so downstream
tooling/users are informed and not surprised by strict-mode warnings.
In `@schemas/sensor.report.v1.schema.json`:
- Line 235: Update the "reasons" description in the Verdict definition to mirror
the wording used in diffguard.check.schema.json by referring to the reason
values as "stable token identifiers" (or the exact same phrase used in the check
schema) and keeping the rest of the sentence structure consistent so both
Verdict defs use identical language; locate the Verdict schema's "reasons"
description and replace the current phrasing with the matching "stable token
identifiers" phrasing.
- Line 219: Update the ToolMeta description in the sensor.report.v1 schema to
reference the correct artifact: replace the phrase "produced a check receipt"
(copied from diffguard.check) with wording that the tool "produced a sensor
report" (e.g., "Metadata describing the tool that produced a sensor report;
includes tool name and version for traceability in CI/CD pipelines"). Edit the
ToolMeta description text in the schema definition for sensor.report.v1 so docs
reflect the correct artifact.
In `@specs.md`:
- Around line 11-17: The spec's acceptance criteria currently rely on exact line
numbers for the #[must_use] attribute in suppression.rs which is brittle; change
the checks to assert the attribute on the function items themselves by scanning
for the function names instead (e.g., look for 'pub fn parse_suppression' and
verify a preceding '#[must_use]' and likewise for 'pub fn
parse_suppression_in_comments') or simply rely on the existing clippy
must_use_candidate verification; update the spec text/tests to remove the "line
70/85" references and replace them with function-name based verification for
parse_suppression and parse_suppression_in_comments in suppression.rs.
---
Outside diff comments:
In `@crates/diffguard-testkit/src/arb.rs`:
- Around line 357-378: arb_defaults currently hardcodes Defaults.ignore_comments
and Defaults.ignore_strings to None, preventing proptest from exploring
Some(true/false); update the tuple strategy in arb_defaults to include optional
booleans for those fields (e.g., use prop::option::of(any::<bool>()) or a helper
arb_optional_bool()) and map them into Defaults.ignore_comments and
Defaults.ignore_strings instead of None so all three states
(None/Some(true)/Some(false)) are exercised by the strategy.
In `@crates/diffguard-types/src/lib.rs`:
- Around line 290-303: Add a short doc comment to Defaults::default explaining
why some fields are initialized to Some(...) while new fields ignore_comments
and ignore_strings are set to None: state that None indicates "inherit"
semantics used by merge_configs (so ..Default::default() keeps existing behavior
and tests like test_merge_defaults_field_wise continue to pass) and mention that
new fields defaulting to None is intentional to avoid breaking merges. Reference
Defaults::default, ignore_comments, ignore_strings, merge_configs, and
test_merge_defaults_field_wise in the comment so future maintainers understand
the rationale.
In `@crates/diffguard-types/tests/properties.rs`:
- Around line 79-100: The arb_defaults() generator currently always sets
Defaults.ignore_comments and Defaults.ignore_strings to None, so property tests
never exercise Some(true)/Some(false); update arb_defaults (the function named
arb_defaults) to generate prop::option::of(prop::bool::ANY) (i.e., Option<bool>)
for those two fields and map them into Defaults.ignore_comments and
Defaults.ignore_strings instead of hardcoding None so serde/schema paths for
both Some(true) and Some(false) are exercised by tests.
In `@fuzz/fuzz_targets/config_parser.rs`:
- Around line 65-76: The struct currently defines fields with initializer
expressions instead of types (e.g., match_mode: Default::default(), multiline:
false, context_patterns: vec![]) which is invalid; change each field declaration
in the struct (the block annotated with #[allow(dead_code)]) to use proper type
annotations (for example match_mode: MatchMode, multiline: bool,
context_patterns: Vec<Pattern> or Vec<String>, multiline_window: Option<...>,
escalate_window: Option<...>, escalate_to: Option<String>, depends_on:
Vec<String>, help: Option<String>, url: Option<String>) so the compiler sees
types not values, and make sure these types align with how the struct is used by
to_toml_string and other methods in this file.
In `@schemas/diffguard.config.schema.json`:
- Around line 245-252: The JSON schema change made the "patterns" array optional
by adding "default": [] and removing it from required, but the runtime expects
non-empty patterns (see RuleCompileError::MissingPatterns and the runtime
validation that emits "Rule '<id>': no patterns defined"); revert the schema to
require "patterns" and remove the default so the property is listed in
"required" and has no default value, ensuring the "patterns" property remains
mandatory and schema-level validation matches runtime checks (apply the same fix
for the other occurrence referenced in the diff).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 129c8011-b88f-4414-a9d5-53be39000899
⛔ Files ignored due to path filters (27)
crates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_append_trend_run.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_append_trend_run_with_limit.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_baseline_deduplication.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_baseline_empty_findings.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_baseline_multiple_findings.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_baseline_single_finding.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_fingerprint_determinism.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_fingerprint_format.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_fingerprint_sensitivity.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_fingerprint_set.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_fingerprint_single_finding.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_merge_deduplication.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_merge_prefers_existing.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_merge_union.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_normalize_idempotent.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_normalize_sets_schema.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_normalize_sorts_entries.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_summarize_single_run.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_summarize_trend_history.snapis excluded by!**/*.snapcrates/diffguard-analytics/tests/snapshots/snapshot_tests__snapshot_trend_run_from_receipt.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__all_language_string_syntax.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__json_double_quoted_string_sanitized.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__json_string_syntax_type.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__toml_double_quoted_string_sanitized.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__toml_string_syntax_type.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__yaml_double_quoted_string_sanitized.snapis excluded by!**/*.snapcrates/diffguard-domain/tests/snapshots/snapshot_tests_work_65ff3da7__yaml_string_syntax_type.snapis excluded by!**/*.snap
📒 Files selected for processing (31)
.hermes/conveyor/work-3d8d9b32/adr.md.hermes/conveyor/work-3d8d9b32/specs.mdCHANGELOG.mdStringSyntax::CStyleadr.mdcrates/diffguard-analytics/Cargo.tomlcrates/diffguard-analytics/tests/snapshot_tests.rscrates/diffguard-domain/src/preprocess.rscrates/diffguard-domain/src/property_test_string_syntax.rscrates/diffguard-domain/tests/property_test_string_syntax_invariant.rscrates/diffguard-domain/tests/snapshot_tests_work_65ff3da7.rscrates/diffguard-testkit/src/arb.rscrates/diffguard-testkit/src/fixtures.rscrates/diffguard-types/src/lib.rscrates/diffguard-types/tests/properties.rscrates/diffguard-types/tests/red_tests_work_a98db3d3.rscrates/diffguard/src/config_loader.rscrates/diffguard/src/main.rscrates/diffguard/tests/duration_overflow_work_3010cb68.rsdiffguard.toml.examplefuzz/fuzz_targets/baseline_receipt.rsfuzz/fuzz_targets/config_parser.rsfuzz/fuzz_targets/evaluate_lines.rsfuzz/fuzz_targets/preprocess.rsfuzz/fuzz_targets/rule_matcher.rsfuzz/fuzz_targets/unified_diff_parser.rsschemas/diffguard.check.schema.jsonschemas/diffguard.config.schema.jsonschemas/diffguard.trend-history.v1.schema.jsonschemas/sensor.report.v1.schema.jsonspecs.md
💤 Files with no reviewable changes (1)
- fuzz/fuzz_targets/preprocess.rs
| # ADR-0013: Close Issue #289 — Severity::Info Already Maps to "info" | ||
|
|
||
| ## Status | ||
| Accepted | ||
|
|
||
| ## Context | ||
|
|
||
| Issue #289 reported that `checkstyle.rs` had `Severity::Info` and `Severity::Warn` both mapping to the string `"warning"`, causing a `clippy::match_same_arms` warning and making the Info arm dead code. | ||
|
|
||
| However, prior investigation found that: | ||
| - The bug was already fixed in PR #460 (commit b31d836) | ||
| - The current code at `checkstyle.rs:71-75` correctly maps all three severities | ||
| - All 28 checkstyle tests pass | ||
| - Clippy is clean with no warnings | ||
|
|
||
| The issue remains OPEN on GitHub despite the fix being merged. | ||
|
|
||
| ## Decision | ||
|
|
||
| **No code changes are needed.** Issue #289 should be closed as "Already resolved" referencing PR #460. | ||
|
|
||
| The current implementation is correct: | ||
| ```rust | ||
| let severity_str = match f.severity { | ||
| Severity::Error => "error", | ||
| Severity::Warn => "warning", | ||
| Severity::Info => "info", // ← Correct | ||
| }; | ||
| ``` | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Tradeoffs | ||
|
|
||
| | Alternative | Why Rejected | | ||
| |-------------|--------------| | ||
| | Create new PR with identical fix | Duplicates PR #460, wastes review time, risks new bugs | | ||
| | Leave issue open | Misleads contributors, suggests bug still exists | | ||
| | Modify working code | Unnecessary churn, no benefit | | ||
|
|
||
| ### Benefits | ||
| - No risk of introducing regressions | ||
| - Preserves existing test coverage (`info_maps_to_info` test) | ||
| - Issue closure provides clear signal to contributors | ||
|
|
||
| ### Risks | ||
| - Issue #289 must be closed on GitHub to prevent confusion | ||
| - If future refactoring moves the severity mapping, regression tests must catch it | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| 1. **Create duplicate PR** — Rejected: PR #460 already contains the correct fix | ||
| 2. **Do nothing** — Rejected: Open issue misleads contributors | ||
| 3. **Modify working code** — Rejected: No benefit, introduces risk | ||
|
|
||
| ## References | ||
|
|
||
| - Issue #289: `checkstyle.rs:50-51: Severity::Info and Severity::Warn produce identical "warning"` | ||
| - PR #460: `fix(checkstyle): Severity::Info maps to 'info' not 'warning'` | ||
| - Commit `b31d836`: Merge commit that applied the fix No newline at end of file |
There was a problem hiding this comment.
This ADR appears unrelated to the PR's stated scope.
The PR description and linked issue (#364) target parse_suppression #[must_use] annotations under work item work-ec9e1665. This ADR (work-3d8d9b32) concerns the checkstyle.rs Severity::Info mapping bug (issue #289) — a different work item. Please confirm whether these .hermes/conveyor/work-3d8d9b32/ artifacts were intentionally included in this PR or should be split into a separate PR/branch to keep the conveyor-tracking PR focused on a single work item.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60-60: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.hermes/conveyor/work-3d8d9b32/adr.md around lines 1 - 60, The PR includes
an unrelated ADR artifact (ADR-0013 in work-3d8d9b32) that belongs to a
different work item than the parse_suppression #[must_use] change; either remove
or move the ADR file(s) out of this branch and into a separate PR/branch for
work-3d8d9b32, or update the PR description to explicitly state why ADR-0013 is
intentionally included; look for the ADR identifier "ADR-0013" and the work
marker "work-3d8d9b32" and the parse_suppression/must_use changes to decide
whether to revert the ADR commit from this branch or create a new branch/PR
containing the ADR alone.
|
|
||
| Add `#[must_use]` to both suppression-parsing functions: | ||
| - `parse_suppression(line: &str) -> Option<Suppression>` at `suppression.rs:70` | ||
| - `parse_suppression_in_comments(line: &str, masked_comments: &MaskedComments) -> Option<Suppression>` at `suppression.rs:85` |
There was a problem hiding this comment.
Incorrect parameter type in documented signature.
The actual signature in crates/diffguard-domain/src/suppression.rs:85-106 (from the relevant code snippet) is:
pub fn parse_suppression_in_comments(line: &str, masked_comments: &str) -> Option<Suppression>The ADR documents masked_comments: &MaskedComments, which does not match the actual &str type. Please correct the ADR to avoid drift between docs and code.
📝 Suggested fix
-- `parse_suppression_in_comments(line: &str, masked_comments: &MaskedComments) -> Option<Suppression>` at `suppression.rs:85`
+- `parse_suppression_in_comments(line: &str, masked_comments: &str) -> Option<Suppression>` at `suppression.rs:85`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adr.md` at line 16, The ADR documents the wrong parameter type for
parse_suppression_in_comments; update the ADR entry to match the actual function
signature in crates/diffguard-domain/src/suppression.rs by changing the
documented parameter from masked_comments: &MaskedComments to masked_comments:
&str (i.e., document the signature as parse_suppression_in_comments(line: &str,
masked_comments: &str) -> Option<Suppression>), and ensure any surrounding prose
that references MaskedComments is adjusted to reflect that the function takes a
&str.
| - Windows target triple detection for MSYS/MINGW environments | ||
| - Concurrency control on SARIF upload to prevent race conditions across workflow runs | ||
| - Improved error handling with user-visible warning messages for fallback installation paths | ||
| - **`parse_unified_diff` now requires explicit Result handling** — Added `#[must_use]` to `parse_unified_diff` so the compiler warns when callers ignore the `Result`. This prevents silent parse failures where malformed diffs are silently ignored. Callers must now explicitly handle the `Result` or use `let _ = ...` to indicate intentional ignore. Closes #329. |
There was a problem hiding this comment.
Changelog entry appears unrelated to this PR's stated objective.
This entry documents #[must_use] on parse_unified_diff (closes #329), but per the PR objectives this PR tracks #[must_use] on parse_suppression / parse_suppression_in_comments (issue #364, implemented in PR #543). If this is a draft/conveyor-tracking PR for #364, the changelog likely needs an entry for parse_suppression instead — or this entry belongs to a different PR and was included by mistake.
Also, the indentation flagged by markdownlint (MD005) is real: the surrounding bullets at this level use no indentation while the previous block's nested bullets used 2 spaces — please confirm this entry is meant to be top-level under ### Changed/### Refactored (currently it sits under ### Changed heading at line 18 but mixes with the list at lines 19-23 which look like they belong under the prior ### Refactored section).
🧰 Tools
🪛 LanguageTool
[style] ~24-~24: Consider using the typographical ellipsis character here instead.
Context: ...w explicitly handle the Result or use let _ = ... to indicate intentional ignore. Closes...
(ELLIPSIS)
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 24, The CHANGELOG entry incorrectly documents
#[must_use] on parse_unified_diff (issue `#329`) while this PR implements
#[must_use] for parse_suppression / parse_suppression_in_comments (issue `#364` /
PR `#543`); update the entry to reference parse_suppression and
parse_suppression_in_comments and the correct issue/PR numbers (or remove the
parse_unified_diff line if it belongs to another PR), and fix the markdown
indentation so the bullet sits at the correct nesting level consistent with the
surrounding "### Changed" / "### Refactored" sections.
| /// Snapshot test for baseline normalization - idempotent. | ||
| #[test] | ||
| fn snapshot_normalize_idempotent() { | ||
| let baseline = FalsePositiveBaseline { | ||
| schema: FALSE_POSITIVE_BASELINE_SCHEMA_V1.to_string(), | ||
| entries: vec![FalsePositiveEntry { | ||
| fingerprint: "abc123".to_string(), | ||
| rule_id: "test.rule".to_string(), | ||
| path: "test.rs".to_string(), | ||
| line: 1, | ||
| note: Some("note".to_string()), | ||
| }], | ||
| }; | ||
| let normalized1 = normalize_false_positive_baseline(baseline.clone()); | ||
| let normalized2 = normalize_false_positive_baseline(normalized1.clone()); | ||
| insta::assert_snapshot!("snapshot_normalize_idempotent", normalized1 == normalized2); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Boolean / length-only snapshots provide little regression value.
snapshot_normalize_idempotentsnapshots onlynormalized1 == normalized2(a bool). If a regression broke normalization shape but kept it idempotent, the snapshot still passes. Prefer snapshotting the normalized JSON and assertingnormalized1 == normalized2as a separateassert_eq!.snapshot_merge_deduplicationsnapshots onlymerged.entries.len(). It cannot detect a bug where the wrong entry wins or order changes. Snapshot the full merged baseline JSON instead.
Proposed changes
let normalized1 = normalize_false_positive_baseline(baseline.clone());
let normalized2 = normalize_false_positive_baseline(normalized1.clone());
- insta::assert_snapshot!("snapshot_normalize_idempotent", normalized1 == normalized2);
+ assert_eq!(normalized1, normalized2, "normalization must be idempotent");
+ let json = serde_json::to_string_pretty(&normalized1).expect("serialize baseline");
+ insta::assert_snapshot!("snapshot_normalize_idempotent", json); let merged = merge_false_positive_baselines(&base, &incoming);
- insta::assert_snapshot!("snapshot_merge_deduplication", merged.entries.len());
+ let json = serde_json::to_string_pretty(&merged).expect("serialize baseline");
+ insta::assert_snapshot!("snapshot_merge_deduplication", json);Also applies to: 327-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-analytics/tests/snapshot_tests.rs` around lines 249 - 265,
Update the tests to snapshot full normalized/merged baseline JSON instead of
boolean/length-only values: in snapshot_normalize_idempotent call
normalize_false_positive_baseline(baseline.clone()), serialize or format the
resulting FalsePositiveBaseline (type FalsePositiveBaseline) and assert that
snapshot matches that JSON, and keep idempotence as a separate
assert_eq!(normalized1, normalized2); similarly in snapshot_merge_deduplication
snapshot the full merged baseline (not just merged.entries.len()) so the
snapshot will catch wrong-winner or ordering regressions, while any length
assertions can remain as explicit asserts if needed.
| fn snapshot_append_trend_run_with_limit() { | ||
| let findings = vec![make_finding("rust.no_unwrap", "src/lib.rs", 42, "unwrap()")]; | ||
| let receipt = make_receipt(findings); | ||
|
|
||
| let history = diffguard_analytics::TrendHistory::default(); | ||
| let history = append_trend_run( | ||
| history, | ||
| trend_run_from_receipt( | ||
| &receipt, | ||
| "2026-01-01T00:00:00Z", | ||
| "2026-01-01T00:00:01Z", | ||
| 1000, | ||
| ), | ||
| Some(2), | ||
| ); | ||
| let history = append_trend_run( | ||
| history, | ||
| trend_run_from_receipt( | ||
| &receipt, | ||
| "2026-01-01T00:00:02Z", | ||
| "2026-01-01T00:00:03Z", | ||
| 1000, | ||
| ), | ||
| Some(2), | ||
| ); | ||
| let history = append_trend_run( | ||
| history, | ||
| trend_run_from_receipt( | ||
| &receipt, | ||
| "2026-01-01T00:00:04Z", | ||
| "2026-01-01T00:00:05Z", | ||
| 1000, | ||
| ), | ||
| Some(2), | ||
| ); | ||
|
|
||
| insta::assert_snapshot!("snapshot_append_trend_run_with_limit", history.runs.len()); | ||
| } |
There was a problem hiding this comment.
Snapshot is too weak to catch a retention-policy regression.
This test trims a 3-run history to Some(2) but only snapshots history.runs.len() (= 2). The contract for append_trend_run is to keep the newest entries (see crates/diffguard-analytics/src/lib.rs drain(0..drop_count)); a regression that kept the oldest two would still pass this snapshot. Snapshot the full history (or at least the retained started_at values) so the eviction policy is locked in.
Proposed strengthening
- insta::assert_snapshot!("snapshot_append_trend_run_with_limit", history.runs.len());
+ let retained_started: Vec<_> = history.runs.iter().map(|r| r.started_at.clone()).collect();
+ let json = serde_json::json!({
+ "len": history.runs.len(),
+ "retained_started_at": retained_started,
+ });
+ insta::assert_snapshot!("snapshot_append_trend_run_with_limit", json);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-analytics/tests/snapshot_tests.rs` around lines 436 - 473,
The test snapshot_append_trend_run_with_limit currently only snapshots
history.runs.len(), which won't catch an eviction-order regression; update the
test to snapshot the retained runs themselves (e.g., the full history.runs or at
minimum the sequence of history.runs[*].started_at) so that append_trend_run's
retention policy is asserted; locate the test and replace the
insta::assert_snapshot! call to snapshot the runs collection (or map to
started_at strings) to lock in the eviction order implemented by
append_trend_run.
| ] | ||
| }, | ||
| "Verdict": { | ||
| "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides human-readable tokens explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor wording: reasons are stable tokens, not human-readable prose.
The examples (no_diff_input, truncated) are machine-stable identifiers intended for programmatic consumption. Calling them "human-readable tokens" is slightly misleading; consider "stable token identifiers" to match how downstream consumers (CI, cockpit) actually treat them.
Proposed wording tweak
- "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides human-readable tokens explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).",
+ "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides stable token identifiers explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides human-readable tokens explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).", | |
| "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides stable token identifiers explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/diffguard.check.schema.json` at line 195, Update the "description"
text for the "reasons" field so it no longer calls the entries "human-readable
tokens" but instead describes them as stable token identifiers for programmatic
consumption; specifically change the phrase around `reasons` in the schema
(referencing the examples `no_diff_input` and `truncated`) to something like
"stable token identifiers" or "machine-stable tokens" and keep the rest of the
context about `status` and `counts` intact.
| "message": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "default": "" | ||
| }, |
There was a problem hiding this comment.
Minor: message default of "" is permitted at runtime but flagged in --strict.
Making message optional with default: "" is internally consistent (runtime accepts empty messages — see crates/diffguard/src/main.rs line 905 in strict mode it only emits a warning, not an error). Just be aware that schema-valid rules will still produce strict-mode warnings; consider documenting message as "recommended" in the description so downstream tooling/users aren't surprised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/diffguard.config.schema.json` around lines 220 - 223, The "message"
property in the JSON schema currently has a default of "" which is allowed at
runtime but triggers --strict warnings; update the "message" property in
schemas/diffguard.config.schema.json to either remove the default or,
preferably, augment its "description" to state that "message" is recommended
(not required) and may be empty in runtime (matching behavior in
crates/diffguard/src/main.rs around the strict-mode handling), so downstream
tooling/users are informed and not surprised by strict-mode warnings.
| ] | ||
| }, | ||
| "ToolMeta": { | ||
| "description": "Metadata describing the tool that produced a check receipt.\n\nIncludes the tool name and version for traceability in CI/CD pipelines.", |
There was a problem hiding this comment.
Wrong artifact name in ToolMeta description for sensor.report.v1.
The description says the tool "produced a check receipt", but this schema is sensor.report.v1 — the produced artifact is a sensor report, not a check receipt. Looks like the wording was copy-pasted from schemas/diffguard.check.schema.json without adapting to this schema's context. Since these descriptions surface in generated/published schema docs for ecosystem integrators, the mismatch is worth correcting.
Proposed fix
"ToolMeta": {
- "description": "Metadata describing the tool that produced a check receipt.\n\nIncludes the tool name and version for traceability in CI/CD pipelines.",
+ "description": "Metadata describing the tool that produced this sensor report.\n\nIncludes the tool name and version for traceability in CI/CD pipelines.",
"type": "object",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "description": "Metadata describing the tool that produced a check receipt.\n\nIncludes the tool name and version for traceability in CI/CD pipelines.", | |
| "ToolMeta": { | |
| "description": "Metadata describing the tool that produced this sensor report.\n\nIncludes the tool name and version for traceability in CI/CD pipelines.", | |
| "type": "object", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/sensor.report.v1.schema.json` at line 219, Update the ToolMeta
description in the sensor.report.v1 schema to reference the correct artifact:
replace the phrase "produced a check receipt" (copied from diffguard.check) with
wording that the tool "produced a sensor report" (e.g., "Metadata describing the
tool that produced a sensor report; includes tool name and version for
traceability in CI/CD pipelines"). Edit the ToolMeta description text in the
schema definition for sensor.report.v1 so docs reflect the correct artifact.
| ] | ||
| }, | ||
| "Verdict": { | ||
| "description": "The overall result of a check run.\n\n`status` is the top-level disposition, `counts` breaks down findings by\nseverity, and `reasons` provides human-readable tokens explaining any\nnon-pass outcome (e.g., `no_diff_input`, `truncated`).", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same reasons wording nit as diffguard.check.schema.json.
If you adopt the "stable token identifiers" wording in the check schema, please mirror it here so the two Verdict defs stay aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/sensor.report.v1.schema.json` at line 235, Update the "reasons"
description in the Verdict definition to mirror the wording used in
diffguard.check.schema.json by referring to the reason values as "stable token
identifiers" (or the exact same phrase used in the check schema) and keeping the
rest of the sentence structure consistent so both Verdict defs use identical
language; locate the Verdict schema's "reasons" description and replace the
current phrasing with the matching "stable token identifiers" phrasing.
| 1. **`#[must_use]` is present on `parse_suppression` at line 70 of `suppression.rs`** | ||
| - Verified by: `grep -n '^\#\[must_use\]' crates/diffguard-domain/src/suppression.rs` shows `#[must_use]` at line 70 | ||
| - No clippy `must_use_candidate` warning for `parse_suppression` under `-W clippy::pedantic` | ||
|
|
||
| 2. **`#[must_use]` is present on `parse_suppression_in_comments` at line 85 of `suppression.rs`** | ||
| - Verified by: `grep -n '^\#\[must_use\]' crates/diffguard-domain/src/suppression.rs` shows `#[must_use]` at line 85 | ||
| - Both sibling functions now consistently carry the attribute |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Line-number-based acceptance criteria are fragile.
Pinning #[must_use] to "line 70" and "line 85" of suppression.rs makes these criteria break under any unrelated edit (added doc comment, reordering, etc.). The relevant snippets confirm #[must_use] is in place but it would be more robust to assert presence on the function items themselves rather than line numbers — e.g., a grep -B1 'pub fn parse_suppression\b' suppression.rs | grep '#\[must_use\]'-style check, or rely solely on the clippy verification at lines 23-24/46.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs.md` around lines 11 - 17, The spec's acceptance criteria currently rely
on exact line numbers for the #[must_use] attribute in suppression.rs which is
brittle; change the checks to assert the attribute on the function items
themselves by scanning for the function names instead (e.g., look for 'pub fn
parse_suppression' and verify a preceding '#[must_use]' and likewise for 'pub fn
parse_suppression_in_comments') or simply rely on the existing clippy
must_use_candidate verification; update the spec text/tests to remove the "line
70/85" references and replace them with function-name based verification for
parse_suppression and parse_suppression_in_comments in suppression.rs.
BUILT Gate — Green Test CoverageRan the green-test-builder assessment against the Edge Cases Covered
What the Implementation Handles Well
Known Gaps (Acceptable)
Test ResultsVerdict: sufficient — 29/29 green tests pass. All acceptance criteria for the BUILT gate are met. |
PROVEN Gate — Property-Based TestingRan property-based testing against the suppression logic in `diffguard-domain` to verify the invariants that `#[must_use]` annotations protect. The property tests target the suppression parsing and tracking behavior itself, not the `#[must_use]` attribute (which is enforced at compile time by clippy). Properties VerifiedThe test suite covers 14 invariants across the suppression system:
Results
AssessmentThe suppression logic is sound. No invariants were violated across ~2,700 combined test iterations. The `#[must_use]` attributes on `parse_suppression` (line 70) and `parse_suppression_in_comments` (line 85) protect callers from silently discarding `Option` values, which would cause suppression directives to be ignored. The property tests confirm the logic these attributes protect is correct and consistent. Verdict: sufficient — PROVEN gate criteria met. |
PROVEN Gate — Integration Test VerificationRan the integration test suite against the Test Scenarios Covered
Component Handoffs Verified
Test ResultsVerdict: sufficient — All five integration test scenarios pass. The |
2 participants
Closes #364
Summary
Add attribute to and in to prevent callers from silently discarding suppression directives.
ADR
Specs
What Changed
This was implemented in PR #543 (commit ) and is being tracked through the conveyor system as work-ec9e1665.
Test Results
The implementation was verified in PR #543. The fuzz harness correctly uses to silence the warning intentionally.
Notes