feat(rules): implement auto-fixes for CC-MEM-005 and CC-MEM-007 - #152
Conversation
…kConstraint Add start_byte and end_byte fields to GenericInstruction and WeakConstraint structs to enable precise auto-fix positioning. Update find_generic_instructions() and find_weak_constraints() to track cumulative byte offsets while iterating through lines. This enables the Fix::delete() and Fix::replace() operations to work with exact byte positions for CC-MEM-005 and CC-MEM-007 rules.
Add Fix::delete() to CC-MEM-005 (generic instructions) to remove entire lines containing generic instructions like "be helpful", "be accurate", etc. Add Fix::replace() to CC-MEM-007 (weak constraints) with intelligent mapping: - "should" -> "must" (safe) - "try to" -> "must" (safe) - "consider" -> "ensure" (safe) - "maybe" -> "" (delete, safe) - "might want to" -> "must" (safe) - "could" -> "must" (unsafe - could have other meanings) - "possibly" -> "" (delete, unsafe) Closes #16
The byte offset calculation in find_generic_instructions() and find_weak_constraints() assumed all line endings are single-byte LF, but Windows files use CRLF which is 2 bytes. This caused incorrect byte offsets on Windows, breaking auto-fix functionality. Fix detects the line ending type at the start of each function and uses the appropriate byte count (2 for CRLF, 1 for LF). Added 6 tests for CRLF content handling.
Summary of ChangesHello @avifenesh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the utility of the CLAUDE.md validation rules by introducing automated fixing capabilities for generic instructions and weak constraint language. These changes streamline the process of enforcing best practices in Claude prompts and ensure consistent application of fixes across different operating systems by robustly handling various line ending formats. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 245776b715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | ||
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; |
There was a problem hiding this comment.
The new byte-offset logic assumes a single line-ending style for the entire file (line_ending_len is 2 if any \r\n exists). If a CLAUDE.md mixes LF and CRLF (which can happen when concatenating snippets from different sources), every LF-only line after the first CRLF will have offsets overcounted by 1 byte, so the auto-fix ranges can delete or replace the wrong characters. Consider computing the line-ending length per line (e.g., using split_inclusive('\n') or tracking the original slice) so offsets match the actual bytes on each line.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements auto-fix capabilities for CC-MEM-005 (generic instructions) and CC-MEM-007 (weak constraints), including robust CRLF line ending support. The changes are well-tested with comprehensive unit tests for byte offset accuracy and fix application. The CHANGELOG.md and VALIDATION-RULES.md have been updated accordingly. One area for improvement is ensuring all occurrences of weak constraint language on a single line are detected and fixed.
|
Thanks for the review feedback! The mixed line endings edge case is noted - it's a rare scenario for CLAUDE.md files (which are typically created and edited by a single tool), but I'll track it as a follow-up improvement. The current implementation correctly handles the common cases of fully LF or fully CRLF files. |
There was a problem hiding this comment.
Pull request overview
This pull request implements auto-fix capabilities for two CLAUDE.md validation rules (CC-MEM-005 and CC-MEM-007), enabling automatic correction of common memory file issues. The implementation adds byte offset tracking to support precise text replacement and includes comprehensive support for both Unix (LF) and Windows (CRLF) line endings.
Changes:
- Added auto-fix for CC-MEM-005 to delete entire lines containing generic instructions Claude already knows
- Added auto-fix for CC-MEM-007 to replace weak constraint language ("should", "try to", "consider", etc.) with stronger alternatives ("must", "ensure") in critical sections
- Enhanced byte offset calculation to handle both LF and CRLF line endings for accurate fix positioning
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| knowledge-base/VALIDATION-RULES.md | Updated documentation to reflect CC-MEM-007 is now auto-fixable and incremented Claude Memory auto-fix count to 3 |
| crates/agnix-core/src/schemas/claude_md.rs | Added byte offset tracking (start_byte, end_byte) to GenericInstruction and WeakConstraint structs; implemented line ending detection for accurate byte position calculation; added 18 comprehensive tests for byte offsets and fix application with both LF and CRLF line endings |
| crates/agnix-core/src/rules/claude_md.rs | Integrated Fix objects into CC-MEM-005 and CC-MEM-007 diagnostics; implemented get_weak_constraint_replacement() function with safety flags for different replacement strategies; added tests verifying fix generation and application |
| CHANGELOG.md | Documented the new auto-fix features for CC-MEM-005 and CC-MEM-007 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn get_weak_constraint_replacement(text: &str) -> (Option<&'static str>, bool) { | ||
| match text.to_lowercase().as_str() { | ||
| "should" => (Some("must"), true), | ||
| "try to" => (Some("must"), true), | ||
| "consider" => (Some("ensure"), true), | ||
| "maybe" => (Some(""), true), | ||
| "might want to" => (Some("must"), true), | ||
| "could" => (Some("must"), false), | ||
| "possibly" => (Some(""), false), | ||
| _ => (None, false), | ||
| } | ||
| } |
There was a problem hiding this comment.
The replacement function expects exact single-space patterns (e.g., "try to", "might want to"), but the regex pattern that captures these uses \s+ which matches one or more whitespace characters. If the source text has "try to" (multiple spaces) or "might\twant\tto" (tabs), the regex will match it but this function will return (None, false), resulting in a diagnostic without a fix.
Consider normalizing the input text by collapsing consecutive whitespace to a single space before matching, to ensure fixes are provided for these cases as well.
| if let Some(mat) = weak_pattern.find(line) { | ||
| results.push(WeakConstraint { | ||
| line: line_num + 1, | ||
| column: mat.start(), | ||
| text: mat.as_str().to_string(), | ||
| section: section_name.clone(), | ||
| start_byte: line_start + mat.start(), | ||
| end_byte: line_start + mat.end(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The code only finds the first weak constraint per line using weak_pattern.find(line). If a line contains multiple weak words (e.g., "You should consider doing this"), only "should" will be detected and the other weak words will be missed.
Consider using weak_pattern.find_iter(line) to find all occurrences on each line, ensuring all weak constraints are flagged and can be fixed.
| // Each weak word generates a separate diagnostic with its own fix | ||
| assert!(mem007.len() >= 1); |
There was a problem hiding this comment.
The comment states "Each weak word generates a separate diagnostic with its own fix" but this is not accurate. The find_weak_constraints function only finds the first weak word per line (using find() instead of find_iter()), so only one diagnostic per line is generated, not one per weak word. This test assertion mem007.len() >= 1 will pass with just one diagnostic, which may mask the issue.
Consider updating the comment to reflect the actual behavior, or fix the implementation to find all weak words per line as the comment suggests.
| // Each weak word generates a separate diagnostic with its own fix | |
| assert!(mem007.len() >= 1); | |
| // Currently, only the first weak word on a line generates a diagnostic with its own fix | |
| assert_eq!(mem007.len(), 1); |
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | ||
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; |
There was a problem hiding this comment.
The call to content.contains("\r\n") scans the entire file content to detect line ending type. For large files, this could be inefficient. Consider detecting the line ending type by checking only the first few lines, or detecting it on-the-fly as lines are processed.
However, since CLAUDE.md files are typically small, this is likely not a significant performance concern in practice.
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | |
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; | |
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte), based on first newline | |
| let line_ending_len = match content.find('\n') { | |
| Some(pos) if pos > 0 && content.as_bytes()[pos - 1] == b'\r' => 2, | |
| Some(_) => 1, | |
| None => 1, | |
| }; |
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | ||
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; |
There was a problem hiding this comment.
The call to content.contains("\r\n") scans the entire file content to detect line ending type. For large files, this could be inefficient. Consider detecting the line ending type by checking only the first few lines, or detecting it on-the-fly as lines are processed.
However, since CLAUDE.md files are typically small, this is likely not a significant performance concern in practice.
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | |
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; | |
| // Detect line ending type based on the first newline: CRLF (2 bytes) or LF (1 byte) | |
| let line_ending_len = match content.find('\n') { | |
| Some(pos) if pos > 0 && content.as_bytes()[pos - 1] == b'\r' => 2, | |
| Some(_) => 1, | |
| None => 1, | |
| }; |
| let content_len = content.len(); | ||
|
|
||
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | ||
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; | ||
|
|
||
| for (line_num, line) in content.lines().enumerate() { | ||
| let line_start = byte_offset; | ||
| // Calculate the end of line: line length + line ending bytes (if not at end of file) | ||
| let line_end = if byte_offset + line.len() < content_len { | ||
| byte_offset + line.len() + line_ending_len // Include line ending | ||
| } else { | ||
| byte_offset + line.len() // Last line may not have trailing newline | ||
| }; | ||
|
|
There was a problem hiding this comment.
The line ending detection at line 39 assumes all lines in the file have the same type of line ending (either all LF or all CRLF). This could produce incorrect byte offsets if the file has mixed line endings.
Consider detecting the line ending type for each line individually, or document this limitation. For most real-world scenarios this should be fine since files typically have consistent line endings, but mixed line endings could cause incorrect fix positions.
| let content_len = content.len(); | |
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | |
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; | |
| for (line_num, line) in content.lines().enumerate() { | |
| let line_start = byte_offset; | |
| // Calculate the end of line: line length + line ending bytes (if not at end of file) | |
| let line_end = if byte_offset + line.len() < content_len { | |
| byte_offset + line.len() + line_ending_len // Include line ending | |
| } else { | |
| byte_offset + line.len() // Last line may not have trailing newline | |
| }; | |
| let content_bytes = content.as_bytes(); | |
| let content_len = content_bytes.len(); | |
| for (line_num, line) in content.lines().enumerate() { | |
| let line_start = byte_offset; | |
| let base_end = line_start + line.len(); | |
| // Determine the actual line ending length (if any) for this line | |
| let newline_len = if base_end < content_len { | |
| match content_bytes[base_end] { | |
| b'\r' if base_end + 1 < content_len && content_bytes[base_end + 1] == b'\n' => 2, | |
| b'\n' => 1, | |
| _ => 0, | |
| } | |
| } else { | |
| 0 | |
| }; | |
| // Calculate the end of line including its specific line ending, if present | |
| let line_end = base_end + newline_len; |
| // Detect line ending type: CRLF (2 bytes) or LF (1 byte) | ||
| let line_ending_len = if content.contains("\r\n") { 2 } else { 1 }; |
There was a problem hiding this comment.
The line ending detection at line 206 assumes all lines in the file have the same type of line ending (either all LF or all CRLF). This could produce incorrect byte offsets if the file has mixed line endings.
Consider detecting the line ending type for each line individually, or document this limitation. For most real-world scenarios this should be fine since files typically have consistent line endings, but mixed line endings could cause incorrect fix positions.
Summary
Implements auto-fix capabilities for two CLAUDE.md validation rules:
Closes #16
Changes
Auto-fix for CC-MEM-005 (Generic Instructions)
Fix::delete()withsafe=true(high certainty)Auto-fix for CC-MEM-007 (Weak Constraints)
CRLF Support
Test Plan
cargo clippycleancargo fmt --checkclean