F-011: refactor(services): replace byte indexing with strip_prefix#15
F-011: refactor(services): replace byte indexing with strip_prefix#15Sephyi wants to merge 1 commit intodevelopmentfrom
Conversation
Six diff-line processing sites used &line[1..] / &l[1..] to drop the
leading '+' or '-', which is safe here because the guards limit those
lines to ASCII prefixes, but strip_prefix expresses the intent directly
and removes the implicit byte-boundary assumption.
Sites updated:
- context.rs detect_import_changes: collapse the dual +/- branch into
a single strip_prefix chain that picks the action label in one pass.
- context.rs detect_intents: swap the negated starts_with guard for a
let-else over strip_prefix('+'), keeping the +++ header filter.
- context.rs detect_metadata_breaking: rewrite the is_added/is_removed
dispatch around strip_prefix, then derive is_removed from is_added.
- splitter.rs diff_fingerprint: filter_map(strip_prefix) in the
added/removed line builders.
- safety.rs scan_full_diff_with_patterns: let-chain strip_prefix('+')
alongside the existing !starts_with("+++") header guard.
Behaviorally identical; zero new allocations. Verified with
cargo fmt --check, cargo clippy --all-targets --all-features -D warnings
(clean), and cargo test --all-targets (440 passed, 1 pre-existing
porcelain timeout flake also present on clean origin/development).
Closes audit entry F-011 from #3.
There was a problem hiding this comment.
Pull request overview
Refactors diff-line parsing in services to avoid byte indexing (&line[1..]) by using strip_prefix, addressing audit entry F-011.
Changes:
- Update
CommitSplitterdiff fingerprinting to usestrip_prefixfor added/removed line extraction. - Update secret scanning over full diffs to use
strip_prefixwhen extracting added-line content. - Update
ContextBuilderdiff heuristics (import/intent/metadata detection) to usestrip_prefixinstead of byte slicing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/services/splitter.rs | Replace &l[1..] slicing with strip_prefix when collecting added/removed lines for indentation-only detection. |
| src/services/safety.rs | Replace &line[1..] slicing with strip_prefix when scanning added lines in full unified diffs. |
| src/services/context.rs | Replace &line[1..] slicing with strip_prefix in several diff-analysis heuristics (imports/intents/metadata). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .iter() | ||
| .filter(|l| l.starts_with('+') && !l.starts_with("+++")) | ||
| .map(|l| &l[1..]) | ||
| .filter(|l| !l.starts_with("+++")) | ||
| .filter_map(|l| l.strip_prefix('+')) | ||
| .collect(); | ||
| let removed_lines: Vec<&str> = lines | ||
| .iter() | ||
| .filter(|l| l.starts_with('-') && !l.starts_with("---")) | ||
| .map(|l| &l[1..]) | ||
| .filter(|l| !l.starts_with("---")) | ||
| .filter_map(|l| l.strip_prefix('-')) |
There was a problem hiding this comment.
The !l.starts_with("+++") / !l.starts_with("---") header filtering is overly broad: it will also exclude legitimate added/removed hunk lines whose content begins with ++/-- (e.g., an added line starting with "++ foo" becomes "+++ foo" in the diff). Since file.diff includes headers and hunks, consider tracking whether you're inside a hunk (after an @@ header) and only skipping +++/--- lines when not in a hunk, or otherwise narrowing header detection so hunk lines aren’t dropped from the indentation-only comparison.
| if !line.starts_with("+++") | ||
| && let Some(content) = line.strip_prefix('+') | ||
| { |
There was a problem hiding this comment.
In scan_full_diff_with_patterns, current_line is only Some after parsing an @@ ... @@ hunk header, so diff file headers should already be out of the way. The extra !line.starts_with("+++") guard can cause false negatives by skipping real added lines whose content begins with "++" (diff line starts with "+++"). Consider removing this guard (or making header skipping stateful) and relying on strip_prefix('+') to identify added lines within hunks.
| if !line.starts_with("+++") | |
| && let Some(content) = line.strip_prefix('+') | |
| { | |
| if let Some(content) = line.strip_prefix('+') { |
| // Skip diff headers | ||
| if line.starts_with("+++") || line.starts_with("---") { | ||
| continue; | ||
| } | ||
| // Detect added/removed import lines | ||
| if (line.starts_with('+') && Self::is_import_line(&line[1..])) | ||
| || (line.starts_with('-') && Self::is_import_line(&line[1..])) | ||
| { | ||
| let action = if line.starts_with('+') { | ||
| "added" | ||
| } else { | ||
| "removed" | ||
| }; | ||
| let (action, content) = if let Some(rest) = line.strip_prefix('+') { | ||
| ("added", rest) | ||
| } else if let Some(rest) = line.strip_prefix('-') { |
There was a problem hiding this comment.
line.starts_with("+++") || line.starts_with("---") treats any hunk line whose content begins with "++"/"--" as a header and skips it (e.g. added line "++ use foo" becomes "+++ use foo"). Since file.diff contains both headers and hunks, consider making header skipping depend on being outside a hunk (track in_hunk via @@), so import changes inside hunks are not missed.
| if line.starts_with("+++") { | ||
| continue; | ||
| } | ||
| let Some(content) = line.strip_prefix('+') else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
The line.starts_with("+++") skip here can drop legitimate added hunk lines whose content begins with "++" (diff line starts with "+++"), which would undercount total_added and bias intent detection. Consider tracking whether the loop is inside a hunk (after @@) and only skipping +++ headers outside hunks (or otherwise using more stateful header detection).
| if line.starts_with("+++") || line.starts_with("---") { | ||
| continue; | ||
| } | ||
| let (is_added, content) = if let Some(rest) = line.strip_prefix('+') { | ||
| (true, rest) | ||
| } else if let Some(rest) = line.strip_prefix('-') { | ||
| (false, rest) |
There was a problem hiding this comment.
Same issue as other diff scans: line.starts_with("+++") || line.starts_with("---") will also skip real added/removed hunk lines whose content begins with "++"/"--" (making the diff line start with "+++"/"---"). Consider tracking in_hunk via @@ and only treating +++/--- as headers outside hunks so metadata breaking-change signals aren't missed.
Summary
refactor(services): replace byte indexing with strip_prefix.
Audit context
Closes audit entry F-011 from #3.
Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets