WIP: Hoist escape_md to shared diffguard-types crate#609
WIP: Hoist escape_md to shared diffguard-types crate#609EffortlessSteven wants to merge 3 commits intomainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes consolidate a duplicated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
Doc Writer ReportDocumentation Added
Tests
Critical Friction IssueBLOCKED: The conveyor system constantly switches my working branch between commands, losing uncommitted changes. This happened 5+ times during this session. |
This commit completes the type migration started in this PR. DiffMeta.lines_scanned was upgraded to u64 but downstream consumers (CheckAnalytics, Evaluation) still used u32, causing type mismatch compile errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/diffguard-domain/src/preprocess.rs (2)
168-202:⚠️ Potential issue | 🟠 MajorIncomplete
#[must_use]coverage —strings_only()andcomments_and_strings()still missing the attribute.Only
none()andcomments_only()got#[must_use]. The companion red testexactly_six_must_use_attributes_in_preprocessintests/red_tests_work_e8a88475.rsexpects exactly 6 occurrences, andpreprocess_options_strings_only_has_must_use/preprocess_options_comments_and_strings_has_must_usewill fail on these two factories. Since all four return identicalSelfconfiguration values, the rationale applies uniformly.🛠 Proposed fix
+ #[must_use] pub fn strings_only() -> Self { Self { mask_comments: false, mask_strings: true, } } + #[must_use] pub fn comments_and_strings() -> Self { Self { mask_comments: true, mask_strings: true, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-domain/src/preprocess.rs` around lines 168 - 202, Add the missing #[must_use] attribute to the two factory methods that create PreprocessOptions so all four factories consistently carry the attribute: annotate the functions strings_only() and comments_and_strings() with #[must_use] (they currently mirror none() and comments_only()); this will satisfy the tests expecting exactly six must_use occurrences for the PreprocessOptions factory methods and keep the behavior unchanged.
273-289:⚠️ Potential issue | 🟠 Major
Preprocessor::newandPreprocessor::with_languageare missing#[must_use].Both constructors return
Selfand are covered by the red tests (preprocessor_new_has_must_use,preprocessor_with_language_has_must_use) plus the total-count assertion. Without this change the test binary cannot go GREEN.🛠 Proposed fix
impl Preprocessor { + #[must_use] pub fn new(opts: PreprocessOptions) -> Self { Self { opts, mode: Mode::Normal, lang: Language::Unknown, } } /// Create a new preprocessor with language-specific syntax support. + #[must_use] pub fn with_language(opts: PreprocessOptions, lang: Language) -> Self {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-domain/src/preprocess.rs` around lines 273 - 289, Add the #[must_use] attribute to both constructor methods so callers are warned when the returned Preprocessor is discarded: annotate the functions Preprocessor::new and Preprocessor::with_language with #[must_use] directly above their signatures, leaving their bodies unchanged.crates/diffguard-types/src/lib.rs (1)
125-140:⚠️ Potential issue | 🟠 MajorBump the check schema when widening
DiffMeta.lines_scanned.
DiffMetais part ofCHECK_SCHEMA_V1, so this changes the serialized contract without changing the schema id. Newer writers can now emit av1receipt that olderv1readers will reject oncelines_scannedexceedsu32::MAX. That compatibility break should be reflected in the schema version rather than hidden behind the existing constant.🤖 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 125 - 140, The change widens DiffMeta::lines_scanned from u32 to u64 which is a breaking serialization change for the existing CHECK_SCHEMA_V1; update the schema version constant and any places that emit/claim the schema id so the new struct is associated with a new schema (e.g. bump CHECK_SCHEMA_V1 -> CHECK_SCHEMA_V2 or create a new CHECK_SCHEMA_V2 constant and update serializer/receipt writers and tests that reference CHECK_SCHEMA_V1), and ensure any consumers that match on the schema id (deserializers, tests, docs) are adjusted to accept the new schema id so older v1 readers are not falsely claimed by writers emitting the widened DiffMeta.crates/diffguard-analytics/src/lib.rs (1)
163-180:⚠️ Potential issue | 🟠 MajorVersion the trend-history schema with this field widening.
TrendRunis persisted underTREND_HISTORY_SCHEMA_V1, so changinglines_scannedtou64silently changes the wire contract. A newer binary can now write av1history that an olderv1reader will fail to deserialize once the value exceedsu32::MAX. This needs a schema bump or an explicit backward-compatibility strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/diffguard-analytics/src/lib.rs` around lines 163 - 180, The struct TrendRun changed the type of lines_scanned from u32 to u64 which breaks the persisted TREND_HISTORY_SCHEMA_V1 wire contract; either introduce a new schema version constant (e.g., TREND_HISTORY_SCHEMA_V2) and update serialization/read/write paths to write the new version and keep readers able to parse v1, or revert to u32 and add an explicit migration path that converts v1 payloads to the widened shape; update all code that reads/writes trend history to branch on the schema constant and add tests exercising both v1 and the new v2 behavior (reference: TrendRun, lines_scanned, TREND_HISTORY_SCHEMA_V1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/diffguard-diff/tests/integration_octal_escaped_paths.rs`:
- Around line 7-10: The test documentation in integration_octal_escaped_paths.rs
currently references an internal casting change in unescape_git_path (u8 as u32
→ u32::from(u8)); remove that implementation-detail commentary and replace it
with a behavior-focused sentence describing what the test validates (e.g., that
octal-escaped path decoding in unescape_git_path correctly unescapes octal
sequences and preserves characters). Keep the test doc concise and high-level,
mention the function under test (unescape_git_path) and the behavior (correct
octal parsing/unescaping), but do not reference specific casts or internal
refactorings.
- Line 89: Replace the implementation-detail comment "These are boundary cases
for the u8→u32 cast" in the integration_octal_escaped_paths test with a
behavior-focused description (e.g., "Boundary cases for octal-escaped byte
values" or "Edge octal values") so the test documents expected behavior rather
than internal casting; locate the comment by searching for the exact string
"These are boundary cases for the u8→u32 cast" in
integration_octal_escaped_paths.rs and update it accordingly.
- Around line 24-31: The inline comment in integration_octal_escaped_paths.rs
that claims "Git outputs paths with spaces as `\040`" is misleading; update the
comment above the diff variable to state that real Git unified diffs use literal
spaces and that this test intentionally mixes an escaped a/ path and a literal
b/ path to exercise the parser's robustness (or alternatively, if you prefer
realistic data, change the test diff string in the diff variable so both file
paths use literal spaces consistently). Ensure the updated comment clearly marks
the mixed-escaping as an intentional edge-case test and keep references to the
diff variable when editing.
In `@crates/diffguard-domain/tests/red_tests_work_e8a88475.rs`:
- Around line 180-192: The test exactly_six_must_use_attributes_in_preprocess
currently hard-codes an expected count of 6 using PREPROCESS_SOURCE, which will
break whenever new #[must_use] helpers are added; update the assertion to assert
that must_use_count >= 6 (or compute the expected minimum from the enumerated
target signatures) and adjust the failure message accordingly so the test acts
as a lower-bound safety net rather than an upper-bound; keep references to
PREPROCESS_SOURCE and the preprocess.rs context in the message to aid debugging.
- Around line 32-61: The current text-scanning helper
find_must_use_before_function is brittle: it looks for an exact previous line
equal to "#[must_use]" and exact fn_signature substring, so additional
attributes (e.g., #[inline], #[cfg], stacked attributes) or small signature
formatting changes will break tests; replace this approach by writing a semantic
test using compile-time checks or trybuild instead of string matching—remove or
deprecate find_must_use_before_function and add a trybuild test or a
compile-fail test that compiles code invoking the target constructors and
discards their return values (or enables #[deny(unused_must_use)]), using the
real function names (the constructors under test and the fn_signature originally
passed to find_must_use_before_function) to verify #[must_use] behavior rather
than parsing source text.
In `@crates/diffguard-types/src/lib.rs`:
- Around line 433-454: The escape_md function in diffguard-types violates the
"data-only" rule; move its implementation out of this crate into diffguard-core
(or a new shared utility crate), e.g. add pub fn escape_md(s: &str) -> String in
diffguard-core, update any renderers to import the function from
diffguard_core::escape_md (or the new crate::escape_md), remove the escape_md
definition from diffguard-types, and update Cargo.toml dependencies and any
use/import paths so the project compiles and tests pass.
---
Outside diff comments:
In `@crates/diffguard-analytics/src/lib.rs`:
- Around line 163-180: The struct TrendRun changed the type of lines_scanned
from u32 to u64 which breaks the persisted TREND_HISTORY_SCHEMA_V1 wire
contract; either introduce a new schema version constant (e.g.,
TREND_HISTORY_SCHEMA_V2) and update serialization/read/write paths to write the
new version and keep readers able to parse v1, or revert to u32 and add an
explicit migration path that converts v1 payloads to the widened shape; update
all code that reads/writes trend history to branch on the schema constant and
add tests exercising both v1 and the new v2 behavior (reference: TrendRun,
lines_scanned, TREND_HISTORY_SCHEMA_V1).
In `@crates/diffguard-domain/src/preprocess.rs`:
- Around line 168-202: Add the missing #[must_use] attribute to the two factory
methods that create PreprocessOptions so all four factories consistently carry
the attribute: annotate the functions strings_only() and comments_and_strings()
with #[must_use] (they currently mirror none() and comments_only()); this will
satisfy the tests expecting exactly six must_use occurrences for the
PreprocessOptions factory methods and keep the behavior unchanged.
- Around line 273-289: Add the #[must_use] attribute to both constructor methods
so callers are warned when the returned Preprocessor is discarded: annotate the
functions Preprocessor::new and Preprocessor::with_language with #[must_use]
directly above their signatures, leaving their bodies unchanged.
In `@crates/diffguard-types/src/lib.rs`:
- Around line 125-140: The change widens DiffMeta::lines_scanned from u32 to u64
which is a breaking serialization change for the existing CHECK_SCHEMA_V1;
update the schema version constant and any places that emit/claim the schema id
so the new struct is associated with a new schema (e.g. bump CHECK_SCHEMA_V1 ->
CHECK_SCHEMA_V2 or create a new CHECK_SCHEMA_V2 constant and update
serializer/receipt writers and tests that reference CHECK_SCHEMA_V1), and ensure
any consumers that match on the schema id (deserializers, tests, docs) are
adjusted to accept the new schema id so older v1 readers are not falsely claimed
by writers emitting the widened DiffMeta.
🪄 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: 3a5f078c-e4fc-4145-a99d-858590a996b8
⛔ Files ignored due to path filters (29)
crates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_common_escapes.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_embedded_octal-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_embedded_octal.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_empty.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_max_octal_values-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_max_octal_values.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_multiple_octal.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_no_escapes.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_non_octal_digits-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_non_octal_digits.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-3.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-4.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-5.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-6.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-7.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal-8.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_single_digit_octal.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_three_digit_octal-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_three_digit_octal.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_trailing_backslash.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal-3.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal-4.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal-5.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal-6.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_two_digit_octal.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_unknown_escape-2.snapis excluded by!**/*.snapcrates/diffguard-diff/src/snapshots/diffguard_diff__unified__tests__snapshot_unescape_git_path_unknown_escape.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/diffguard-analytics/src/lib.rscrates/diffguard-core/src/render.rscrates/diffguard-diff/tests/integration_octal_escaped_paths.rscrates/diffguard-domain/src/evaluate.rscrates/diffguard-domain/src/preprocess.rscrates/diffguard-domain/tests/red_tests_work_e8a88475.rscrates/diffguard-types/src/lib.rscrates/diffguard/src/main.rs
| //! The change being tested: replacing `u8 as u32` with `u32::from(u8)` in | ||
| //! unescape_git_path's octal parsing branch. This is a lossless widening cast | ||
| //! that doesn't affect behavior, but these tests verify the full integration | ||
| //! path still works correctly. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove implementation-detail commentary from integration test documentation.
These lines describe an internal casting change (u8 as u32 → u32::from(u8)) in unescape_git_path. Integration tests should document the behavior being tested, not internal implementation details. This commentary creates confusion about the test suite's purpose.
♻️ Suggested revision
-//! The change being tested: replacing `u8 as u32` with `u32::from(u8)` in
-//! unescape_git_path's octal parsing branch. This is a lossless widening cast
-//! that doesn't affect behavior, but these tests verify the full integration
-//! path still works correctly.
+//! These tests verify that the parser correctly handles all valid octal escape
+//! sequences, including edge cases like boundary values and non-printable characters.📝 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.
| //! The change being tested: replacing `u8 as u32` with `u32::from(u8)` in | |
| //! unescape_git_path's octal parsing branch. This is a lossless widening cast | |
| //! that doesn't affect behavior, but these tests verify the full integration | |
| //! path still works correctly. | |
| //! These tests verify that the parser correctly handles all valid octal escape | |
| //! sequences, including edge cases like boundary values and non-printable characters. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-diff/tests/integration_octal_escaped_paths.rs` around lines
7 - 10, The test documentation in integration_octal_escaped_paths.rs currently
references an internal casting change in unescape_git_path (u8 as u32 →
u32::from(u8)); remove that implementation-detail commentary and replace it with
a behavior-focused sentence describing what the test validates (e.g., that
octal-escaped path decoding in unescape_git_path correctly unescapes octal
sequences and preserves characters). Keep the test doc concise and high-level,
mention the function under test (unescape_git_path) and the behavior (correct
octal parsing/unescaping), but do not reference specific casts or internal
refactorings.
| let diff = r#" | ||
| diff --git "a/path\040with spaces/file.rs" "b/path with spaces/file.rs" | ||
| --- "a/path\040with spaces/file.rs" | ||
| +++ "b/path with spaces/file.rs" | ||
| @@ -1 +1,2 @@ | ||
| fn existing() {} | ||
| +fn added() {} | ||
| "#; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Git's actual escaping behavior for paths with spaces
# Create a temporary test repo
temp_dir=$(mktemp -d)
cd "$temp_dir"
git init -q
# Create a file with spaces in the path
mkdir -p "path with spaces"
echo "original" > "path with spaces/file.txt"
git add .
git commit -q -m "initial"
# Modify the file to generate a diff
echo "modified" >> "path with spaces/file.txt"
# Show how Git formats the diff header
echo "=== Git diff header format for path with spaces ==="
git diff HEAD | grep -A2 "^diff --git"
# Cleanup
cd -
rm -rf "$temp_dir"Repository: EffortlessMetrics/diffguard
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Description: Verify Git's actual escaping behavior for paths with spaces (corrected)
# Configure git first
git config --global user.email "test@example.com"
git config --global user.name "Test User"
# Create a temporary test repo
temp_dir=$(mktemp -d)
cd "$temp_dir"
git init -q
# Create a file with spaces in the path
mkdir -p "path with spaces"
echo "original" > "path with spaces/file.txt"
git add .
git commit -q -m "initial"
# Modify the file to generate a diff
echo "modified" >> "path with spaces/file.txt"
# Show how Git formats the diff header
echo "=== Git diff header format for path with spaces ==="
git diff HEAD | head -6
# Cleanup
cd -
rm -rf "$temp_dir"Repository: EffortlessMetrics/diffguard
Length of output: 407
🏁 Script executed:
# Examine the actual test file
head -50 crates/diffguard-diff/tests/integration_octal_escaped_paths.rsRepository: EffortlessMetrics/diffguard
Length of output: 1970
🏁 Script executed:
# Check the full test file for context
cat crates/diffguard-diff/tests/integration_octal_escaped_paths.rsRepository: EffortlessMetrics/diffguard
Length of output: 4645
🏁 Script executed:
# Also check if there are other diff-related tests to understand testing patterns
fd -t f "test" crates/diffguard-diff/tests/ | head -10Repository: EffortlessMetrics/diffguard
Length of output: 53
Correct the misleading comment about Git's escaping behavior.
The inline comment at line 23 incorrectly states "Git outputs paths with spaces as \040". In reality, when Git generates a unified diff for a file with spaces in its path, it outputs literal spaces without octal escaping:
diff --git a/path with spaces/file.txt b/path with spaces/file.txtThe test itself appears designed to verify the parser's robustness in handling mixed escaping formats (a/ path escaped, b/ path literal). If that's the intent, add a comment clarifying this is intentional edge-case testing rather than realistic Git output. Otherwise, update the test data to match actual Git behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-diff/tests/integration_octal_escaped_paths.rs` around lines
24 - 31, The inline comment in integration_octal_escaped_paths.rs that claims
"Git outputs paths with spaces as `\040`" is misleading; update the comment
above the diff variable to state that real Git unified diffs use literal spaces
and that this test intentionally mixes an escaped a/ path and a literal b/ path
to exercise the parser's robustness (or alternatively, if you prefer realistic
data, change the test diff string in the diff variable so both file paths use
literal spaces consistently). Ensure the updated comment clearly marks the
mixed-escaping as an intentional edge-case test and keep references to the diff
variable when editing.
| #[test] | ||
| fn test_parse_diff_with_octal_edge_cases() { | ||
| // \177 = 127 (DEL), \000 = 0 (NUL) | ||
| // These are boundary cases for the u8→u32 cast |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove implementation-detail reference from test comment.
The comment "These are boundary cases for the u8→u32 cast" references internal implementation details. Integration tests should focus on the behavior being tested (edge octal values), not on internal casting operations.
♻️ Suggested revision
- // \177 = 127 (DEL), \000 = 0 (NUL)
- // These are boundary cases for the u8→u32 cast
+ // \177 = 127 (DEL), \000 = 0 (NUL)
+ // Edge case: octal values at the boundaries of the valid byte range📝 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.
| // These are boundary cases for the u8→u32 cast | |
| // \177 = 127 (DEL), \000 = 0 (NUL) | |
| // Edge case: octal values at the boundaries of the valid byte range |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-diff/tests/integration_octal_escaped_paths.rs` at line 89,
Replace the implementation-detail comment "These are boundary cases for the
u8→u32 cast" in the integration_octal_escaped_paths test with a behavior-focused
description (e.g., "Boundary cases for octal-escaped byte values" or "Edge octal
values") so the test documents expected behavior rather than internal casting;
locate the comment by searching for the exact string "These are boundary cases
for the u8→u32 cast" in integration_octal_escaped_paths.rs and update it
accordingly.
| fn find_must_use_before_function(source: &str, fn_signature: &str) -> Option<(bool, usize)> { | ||
| let lines: Vec<&str> = source.lines().collect(); | ||
|
|
||
| for (i, line) in lines.iter().enumerate() { | ||
| if line.contains(fn_signature) { | ||
| // Check if the previous non-empty line has #[must_use] | ||
| // We need to go back and skip empty lines and comments | ||
| let mut check_idx = i; | ||
| while check_idx > 0 { | ||
| check_idx -= 1; | ||
| let prev_line = lines[check_idx].trim(); | ||
| if prev_line.is_empty() { | ||
| continue; | ||
| } | ||
| if prev_line.starts_with("//") { | ||
| continue; | ||
| } | ||
| if prev_line.starts_with("/*") || prev_line.starts_with("*/") || prev_line.starts_with("*") { | ||
| continue; | ||
| } | ||
| // Found a meaningful previous line | ||
| let has_must_use = prev_line == "#[must_use]"; | ||
| return Some((has_must_use, i + 1)); // 1-indexed line number | ||
| } | ||
| // Function is at start of file or only preceded by comments/empty lines | ||
| return Some((false, i + 1)); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Text-based attribute scanning is brittle; consider a compile-time check instead.
find_must_use_before_function only skips blank/comment lines and then asserts the immediately preceding meaningful line equals "#[must_use]" exactly. Any future stacked attribute (e.g., #[inline], #[cfg(...)], #[allow(...)]) placed between #[must_use] and the fn signature — or even a different formatting of the attribute — will silently flip these tests to RED despite correct semantics. The substring match on fn_signature also couples the test to exact parameter spellings/whitespace (e.g., renaming opts would break it).
A compile-fail/#[deny(unused_must_use)] style test, or a trybuild fixture that calls each constructor and discards the result, would enforce the real invariant (#[must_use] behavior) rather than a textual proxy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-domain/tests/red_tests_work_e8a88475.rs` around lines 32 -
61, The current text-scanning helper find_must_use_before_function is brittle:
it looks for an exact previous line equal to "#[must_use]" and exact
fn_signature substring, so additional attributes (e.g., #[inline], #[cfg],
stacked attributes) or small signature formatting changes will break tests;
replace this approach by writing a semantic test using compile-time checks or
trybuild instead of string matching—remove or deprecate
find_must_use_before_function and add a trybuild test or a compile-fail test
that compiles code invoking the target constructors and discards their return
values (or enables #[deny(unused_must_use)]), using the real function names (the
constructors under test and the fn_signature originally passed to
find_must_use_before_function) to verify #[must_use] behavior rather than
parsing source text.
| fn exactly_six_must_use_attributes_in_preprocess() { | ||
| let must_use_count = PREPROCESS_SOURCE | ||
| .lines() | ||
| .filter(|line| line.trim() == "#[must_use]") | ||
| .count(); | ||
|
|
||
| assert_eq!( | ||
| must_use_count, 6, | ||
| "Expected exactly 6 #[must_use] attributes in preprocess.rs, but found {}. \ | ||
| The scope of this issue is limited to 6 specific functions. \ | ||
| If more #[must_use] attributes are needed, that should be a separate issue.", | ||
| must_use_count | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hard-coded count of 6 will need to be updated in two places on any future addition.
The assertion must_use_count == 6 makes adding a new #[must_use]-annotated helper to preprocess.rs fail this test even when correct, forcing contributors to find and update this unrelated test. Consider asserting >= 6 (or deriving the expected count from the set of target signatures enumerated above) so the test remains a lower-bound safety net rather than an upper-bound tripwire.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/diffguard-domain/tests/red_tests_work_e8a88475.rs` around lines 180 -
192, The test exactly_six_must_use_attributes_in_preprocess currently hard-codes
an expected count of 6 using PREPROCESS_SOURCE, which will break whenever new
#[must_use] helpers are added; update the assertion to assert that
must_use_count >= 6 (or compute the expected minimum from the enumerated target
signatures) and adjust the failure message accordingly so the test acts as a
lower-bound safety net rather than an upper-bound; keep references to
PREPROCESS_SOURCE and the preprocess.rs context in the message to aid debugging.
| // Utility for markdown escaping, used by rendering crates — kept here to avoid duplication across crates. | ||
| pub fn escape_md(s: &str) -> String { | ||
| // Escapes special Markdown characters in table cell content. | ||
| // | ||
| // Escapes pipe (`|`), backtick (`` ` ``), hash (`#`), asterisk (`*`), | ||
| // underscore (`_`), open bracket (`[`), close bracket (`]`), and greater-than | ||
| // (`>`) characters by prefixing with backslash. Also escapes CRLF (`\r\n`) | ||
| // and LF (`\n`) line endings to prevent breaking the markdown table structure. | ||
| // | ||
| // These escapes are needed to prevent breaking the markdown table structure | ||
| // and prevent unintended markdown formatting. | ||
| s.replace('|', "\\|") | ||
| .replace('`', "\\`") | ||
| .replace('#', "\\#") | ||
| .replace('*', "\\*") | ||
| .replace('_', "\\_") | ||
| .replace('[', "\\[") | ||
| .replace(']', "\\]") | ||
| .replace('>', "\\>") | ||
| .replace('\r', "\\r") | ||
| .replace('\n', "\\n") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep markdown escaping out of diffguard-types.
This helper adds presentation logic to the DTO crate, which weakens the layering boundary and makes diffguard-types a general utility dependency instead of a pure schema crate. Please move escape_md into diffguard-core or a dedicated shared utility crate and have the renderers import it from there.
As per coding guidelines, crates/diffguard-types/**/*.rs: "This crate contains only data structures with no logic - keep implementation intentionally 'dumb'".
🤖 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 433 - 454, The escape_md
function in diffguard-types violates the "data-only" rule; move its
implementation out of this crate into diffguard-core (or a new shared utility
crate), e.g. add pub fn escape_md(s: &str) -> String in diffguard-core, update
any renderers to import the function from diffguard_core::escape_md (or the new
crate::escape_md), remove the escape_md definition from diffguard-types, and
update Cargo.toml dependencies and any use/import paths so the project compiles
and tests pass.
Fix formatting violations causing CI failures (Format/Clippy/Test/xtask ci). Formatting-only changes — no logic changes.
Code Review: PR #609 — Hoist escape_md to diffguard-typesWhat's good
Critical Issue: escape_md hoisting conflicts with PR #633PR #633 (feat/work-e8a88475/diffguard-domain-must-use-candidates) makes identical changes to These two PRs should be merged together (or one should subsume the other), not merged independently. Other Concerns1. Run 2. Snapshot file spam SummaryGood refactor direction. The main action item: coordinate with PR #633 on the |
1 participant
Closes #568
Summary
Refactor escape_md from two private locations to a single public location in diffguard-types, eliminating copy-paste duplication.
ADR
ADR-057: Hoist escape_md to diffguard-types
What Changed
Test Results
Implementation committed by code-builder. Tests to be confirmed at next gate.
Notes
Draft PR - not ready for review until GREEN tests confirmed