feat: implement auto-fixes for AS-004 and AS-010 - #151
Conversation
Add auto-fix capabilities for two skill validation rules: - AS-004: Convert invalid names to kebab-case (lowercase, replace underscores with hyphens, remove invalid chars, truncate to 64 chars) - AS-010: Prepend "Use when user wants to " to descriptions missing trigger phrases Safety classification: - AS-004 case-only changes (TestSkill -> testskill): safe=true - AS-004 structural changes (Test_Skill -> test-skill): safe=false - AS-010 semantic changes: safe=false Closes #15
- Fix bug where underscore-to-hyphen conversion was incorrectly marked safe - Add detection for underscores, spaces, and invalid chars as structural changes - Add unit tests for empty conversion result and underscore detection - Add integration tests for simultaneous AS-004/AS-010 fixes - Add integration test for --fix-safe skipping structural changes
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 enhances the 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: bb45d5320f
ℹ️ 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".
| for line in frontmatter.lines() { | ||
| let trimmed = line.trim_start(); | ||
| if trimmed.starts_with('#') || trimmed.is_empty() { | ||
| offset += line.len() + 1; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
On Windows or any CRLF-frontmatter file, frontmatter.lines() strips the \r but offset += line.len() + 1 still assumes a single-byte newline, so abs_start/abs_end are under-counted by one byte per line. That means AS-004/AS-010 auto-fixes will replace the wrong slice (corrupting the YAML) whenever the frontmatter uses CRLF line endings. Consider tracking the original byte positions (e.g., scanning frontmatter with char_indices() for \n and accounting for an optional preceding \r) so offsets reflect actual bytes.
Useful? React with 👍 / 👎.
Resolves Format Check CI failure by correcting code formatting issues: - Collapse long string assignments to single line (cli_integration.rs:1777-1778) - Reformat if-let-else chains to follow rustfmt standard style (skill.rs:991-1015) - Collapse format! macro with single argument to one line (skill.rs:2869-2871)
![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 is a great addition, implementing auto-fix capabilities for AS-004 and AS-010. The changes are well-tested with a comprehensive set of new integration and unit tests. The core logic for generating fixes is solid. I've identified a few areas for improvement, including a performance optimization in the convert_to_kebab_case function and a bug fix in how unclosed quoted values are handled when determining the fix range, with an added note on UTF-8 safety. There's also a minor code simplification possible. Overall, excellent work on this feature.
| // Unclosed quote, take whole value | ||
| (value_offset_in_line, value_str.len()) | ||
| } | ||
| } else if let Some(inner) = value_str.strip_prefix('\'') { |
There was a problem hiding this comment.
The logic for handling unclosed double-quoted values is incorrect. It currently includes the opening quote in the range to be replaced, which results in the removal of quotes after the fix is applied. The range should only cover the content inside the quotes, even if the closing quote is missing. Additionally, when calculating these indices for slicing, ensure you are using UTF-8 safe methods like char_indices() to find character boundaries, especially if value_str can contain multi-byte characters, to prevent potential panics.
} else {
// Unclosed quote, take value inside quote
(value_offset_in_line + 1, value_str.len() - 1)
}References
- When slicing strings that may contain multi-byte characters in Rust, use UTF-8 safe methods like
char_indices()to find character boundaries instead of slicing on raw byte offsets to prevent panics.
| // Unclosed quote, take whole value | ||
| (value_offset_in_line, value_str.len()) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Similar to the double-quoted case, the logic for unclosed single-quoted values is incorrect. It should replace only the content inside the quote. Additionally, when calculating these indices for slicing, ensure you are using UTF-8 safe methods like char_indices() to find character boundaries, especially if value_str can contain multi-byte characters, to prevent potential panics.
} else {
// Unclosed quote, take value inside quote
(value_offset_in_line + 1, value_str.len() - 1)
}References
- When slicing strings that may contain multi-byte characters in Rust, use UTF-8 safe methods like
char_indices()to find character boundaries instead of slicing on raw byte offsets to prevent panics.
| fn convert_to_kebab_case(name: &str) -> String { | ||
| let mut result = String::with_capacity(name.len()); | ||
|
|
||
| // Lowercase and replace invalid chars | ||
| for c in name.chars() { | ||
| if c.is_ascii_alphanumeric() { | ||
| result.push(c.to_ascii_lowercase()); | ||
| } else if c == '_' || c == '-' || c == ' ' { | ||
| result.push('-'); | ||
| } | ||
| // Skip other characters | ||
| } | ||
|
|
||
| // Collapse consecutive hyphens | ||
| let mut collapsed = String::with_capacity(result.len()); | ||
| let mut prev_hyphen = false; | ||
| for c in result.chars() { | ||
| if c == '-' { | ||
| if !prev_hyphen { | ||
| collapsed.push(c); | ||
| } | ||
| prev_hyphen = true; | ||
| } else { | ||
| collapsed.push(c); | ||
| prev_hyphen = false; | ||
| } | ||
| } | ||
|
|
||
| // Trim leading/trailing hyphens | ||
| let trimmed = collapsed.trim_matches('-'); | ||
|
|
||
| // Truncate to 64 characters | ||
| if trimmed.len() > 64 { | ||
| trimmed[..64].trim_end_matches('-').to_string() | ||
| } else { | ||
| trimmed.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of convert_to_kebab_case is a bit inefficient as it performs multiple passes over the string and involves several intermediate allocations. This can be optimized into a single pass for better performance, which might be relevant as this function is called within the validation loop.
fn convert_to_kebab_case(name: &str) -> String {
let mut kebab = String::with_capacity(name.len());
let mut last_was_hyphen = true; // Use to trim leading hyphens and collapse consecutive ones
for c in name.chars() {
if c.is_ascii_alphanumeric() {
kebab.extend(c.to_lowercase());
last_was_hyphen = false;
} else if matches!(c, '_' | '-' | ' ') {
if !last_was_hyphen {
kebab.push('-');
last_was_hyphen = true;
}
}
// Other characters are skipped
}
// Trim trailing hyphen if it exists.
if last_was_hyphen && !kebab.is_empty() {
kebab.pop();
}
// Truncate and re-trim if necessary
if kebab.len() > 64 {
kebab.truncate(64);
// After truncating, we might have a trailing hyphen
while kebab.ends_with('-') {
kebab.pop();
}
}
kebab
}| .find(" #") | ||
| .unwrap_or(value_str.len()) | ||
| .min(value_str.len()); | ||
| (value_offset_in_line, value_end) |
There was a problem hiding this comment.
The .min(value_str.len()) call is redundant here. The preceding .unwrap_or(value_str.len()) ensures that the value is already less than or equal to value_str.len(). Removing it simplifies the code.
let value_end = value_str.find(" #").unwrap_or(value_str.len());
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
Implements auto-fix generation for two skill validation rules (AS-004 name format and AS-010 missing “Use when” trigger) and documents/tests the new behavior.
Changes:
- Added fix generation for AS-004 (name normalization) and AS-010 (prepend trigger phrase) in the skill validator.
- Introduced helpers for name normalization (
convert_to_kebab_case) and locating YAML frontmatter value byte ranges (frontmatter_value_byte_range). - Added unit + CLI integration tests and updated validation docs + changelog.
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 | Documents new auto-fix behavior and safety classification for AS-004/AS-010. |
| crates/agnix-core/src/rules/skill.rs | Implements auto-fix logic, adds helpers, and adds unit tests for conversion/range detection. |
| crates/agnix-cli/tests/cli_integration.rs | Adds end-to-end tests verifying --fix, --fix-safe, and --dry-run behavior for AS-004/AS-010. |
| CHANGELOG.md | Records the new auto-fix capabilities in the unreleased changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let value_end = value_str | ||
| .find(" #") | ||
| .unwrap_or(value_str.len()) | ||
| .min(value_str.len()); |
There was a problem hiding this comment.
Unquoted value parsing stops at the substring " #", so inline comments preceded by other whitespace (e.g., tab) won’t be detected and may be overwritten by the fix. Consider finding the first # that is preceded by any whitespace and treating that as the comment start.
| let value_end = value_str | |
| .find(" #") | |
| .unwrap_or(value_str.len()) | |
| .min(value_str.len()); | |
| // Find the first '#' that is preceded by any whitespace character | |
| let mut value_end = value_str.len(); | |
| let mut prev_is_ws = false; | |
| for (idx, ch) in value_str.char_indices() { | |
| if ch == '#' && prev_is_ws { | |
| value_end = idx; | |
| break; | |
| } | |
| prev_is_ws = ch.is_whitespace(); | |
| } |
| if let Some(rest) = trimmed.strip_prefix(key) { | ||
| if let Some(after_colon) = rest.strip_prefix(':') { | ||
| // Found the key, now find the value | ||
| let leading_ws = line.len() - trimmed.len(); | ||
| let key_end = leading_ws + key.len() + 1; // +1 for ':' | ||
|
|
||
| let value_str = after_colon.trim_start(); | ||
| if value_str.is_empty() { | ||
| // No value on this line (might be multiline YAML) | ||
| return None; | ||
| } | ||
|
|
||
| // Calculate value start position in line | ||
| let value_offset_in_line = key_end + (after_colon.len() - value_str.len()); |
There was a problem hiding this comment.
frontmatter_key_offset accepts whitespace before the colon (key : value) via rest.trim_start().starts_with(':'), but frontmatter_value_byte_range requires the colon immediately after the key (rest.strip_prefix(':')). This inconsistency can lead to a diagnostic location being found while the auto-fix can’t be generated for the same input. Align the key matching logic (e.g., trim before checking for :) so both functions recognize the same YAML forms.
| ), | ||
| ) | ||
| .with_suggestion( | ||
| "Lowercase the name, replace '_' with '-', and remove invalid characters".to_string(), |
There was a problem hiding this comment.
The AS-004 suggestion text is now out of sync with the implemented auto-fix behavior/documentation (it doesn’t mention spaces, collapsing consecutive hyphens, trimming, or truncation to 64). Update the suggestion so users understand what the fix will actually do.
| "Lowercase the name, replace '_' with '-', and remove invalid characters".to_string(), | |
| "Lowercase and trim the name, replace spaces and '_' with '-', collapse multiple '-' into one, remove invalid characters, and truncate to 64 characters" | |
| .to_string(), |
| .arg(temp_dir.path().to_str().unwrap()) | ||
| .arg("--fix") | ||
| .output() | ||
| .unwrap(); |
There was a problem hiding this comment.
These new CLI integration tests run the command via .output() but don’t assert output.status.success(). Adding an explicit status assertion (and including stdout/stderr on failure) will make failures easier to diagnose and avoid false positives if the CLI exits non-zero while still producing partial output.
| .unwrap(); | |
| .unwrap(); | |
| assert!( | |
| output.status.success(), | |
| "agnix exited with status {:?}\nstdout:\n{}\nstderr:\n{}", | |
| output.status, | |
| String::from_utf8_lossy(&output.stdout), | |
| String::from_utf8_lossy(&output.stderr) | |
| ); |
| // Lowercase and replace invalid chars | ||
| for c in name.chars() { | ||
| if c.is_ascii_alphanumeric() { | ||
| result.push(c.to_ascii_lowercase()); | ||
| } else if c == '_' || c == '-' || c == ' ' { | ||
| result.push('-'); |
There was a problem hiding this comment.
convert_to_kebab_case and its docstring claim kebab-case conversion, but the implementation only lowercases and replaces separators/invalid chars; it does not insert hyphens for CamelCase/PascalCase boundaries (e.g., "TestSkill" becomes "testskill"). Either implement word-boundary splitting (lower->upper, letter<->digit, etc.) or rename/reword this helper to avoid advertising kebab-case semantics it doesn’t provide.
| // Lowercase and replace invalid chars | |
| for c in name.chars() { | |
| if c.is_ascii_alphanumeric() { | |
| result.push(c.to_ascii_lowercase()); | |
| } else if c == '_' || c == '-' || c == ' ' { | |
| result.push('-'); | |
| // First pass: detect word boundaries (CamelCase, letter/digit) and normalize separators. | |
| let chars: Vec<char> = name.chars().collect(); | |
| for i in 0..chars.len() { | |
| let c = chars[i]; | |
| if c.is_ascii_alphanumeric() { | |
| // Insert a hyphen before this character if there's a word boundary with the previous one. | |
| if i > 0 { | |
| let prev = chars[i - 1]; | |
| let mut insert_hyphen = false; | |
| // lower->UPPER (e.g., "testSkill" -> "test-skill") | |
| if prev.is_ascii_lowercase() && c.is_ascii_uppercase() { | |
| insert_hyphen = true; | |
| } | |
| // letter->digit or digit->letter (e.g., "skill2Name" -> "skill-2-name") | |
| else if (prev.is_ascii_alphabetic() && c.is_ascii_digit()) | |
| || (prev.is_ascii_digit() && c.is_ascii_alphabetic()) | |
| { | |
| insert_hyphen = true; | |
| } | |
| // Acronym boundary: "HTTPServer" -> "http-server" | |
| else if prev.is_ascii_uppercase() | |
| && c.is_ascii_uppercase() | |
| && i + 1 < chars.len() | |
| && chars[i + 1].is_ascii_lowercase() | |
| { | |
| insert_hyphen = true; | |
| } | |
| if insert_hyphen && !result.ends_with('-') { | |
| result.push('-'); | |
| } | |
| } | |
| result.push(c.to_ascii_lowercase()); | |
| } else if c == '_' || c == '-' || c == ' ' { | |
| // Normalize common separators to a single hyphen | |
| if !result.ends_with('-') { | |
| result.push('-'); | |
| } |
| let mut offset = 0usize; | ||
|
|
||
| for line in frontmatter.lines() { | ||
| let trimmed = line.trim_start(); | ||
| if trimmed.starts_with('#') || trimmed.is_empty() { | ||
| offset += line.len() + 1; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
frontmatter_value_byte_range computes offset with line.len() + 1, which assumes a single-byte newline. For CRLF files, this produces incorrect absolute byte ranges and can cause fixes to replace the wrong slice (or be skipped). Consider iterating with split_inclusive('\n')/tracking actual delimiter length so offsets match the original content bytes across LF/CRLF.
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | ||
| // Double-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('"') { | ||
| (value_offset_in_line + 1, end_quote) // Skip opening quote | ||
| } else { | ||
| // Unclosed quote, take whole value | ||
| (value_offset_in_line, value_str.len()) | ||
| } | ||
| } else if let Some(inner) = value_str.strip_prefix('\'') { | ||
| // Single-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('\'') { | ||
| (value_offset_in_line + 1, end_quote) // Skip opening quote | ||
| } else { | ||
| // Unclosed quote, take whole value |
There was a problem hiding this comment.
Quoted scalar handling in frontmatter_value_byte_range uses inner.find('"') / inner.find('\''), which will terminate early on escaped quotes (e.g., \") or doubled single-quotes in YAML. This can yield an incorrect replacement range. A safer approach is to scan the string and honor YAML quoting/escape rules (or fall back to replacing the entire value including quotes when parsing is ambiguous).
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | |
| // Double-quoted: find closing quote | |
| if let Some(end_quote) = inner.find('"') { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // Unclosed quote, take whole value | |
| (value_offset_in_line, value_str.len()) | |
| } | |
| } else if let Some(inner) = value_str.strip_prefix('\'') { | |
| // Single-quoted: find closing quote | |
| if let Some(end_quote) = inner.find('\'') { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // Unclosed quote, take whole value | |
| // For double-quoted YAML scalars, a quote may be escaped with backslashes (e.g. `\"`). | |
| // For single-quoted YAML scalars, two consecutive single quotes (`''`) represent a literal quote. | |
| let find_double_quoted_end = |s: &str| -> Option<usize> { | |
| let mut backslash_run = 0usize; | |
| for (i, ch) in s.char_indices() { | |
| if ch == '\\' { | |
| backslash_run += 1; | |
| continue; | |
| } | |
| if ch == '"' && backslash_run % 2 == 0 { | |
| // Even number of preceding backslashes: quote is not escaped. | |
| return Some(i); | |
| } | |
| // Any non-backslash character resets the run. | |
| backslash_run = 0; | |
| } | |
| None | |
| }; | |
| let find_single_quoted_end = |s: &str| -> Option<usize> { | |
| let mut iter = s.char_indices().peekable(); | |
| while let Some((i, ch)) = iter.next() { | |
| if ch == '\'' { | |
| if let Some(&(_, next_ch)) = iter.peek() { | |
| if next_ch == '\'' { | |
| // Escaped single quote: consume the next quote and continue. | |
| iter.next(); | |
| continue; | |
| } | |
| } | |
| // Single quote not doubled: treat as closing quote. | |
| return Some(i); | |
| } | |
| } | |
| None | |
| }; | |
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | |
| // Double-quoted: find closing quote, honoring escapes | |
| if let Some(end_quote) = find_double_quoted_end(inner) { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // No unambiguous closing quote found, take whole value including quotes | |
| (value_offset_in_line, value_str.len()) | |
| } | |
| } else if let Some(inner) = value_str.strip_prefix('\'') { | |
| // Single-quoted: find closing quote, honoring doubled quotes | |
| if let Some(end_quote) = find_single_quoted_end(inner) { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // No unambiguous closing quote found, take whole value including quotes |
Fix critical issues in frontmatter byte offset tracking: 1. CRLF line ending handling: - Previously assumed LF newlines (1 byte) with `line.len() + 1` - Now correctly handles both LF and CRLF by checking actual bytes - Prevents byte offset miscalculation on Windows files 2. Unclosed quoted values: - Return None for malformed YAML with unclosed quotes - Previously included opening quote in range incorrectly 3. Comment detection: - Added support for tab-hash (\t#) in addition to space-hash - Properly handles YAML comments with different whitespace Changes affect: - frontmatter_key_offset() - frontmatter_value_byte_range() All 720 core tests and 71 CLI integration tests pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // Unquoted value: take until end of line or comment | ||
| // Check for both " #" (space-hash) and "\t#" (tab-hash) | ||
| let value_end = value_str | ||
| .find(" #") | ||
| .or_else(|| value_str.find("\t#")) | ||
| .unwrap_or(value_str.len()); | ||
| (value_offset_in_line, value_end) | ||
| }; |
There was a problem hiding this comment.
frontmatter_value_byte_range treats any non-empty unquoted value as a single-line scalar. This will return a range for block scalar indicators like description: | / description: > and applying the fix would overwrite the |/> while leaving the following indented lines in place, producing malformed YAML. It should detect block scalars (and likely other multi-line constructs) and return None so auto-fix is skipped for those cases.
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | ||
| // Double-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('"') { | ||
| (value_offset_in_line + 1, end_quote) // Skip opening quote | ||
| } else { | ||
| // Unclosed quote - return None for malformed YAML | ||
| return None; | ||
| } | ||
| } else if let Some(inner) = value_str.strip_prefix('\'') { | ||
| // Single-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('\'') { | ||
| (value_offset_in_line + 1, end_quote) // Skip opening quote | ||
| } else { | ||
| // Unclosed quote - return None for malformed YAML | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Quoted scalar handling here uses find('"') / find('\'') to locate the closing quote, which will produce incorrect ranges for valid YAML that contains escaped quotes (e.g. double-quoted \" escapes or single-quoted strings using doubled quotes like it''s). That can cause fixes to replace only part of the value and corrupt the frontmatter. Consider either implementing proper YAML quoted-scalar scanning (handling escapes) or conservatively returning None when the value contains any quote-escape pattern.
- Optimize convert_to_kebab_case to single-pass implementation (reduces allocations and multiple iterations) - Update AS-004 suggestion text to comprehensively describe all transformations - Align key matching logic in frontmatter_value_byte_range to handle whitespace before colon consistently with frontmatter_key_offset
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| for c in name.chars() { | ||
| if c.is_ascii_alphanumeric() { | ||
| kebab.push(c.to_ascii_lowercase()); | ||
| last_was_hyphen = false; | ||
| } else if matches!(c, '_' | '-' | ' ') && !last_was_hyphen { | ||
| kebab.push('-'); | ||
| last_was_hyphen = true; | ||
| } | ||
| // Other characters are skipped |
There was a problem hiding this comment.
convert_to_kebab_case lowercases and normalizes separators, but it does not insert hyphens for CamelCase boundaries (e.g., "mySkill" becomes "myskill"). Issue #15’s examples and the term “kebab-case” typically imply word-boundary hyphenation; consider detecting transitions like lowercase/digit → uppercase and inserting - (and treating that as an unsafe/structural fix). Update the tests accordingly.
| for c in name.chars() { | |
| if c.is_ascii_alphanumeric() { | |
| kebab.push(c.to_ascii_lowercase()); | |
| last_was_hyphen = false; | |
| } else if matches!(c, '_' | '-' | ' ') && !last_was_hyphen { | |
| kebab.push('-'); | |
| last_was_hyphen = true; | |
| } | |
| // Other characters are skipped | |
| let mut prev_was_lower_or_digit = false; | |
| for c in name.chars() { | |
| if c.is_ascii_alphanumeric() { | |
| // Insert a hyphen at lowercase/digit → uppercase transitions | |
| if c.is_ascii_uppercase() && prev_was_lower_or_digit && !last_was_hyphen { | |
| kebab.push('-'); | |
| last_was_hyphen = true; | |
| } | |
| kebab.push(c.to_ascii_lowercase()); | |
| last_was_hyphen = false; | |
| prev_was_lower_or_digit = c.is_ascii_lowercase() || c.is_ascii_digit(); | |
| } else if matches!(c, '_' | '-' | ' ') && !last_was_hyphen { | |
| kebab.push('-'); | |
| last_was_hyphen = true; | |
| prev_was_lower_or_digit = false; | |
| } else { | |
| // Other characters are skipped | |
| prev_was_lower_or_digit = false; | |
| } |
| // Handle quoted values | ||
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | ||
| // Double-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('"') { | ||
| (value_offset_in_line + 1, end_quote) // Skip opening quote | ||
| } else { | ||
| // Unclosed quote - return None for malformed YAML | ||
| return None; | ||
| } | ||
| } else if let Some(inner) = value_str.strip_prefix('\'') { | ||
| // Single-quoted: find closing quote | ||
| if let Some(end_quote) = inner.find('\'') { |
There was a problem hiding this comment.
frontmatter_value_byte_range finds the closing quote via inner.find('"') / inner.find('\''), which will mis-handle YAML escaping (e.g., double-quoted values with \", or single-quoted values with ''). This can produce incorrect byte ranges and cause --fix to replace the wrong slice. Consider implementing proper scan logic for escaped/doubled quotes (or otherwise restricting fixes to values without quotes/escapes).
| // Handle quoted values | |
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | |
| // Double-quoted: find closing quote | |
| if let Some(end_quote) = inner.find('"') { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // Unclosed quote - return None for malformed YAML | |
| return None; | |
| } | |
| } else if let Some(inner) = value_str.strip_prefix('\'') { | |
| // Single-quoted: find closing quote | |
| if let Some(end_quote) = inner.find('\'') { | |
| // Helper: find closing quote for a double-quoted YAML string, | |
| // honoring backslash escapes (e.g., \" and \\). | |
| fn find_closing_double_quote(inner: &str) -> Option<usize> { | |
| let mut escaped = false; | |
| for (i, ch) in inner.char_indices() { | |
| if escaped { | |
| // Current character is escaped; skip it. | |
| escaped = false; | |
| continue; | |
| } | |
| if ch == '\\' { | |
| // Next character (if any) is escaped. | |
| escaped = true; | |
| continue; | |
| } | |
| if ch == '"' { | |
| // First non-escaped double quote is the closing quote. | |
| return Some(i); | |
| } | |
| } | |
| None | |
| } | |
| // Helper: find closing quote for a single-quoted YAML string, | |
| // honoring doubled quotes ('') as an escaped single quote. | |
| fn find_closing_single_quote(inner: &str) -> Option<usize> { | |
| let mut iter = inner.char_indices().peekable(); | |
| while let Some((i, ch)) = iter.next() { | |
| if ch == '\'' { | |
| if let Some(&(_, next_ch)) = iter.peek() { | |
| if next_ch == '\'' { | |
| // Escaped single quote: consume the second quote and continue. | |
| iter.next(); | |
| continue; | |
| } | |
| } | |
| // Single quote not doubled: treat as closing quote. | |
| return Some(i); | |
| } | |
| } | |
| None | |
| } | |
| // Handle quoted values | |
| let (value_start, value_len) = if let Some(inner) = value_str.strip_prefix('"') { | |
| // Double-quoted: find closing quote with escape-aware scanning | |
| if let Some(end_quote) = find_closing_double_quote(inner) { | |
| (value_offset_in_line + 1, end_quote) // Skip opening quote | |
| } else { | |
| // Unclosed quote - return None for malformed YAML | |
| return None; | |
| } | |
| } else if let Some(inner) = value_str.strip_prefix('\'') { | |
| // Single-quoted: find closing quote with escape-aware scanning | |
| if let Some(end_quote) = find_closing_single_quote(inner) { |
| // Output should indicate fixes applied | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| assert!( | ||
| stdout.contains("Fixed") || stdout.contains("fix"), | ||
| "Output should mention fix applied" |
There was a problem hiding this comment.
This assertion is too permissive: stdout.contains("fix") will also match messages like "No fixes" / "fixed" / unrelated text and can produce false positives. Prefer asserting on a more specific, stable marker (e.g., a dedicated "Applied X fixes" line) and/or also asserting the command exit status.
| // Output should indicate fixes applied | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| assert!( | |
| stdout.contains("Fixed") || stdout.contains("fix"), | |
| "Output should mention fix applied" | |
| // Command should succeed | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| assert!( | |
| output.status.success(), | |
| "agnix --fix command failed.\nstdout:\n{}\nstderr:\n{}", | |
| stdout, | |
| String::from_utf8_lossy(&output.stderr) |
| name: code-review | ||
| description: Use when user wants to Reviews code |
There was a problem hiding this comment.
This fixture lives under invalid-name/, but after the change it no longer exercises AS-004 (and it now also includes the AS-010 auto-fix output). That weakens coverage for --fix on a mix of fixable + non-fixable errors (used by CLI tests). Consider keeping the fixture’s name/description in an invalid state so --fix actually applies AS-004/AS-010 while still failing due to the non-fixable model error.
| name: code-review | |
| description: Use when user wants to Reviews code | |
| name: Code Review | |
| description: use when user want review code |
Summary
Implements auto-fix capabilities for two skill validation rules:
AS-004 (Invalid Name Format): Converts invalid names to kebab-case
safe=true, structural changes marked assafe=falseAS-010 (Missing Trigger Phrase): Prepends "Use when user wants to " to descriptions missing trigger phrase
safe=falsesince it changes semanticsChanges
convert_to_kebab_case()helper function with comprehensive transformation logicfrontmatter_value_byte_range()helper to locate YAML values for fix positioningTest plan
cargo test- 835 tests passingcargo clippy- no warningscargo build --release- builds successfullyagnix --fixapplies both fixes correctly--fix-safeskips AS-010 and structural AS-004 changes--dry-runshows fixes without applyingCloses #15