From bdc7065c6cdc90d97aff3443c1979c2fe9641d5a Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:22:13 +0100 Subject: [PATCH 1/5] fix(ci): the invisible-character gate never matched anything MEASURED 2026-08-27: this gate's pattern caught 0 OF 6 invisible-character test cases. It has never detected an NBSP, zero-width space, BOM, soft hyphen, bidi override or word joiner. ROOT CAUSE: the pattern used UTF-8 BYTE sequences (\xc2\xa0) while grep -P matches CHARACTERS. Bytes c2 a0 are ONE character U+00A0; \xc2\xa0 asks for TWO characters, U+00C2 then U+00A0, which is never present. grep -P '\xc2\xa0' -> miss grep -P '\x{a0}' -> MATCH Only \x00 worked, being single-byte in both readings. FIXED: codepoint escapes; C0 control characters \x01-\x08,\x0B,\x0C,\x0E-\x1F added (TAB/LF/CR excluded); and grep -a, without which grep skips any NUL-bearing file as binary. The C0 range matters: a stray BACKSPACE byte made a workflow unparseable in developer-ecosystem, so it never ran, and this linter called it clean. Canonical fix: hyperpolymath/empty-linter#70. 1 file(s) here. VERIFIED: YAML re-parsed, and the corrected pattern was confirmed to catch a real NBSP before the change was kept. --- .github/workflows/dogfood-gate.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/dogfood-gate.yml b/.github/workflows/dogfood-gate.yml index f69883d1..1b15f656 100644 --- a/.github/workflows/dogfood-gate.yml +++ b/.github/workflows/dogfood-gate.yml @@ -150,7 +150,7 @@ jobs: # Checks for: zero-width spaces, zero-width joiners, BOM, soft hyphens, # non-breaking spaces, null bytes, and other invisible Unicode in source files. set +e - PATTERNS='\xc2\xa0|\xe2\x80\x8b|\xe2\x80\x8c|\xe2\x80\x8d|\xef\xbb\xbf|\xc2\xad|\xe2\x80\x8e|\xe2\x80\x8f|\xe2\x80\xaa|\xe2\x80\xab|\xe2\x80\xac|\xe2\x80\xad|\xe2\x80\xae|\x00' + PATTERNS='\x00|[\x01-\x08\x0B\x0C\x0E-\x1F]|\x{a0}|\x{ad}|\x{200b}|\x{200c}|\x{200d}|\x{200e}|\x{200f}|\x{202a}|\x{202b}|\x{202c}|\x{202d}|\x{202e}|\x{2060}|\x{feff}' find "$GITHUB_WORKSPACE" \ -not -path '*/.git/*' -not -path '*/node_modules/*' \ -not -path '*/.deno/*' -not -path '*/target/*' \ @@ -161,7 +161,7 @@ jobs: -o -name '*.yml' -o -name '*.yaml' -o -name '*.md' -o -name '*.adoc' \ -o -name '*.idr' -o -name '*.zig' -o -name '*.v' -o -name '*.jl' \ -o -name '*.gleam' -o -name '*.hs' -o -name '*.ml' -o -name '*.sh' \) \ - -exec grep -Prl "$PATTERNS" {} \; > /tmp/empty-lint-results.txt 2>/dev/null + -exec grep -aPrl "$PATTERNS" {} \; > /tmp/empty-lint-results.txt 2>/dev/null EL_EXIT=$? set -e From 4e5a45d3db44815dafaef9a9b3e673e299fb5309 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:35:47 +0100 Subject: [PATCH 2/5] fix(ci): make invisible-character PCRE locale-independent --- .github/workflows/dogfood-gate.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/dogfood-gate.yml b/.github/workflows/dogfood-gate.yml index 1b15f656..60d8e224 100644 --- a/.github/workflows/dogfood-gate.yml +++ b/.github/workflows/dogfood-gate.yml @@ -150,7 +150,7 @@ jobs: # Checks for: zero-width spaces, zero-width joiners, BOM, soft hyphens, # non-breaking spaces, null bytes, and other invisible Unicode in source files. set +e - PATTERNS='\x00|[\x01-\x08\x0B\x0C\x0E-\x1F]|\x{a0}|\x{ad}|\x{200b}|\x{200c}|\x{200d}|\x{200e}|\x{200f}|\x{202a}|\x{202b}|\x{202c}|\x{202d}|\x{202e}|\x{2060}|\x{feff}' + PATTERNS='(*UTF)[\x00-\x08\x0B\x0C\x0E-\x1F\x{a0}\x{ad}\x{200b}-\x{200f}\x{202a}-\x{202f}\x{2060}\x{2066}-\x{2069}\x{feff}]' find "$GITHUB_WORKSPACE" \ -not -path '*/.git/*' -not -path '*/node_modules/*' \ -not -path '*/.deno/*' -not -path '*/target/*' \ From 86fbcab11d1fd1040ce1743b05bf4a6b30111f64 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:40:39 +0100 Subject: [PATCH 3/5] Update .github/workflows/dogfood-gate.yml Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com> Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> --- .github/workflows/dogfood-gate.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/dogfood-gate.yml b/.github/workflows/dogfood-gate.yml index 60d8e224..345bc8e0 100644 --- a/.github/workflows/dogfood-gate.yml +++ b/.github/workflows/dogfood-gate.yml @@ -161,7 +161,7 @@ jobs: -o -name '*.yml' -o -name '*.yaml' -o -name '*.md' -o -name '*.adoc' \ -o -name '*.idr' -o -name '*.zig' -o -name '*.v' -o -name '*.jl' \ -o -name '*.gleam' -o -name '*.hs' -o -name '*.ml' -o -name '*.sh' \) \ - -exec grep -aPrl "$PATTERNS" {} \; > /tmp/empty-lint-results.txt 2>/dev/null + -exec grep -aPl "$PATTERNS" {} + > /tmp/empty-lint-results.txt 2>/dev/null EL_EXIT=$? set -e From 6af0f1b65dd03d5e34cf32461df4055c63331024 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 00:28:47 +0100 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=94=A7=20CodeRabbit=20CI=20Fix:=20Fix?= =?UTF-8?q?=20failing=20CI=20checks=20across=20security,=20Rust,=20and=20g?= =?UTF-8?q?overnance=20workflows=20(#513)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failure fixes was requested by @hyperpolymath. * https://github.com/hyperpolymath/gitbot-fleet/pull/497#issuecomment-5439764257 The following files were modified: * `bots/seambot/tests/github_integration.rs` * `dashboard/src/main.rs` * `robot-repo-automaton/src/fixer.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- bots/seambot/tests/github_integration.rs | 2 +- dashboard/src/main.rs | 11 +- robot-repo-automaton/src/fixer.rs | 406 ++++++++++++++++++++++- 3 files changed, 401 insertions(+), 18 deletions(-) diff --git a/bots/seambot/tests/github_integration.rs b/bots/seambot/tests/github_integration.rs index 3f5173b2..8cdb39c6 100644 --- a/bots/seambot/tests/github_integration.rs +++ b/bots/seambot/tests/github_integration.rs @@ -153,7 +153,7 @@ mod tests { let response = r#"{ "token": "ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", "expires_at": "2024-01-15T12:00:00Z" - }"#; + }"#; // gitleaks:allow -- placeholder token (all 'x's), not a real credential let parsed: serde_json::Value = serde_json::from_str(response).unwrap(); assert!(parsed["token"].as_str().unwrap().starts_with("ghs_")); diff --git a/dashboard/src/main.rs b/dashboard/src/main.rs index b8f8f501..dede6a8c 100644 --- a/dashboard/src/main.rs +++ b/dashboard/src/main.rs @@ -175,7 +175,11 @@ async fn report_handler( match format.to_lowercase().as_str() { "html" => (StatusCode::OK, [("content-type", "text/html")], report), - "json" => (StatusCode::OK, [("content-type", "application/json")], report), + "json" => ( + StatusCode::OK, + [("content-type", "application/json")], + report, + ), _ => (StatusCode::OK, [("content-type", "text/plain")], report), } } @@ -211,10 +215,7 @@ async fn websocket_handler( } /// Handle WebSocket connection -async fn websocket_connection( - mut socket: axum::extract::ws::WebSocket, - state: AppState, -) { +async fn websocket_connection(mut socket: axum::extract::ws::WebSocket, state: AppState) { use axum::extract::ws::Message; use tokio::time::{interval, Duration}; diff --git a/robot-repo-automaton/src/fixer.rs b/robot-repo-automaton/src/fixer.rs index c288a44d..91a788ad 100644 --- a/robot-repo-automaton/src/fixer.rs +++ b/robot-repo-automaton/src/fixer.rs @@ -1,12 +1,394 @@ - content - .replace("gitbot-fleet", repo_name) - .replace("{{LICENSE}}", "MPL-2.0") - .replace("{{YEAR}}", &year) - .replace("{{AUTHOR}}", "Jonathan D.A. Jewell") - .replace("{{EMAIL}}", "j.d.a.jewell@open.ac.uk"); - content - .replace("gitbot-fleet", repo_name) - .replace("{{LICENSE}}", "MPL-2.0") - .replace("{{YEAR}}", &year) - .replace("{{AUTHOR}}", "Jonathan D.A. Jewell") - .replace("{{EMAIL}}", "j.d.a.jewell@open.ac.uk") +// SPDX-License-Identifier: MPL-2.0 +//! Fix application - delete, modify, create, disable. + +use regex::Regex; +use std::fs; +use std::path::{Component, Path, PathBuf}; + +use crate::catalog::{Fix, FixAction}; +use crate::detector::DetectedIssue; +use crate::error::{Error, Result}; + +/// Result of applying a single fix. +#[derive(Debug, Clone)] +pub struct FixResult { + pub success: bool, + pub files_modified: Vec, + pub action_taken: String, + pub error: Option, +} + +/// Applies fixes to a repository on disk. +pub struct Fixer { + repo_path: PathBuf, + dry_run: bool, +} + +impl Fixer { + /// Create a new fixer for the given repository. + pub fn new(repo_path: PathBuf, dry_run: bool) -> Self { + Self { repo_path, dry_run } + } + + /// Resolve a fix target relative to the repository root, rejecting any + /// path that would escape the repository. The target need not exist + /// (e.g. for `Create`), so this is pure path arithmetic. + fn resolve_target(&self, target: &str) -> Result { + let joined = self.repo_path.join(target); + + let mut normalized = PathBuf::new(); + for component in joined.components() { + match component { + Component::ParentDir => { + if !normalized.pop() { + return Err(Error::Fix(format!( + "Fix target '{target}' resolves outside the repository" + ))); + } + } + Component::CurDir => {} + other => normalized.push(other), + } + } + + if !normalized.starts_with(&self.repo_path) { + return Err(Error::Fix(format!( + "Fix target '{target}' resolves outside the repository" + ))); + } + + Ok(normalized) + } + + /// Apply a single fix, returning the outcome. A rejected or failed fix + /// is reported via `FixResult`, not `Err` (the operation itself did not + /// error; the requested change simply could not be made safely). + pub fn apply(&self, _issue: &DetectedIssue, fix: &Fix) -> Result { + let target = match self.resolve_target(&fix.target) { + Ok(path) => path, + Err(e) => { + return Ok(FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "rejected".to_string(), + error: Some(e.to_string()), + }); + } + }; + + let result = match fix.action { + FixAction::Delete => self.apply_delete(&target), + FixAction::Modify => self.apply_modify(&target, fix), + FixAction::Create => self.apply_create(&target, fix), + FixAction::Disable => FixResult { + success: true, + files_modified: Vec::new(), + action_taken: "Disable: no-op, manual review required".to_string(), + error: None, + }, + }; + + Ok(result) + } + + fn apply_delete(&self, target: &Path) -> FixResult { + if !target.exists() { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("Delete: {} already absent", target.display()), + error: None, + }; + } + + if self.dry_run { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("DRY RUN: would delete {}", target.display()), + error: None, + }; + } + + match fs::remove_file(target) { + Ok(()) => FixResult { + success: true, + files_modified: vec![target.to_path_buf()], + action_taken: format!("Deleted {}", target.display()), + error: None, + }, + Err(e) => FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Delete: failed".to_string(), + error: Some(e.to_string()), + }, + } + } + + fn apply_create(&self, target: &Path, fix: &Fix) -> FixResult { + if target.exists() { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("Create: {} already exists", target.display()), + error: None, + }; + } + + if self.dry_run { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("DRY RUN: would create {}", target.display()), + error: None, + }; + } + + if let Some(parent) = target.parent() { + if let Err(e) = fs::create_dir_all(parent) { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Create: failed".to_string(), + error: Some(e.to_string()), + }; + } + } + + let content = fix.fallback.clone().unwrap_or_default(); + match fs::write(target, content) { + Ok(()) => FixResult { + success: true, + files_modified: vec![target.to_path_buf()], + action_taken: format!("Created {}", target.display()), + error: None, + }, + Err(e) => FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Create: failed".to_string(), + error: Some(e.to_string()), + }, + } + } + + fn apply_modify(&self, target: &Path, fix: &Fix) -> FixResult { + let Some(modification) = fix.modification.as_deref() else { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Modify: failed".to_string(), + error: Some("Modify fix has no modification instruction".to_string()), + }; + }; + + let original = match fs::read(target) { + Ok(bytes) => bytes, + Err(e) => { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Modify: failed".to_string(), + error: Some(e.to_string()), + }; + } + }; + + let original_text = match String::from_utf8(original) { + Ok(text) => text, + Err(_) => { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Modify: failed".to_string(), + error: Some(format!( + "Refusing to modify binary file {}", + target.display() + )), + }; + } + }; + + let new_text = match Self::apply_modification(&original_text, modification) { + Ok(text) => text, + Err(e) => { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Modify: failed".to_string(), + error: Some(e), + }; + } + }; + + if self.dry_run { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("DRY RUN: would modify {}", target.display()), + error: None, + }; + } + + if new_text == original_text { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("Modify: {} already up to date", target.display()), + error: None, + }; + } + + match fs::write(target, new_text) { + Ok(()) => FixResult { + success: true, + files_modified: vec![target.to_path_buf()], + action_taken: format!("Modified {}", target.display()), + error: None, + }, + Err(e) => FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Modify: failed".to_string(), + error: Some(e.to_string()), + }, + } + } + + /// Apply a `replace-line:`, `replace-pattern:`, `insert-before:` or + /// `insert-after:` modification instruction to `content`. + fn apply_modification( + content: &str, + modification: &str, + ) -> std::result::Result { + if let Some(rest) = modification.strip_prefix("replace-pattern:") { + let (pattern, replacement) = rest + .split_once(':') + .ok_or_else(|| "Invalid replace-pattern instruction".to_string())?; + let re = Regex::new(pattern).map_err(|e| format!("Invalid regex: {e}"))?; + return Ok(re.replace_all(content, replacement).into_owned()); + } + + let mut lines: Vec = content.lines().map(str::to_string).collect(); + let trailing_newline = content.ends_with('\n'); + + if let Some(rest) = modification.strip_prefix("replace-line:") { + let (line_no, replacement) = rest + .split_once(':') + .ok_or_else(|| "Invalid replace-line instruction".to_string())?; + let line_no: usize = line_no + .parse() + .map_err(|_| "Invalid line number in replace-line instruction".to_string())?; + if line_no == 0 || line_no > lines.len() { + return Err(format!( + "replace-line: line {line_no} does not exist (file has {} lines)", + lines.len() + )); + } + lines[line_no - 1] = replacement.to_string(); + } else if let Some(rest) = modification.strip_prefix("insert-before:") { + let (line_no, text) = rest + .split_once(':') + .ok_or_else(|| "Invalid insert-before instruction".to_string())?; + let line_no: usize = line_no + .parse() + .map_err(|_| "Invalid line number in insert-before instruction".to_string())?; + if line_no == 0 || line_no > lines.len() + 1 { + return Err(format!( + "insert-before: line {line_no} does not exist (file has {} lines)", + lines.len() + )); + } + lines.insert(line_no - 1, text.to_string()); + } else if let Some(rest) = modification.strip_prefix("insert-after:") { + let (line_no, text) = rest + .split_once(':') + .ok_or_else(|| "Invalid insert-after instruction".to_string())?; + let line_no: usize = line_no + .parse() + .map_err(|_| "Invalid line number in insert-after instruction".to_string())?; + if line_no == 0 || line_no > lines.len() { + return Err(format!( + "insert-after: line {line_no} does not exist (file has {} lines)", + lines.len() + )); + } + lines.insert(line_no, text.to_string()); + } else { + return Err(format!("Unknown modification instruction: {modification}")); + } + + let mut result = lines.join("\n"); + if trailing_newline { + result.push('\n'); + } + Ok(result) + } + + /// Apply a batch of auto-approved fixes and commit the results locally. + pub fn apply_and_commit( + &self, + _issues: &[DetectedIssue], + auto_fixes: &[(DetectedIssue, Fix)], + ) -> Result> { + let mut results = Vec::with_capacity(auto_fixes.len()); + let mut modified: Vec = Vec::new(); + let mut messages: Vec = Vec::new(); + + for (issue, fix) in auto_fixes { + let result = self.apply(issue, fix)?; + if result.success && !result.files_modified.is_empty() { + modified.extend(result.files_modified.clone()); + messages.push(issue.commit_message.clone()); + } + results.push(result); + } + + if !self.dry_run && !modified.is_empty() { + self.commit_changes(&modified, &messages)?; + } + + Ok(results) + } + + /// Stage and commit the given files in the local repository. + fn commit_changes(&self, files: &[PathBuf], messages: &[String]) -> Result<()> { + let repo = git2::Repository::open(&self.repo_path)?; + let mut index = repo.index()?; + + for file in files { + let relative = file.strip_prefix(&self.repo_path).unwrap_or(file); + if file.exists() { + index.add_path(relative)?; + } else { + let _ = index.remove_path(relative); + } + } + index.write()?; + + let tree_id = index.write_tree()?; + let tree = repo.find_tree(tree_id)?; + let signature = git2::Signature::now("robot-repo-automaton", "noreply@hyperpolymath.dev")?; + + let message = if messages.is_empty() { + "fix: automated compliance fixes".to_string() + } else { + messages.join("\n") + }; + + let parent_commit = repo.head().ok().and_then(|h| h.peel_to_commit().ok()); + let parents: Vec<&git2::Commit> = parent_commit.iter().collect(); + + repo.commit( + Some("HEAD"), + &signature, + &signature, + &message, + &tree, + &parents, + )?; + + Ok(()) + } +} From 09f08a61b048b5520240f5bbd290d3235ee5d174 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 02:18:04 +0000 Subject: [PATCH 5/5] fix(fixer): harden automated fix application --- robot-repo-automaton/README.adoc | 4 + robot-repo-automaton/src/fixer.rs | 290 +++++++++++++++++-- robot-repo-automaton/tests/fixer_tests.rs | 324 ++++++++++++++++++++++ 3 files changed, 593 insertions(+), 25 deletions(-) diff --git a/robot-repo-automaton/README.adoc b/robot-repo-automaton/README.adoc index 878cb95f..c55d6442 100644 --- a/robot-repo-automaton/README.adoc +++ b/robot-repo-automaton/README.adoc @@ -316,6 +316,10 @@ The error catalog uses S-expression format (Guile Scheme compatible): | Apply a line-level transformation. Requires a `modification` field with spec such as `replace-line:N:content`, `insert-before:N:content`, `replace-pattern:regex:replacement`, `prepend:content`, or `append:content`. + For `replace-pattern`, the first unescaped colon separates the regex and + replacement. Write a colon within either field as `\:` (for example, + `replace-pattern:https?\://old:https\://new`). Regex replacement expansion is + preserved, so capture references such as `$1` and `$name` are supported. | `create` | Create the target file from a template or `fallback` content. Supports diff --git a/robot-repo-automaton/src/fixer.rs b/robot-repo-automaton/src/fixer.rs index 91a788ad..b329436c 100644 --- a/robot-repo-automaton/src/fixer.rs +++ b/robot-repo-automaton/src/fixer.rs @@ -3,6 +3,7 @@ use regex::Regex; use std::fs; +use std::io::ErrorKind; use std::path::{Component, Path, PathBuf}; use crate::catalog::{Fix, FixAction}; @@ -30,11 +31,20 @@ impl Fixer { Self { repo_path, dry_run } } - /// Resolve a fix target relative to the repository root, rejecting any - /// path that would escape the repository. The target need not exist - /// (e.g. for `Create`), so this is pure path arithmetic. + /// Resolve a fix target relative to the canonical repository root. + /// + /// The target need not exist (for example, for `Create`), so the nearest + /// existing ancestor is canonicalized and any missing suffix is appended. + /// This rejects lexical traversal, symlink escapes, and symlink targets + /// (including dangling symlinks) before a mutation can occur. fn resolve_target(&self, target: &str) -> Result { - let joined = self.repo_path.join(target); + let canonical_root = self.repo_path.canonicalize().map_err(|e| { + Error::Fix(format!( + "Failed to canonicalize repository '{}': {e}", + self.repo_path.display() + )) + })?; + let joined = canonical_root.join(target); let mut normalized = PathBuf::new(); for component in joined.components() { @@ -51,13 +61,73 @@ impl Fixer { } } - if !normalized.starts_with(&self.repo_path) { + if !normalized.starts_with(&canonical_root) { return Err(Error::Fix(format!( "Fix target '{target}' resolves outside the repository" ))); } - Ok(normalized) + match fs::symlink_metadata(&normalized) { + Ok(metadata) if metadata.file_type().is_symlink() => { + return Err(Error::Fix(format!( + "Fix target '{target}' is a symlink and cannot be mutated" + ))); + } + Ok(_) => {} + Err(e) if e.kind() == ErrorKind::NotFound => {} + Err(e) => { + return Err(Error::Fix(format!( + "Failed to inspect fix target '{target}': {e}" + ))); + } + } + + let mut ancestor = normalized.clone(); + let mut missing = Vec::new(); + let canonical_ancestor = loop { + match fs::symlink_metadata(&ancestor) { + Ok(_) => { + break ancestor.canonicalize().map_err(|e| { + Error::Fix(format!( + "Failed to canonicalize fix target ancestor '{}': {e}", + ancestor.display() + )) + })?; + } + Err(e) if e.kind() == ErrorKind::NotFound => { + let component = ancestor.file_name().ok_or_else(|| { + Error::Fix(format!( + "Fix target '{target}' resolves outside the repository" + )) + })?; + missing.push(component.to_os_string()); + if !ancestor.pop() { + return Err(Error::Fix(format!( + "Fix target '{target}' resolves outside the repository" + ))); + } + } + Err(e) => { + return Err(Error::Fix(format!( + "Failed to inspect fix target ancestor '{}': {e}", + ancestor.display() + ))); + } + } + }; + + if !canonical_ancestor.starts_with(&canonical_root) { + return Err(Error::Fix(format!( + "Fix target '{target}' resolves outside the repository" + ))); + } + + let mut resolved = canonical_ancestor; + for component in missing.iter().rev() { + resolved.push(component); + } + + Ok(resolved) } /// Apply a single fix, returning the outcome. A rejected or failed fix @@ -80,17 +150,34 @@ impl Fixer { FixAction::Delete => self.apply_delete(&target), FixAction::Modify => self.apply_modify(&target, fix), FixAction::Create => self.apply_create(&target, fix), - FixAction::Disable => FixResult { - success: true, - files_modified: Vec::new(), - action_taken: "Disable: no-op, manual review required".to_string(), - error: None, - }, + FixAction::Disable => { + let disabled = target.with_extension("yml.disabled"); + match self.resolve_target_path(&disabled) { + Ok(disabled) => self.apply_disable(&target, &disabled), + Err(e) => FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Disable: rejected".to_string(), + error: Some(e.to_string()), + }, + } + } }; Ok(result) } + fn resolve_target_path(&self, target: &Path) -> Result { + let canonical_root = self.repo_path.canonicalize()?; + let relative = target.strip_prefix(&canonical_root).map_err(|_| { + Error::Fix(format!( + "Fix target '{}' resolves outside the repository", + target.display() + )) + })?; + self.resolve_target(&relative.to_string_lossy()) + } + fn apply_delete(&self, target: &Path) -> FixResult { if !target.exists() { return FixResult { @@ -127,7 +214,7 @@ impl Fixer { } fn apply_create(&self, target: &Path, fix: &Fix) -> FixResult { - if target.exists() { + if fs::symlink_metadata(target).is_ok() { return FixResult { success: true, files_modified: Vec::new(), @@ -222,20 +309,20 @@ impl Fixer { } }; - if self.dry_run { + if new_text == original_text { return FixResult { success: true, files_modified: Vec::new(), - action_taken: format!("DRY RUN: would modify {}", target.display()), + action_taken: format!("Modify: {} already up to date", target.display()), error: None, }; } - if new_text == original_text { + if self.dry_run { return FixResult { success: true, files_modified: Vec::new(), - action_taken: format!("Modify: {} already up to date", target.display()), + action_taken: format!("DRY RUN: would modify {}", target.display()), error: None, }; } @@ -256,6 +343,70 @@ impl Fixer { } } + fn apply_disable(&self, target: &Path, disabled: &Path) -> FixResult { + if !target.exists() { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!("Disable: {} already absent", target.display()), + error: None, + }; + } + + if fs::symlink_metadata(disabled).is_ok() { + return FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Disable: failed".to_string(), + error: Some(format!( + "Refusing to overwrite existing disabled target {}", + disabled.display() + )), + }; + } + + if self.dry_run { + return FixResult { + success: true, + files_modified: Vec::new(), + action_taken: format!( + "DRY RUN: would rename {} to {}", + target.display(), + disabled.display() + ), + error: None, + }; + } + + // `hard_link` publishes the destination without overwriting a file + // that appears concurrently. Removing the source completes the rename. + match fs::hard_link(target, disabled) { + Ok(()) => match fs::remove_file(target) { + Ok(()) => FixResult { + success: true, + files_modified: vec![target.to_path_buf(), disabled.to_path_buf()], + action_taken: format!("Renamed {} to {}", target.display(), disabled.display()), + error: None, + }, + Err(e) => { + let _ = fs::remove_file(disabled); + FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Disable: failed".to_string(), + error: Some(e.to_string()), + } + } + }, + Err(e) => FixResult { + success: false, + files_modified: Vec::new(), + action_taken: "Disable: failed".to_string(), + error: Some(e.to_string()), + }, + } + } + /// Apply a `replace-line:`, `replace-pattern:`, `insert-before:` or /// `insert-after:` modification instruction to `content`. fn apply_modification( @@ -263,14 +414,26 @@ impl Fixer { modification: &str, ) -> std::result::Result { if let Some(rest) = modification.strip_prefix("replace-pattern:") { - let (pattern, replacement) = rest - .split_once(':') + let separator = rest + .char_indices() + .find_map(|(index, character)| { + (character == ':' && !Self::is_escaped(rest, index)).then_some(index) + }) .ok_or_else(|| "Invalid replace-pattern instruction".to_string())?; - let re = Regex::new(pattern).map_err(|e| format!("Invalid regex: {e}"))?; - return Ok(re.replace_all(content, replacement).into_owned()); + let pattern = Self::unescape_colons(&rest[..separator]); + let replacement = Self::unescape_colons(&rest[separator + 1..]); + if pattern.is_empty() { + return Err("Invalid replace-pattern instruction: empty regex".to_string()); + } + let re = Regex::new(&pattern).map_err(|e| format!("Invalid regex: {e}"))?; + return Ok(re.replace_all(content, replacement.as_str()).into_owned()); } - let mut lines: Vec = content.lines().map(str::to_string).collect(); + let line_ending = Self::dominant_line_ending(content); + let mut lines: Vec = content + .split_terminator('\n') + .map(|line| line.strip_suffix('\r').unwrap_or(line).to_string()) + .collect(); let trailing_newline = content.ends_with('\n'); if let Some(rest) = modification.strip_prefix("replace-line:") { @@ -319,19 +482,67 @@ impl Fixer { return Err(format!("Unknown modification instruction: {modification}")); } - let mut result = lines.join("\n"); + let mut result = lines.join(line_ending); if trailing_newline { - result.push('\n'); + result.push_str(line_ending); } Ok(result) } + fn is_escaped(value: &str, index: usize) -> bool { + value[..index] + .bytes() + .rev() + .take_while(|byte| *byte == b'\\') + .count() + % 2 + == 1 + } + + fn unescape_colons(value: &str) -> String { + let mut output = String::with_capacity(value.len()); + let mut characters = value.chars().peekable(); + while let Some(character) = characters.next() { + if character == '\\' && characters.peek() == Some(&':') { + characters.next(); + output.push(':'); + } else { + output.push(character); + } + } + output + } + + fn dominant_line_ending(content: &str) -> &'static str { + let bytes = content.as_bytes(); + let mut crlf = 0; + let mut lf = 0; + for (index, byte) in bytes.iter().enumerate() { + if *byte == b'\n' { + if index > 0 && bytes[index - 1] == b'\r' { + crlf += 1; + } else { + lf += 1; + } + } + } + if crlf > 0 && crlf >= lf { + "\r\n" + } else { + "\n" + } + } + /// Apply a batch of auto-approved fixes and commit the results locally. pub fn apply_and_commit( &self, _issues: &[DetectedIssue], auto_fixes: &[(DetectedIssue, Fix)], ) -> Result> { + if !self.dry_run && !auto_fixes.is_empty() { + self.ensure_clean_index()?; + } + let mut results = Vec::with_capacity(auto_fixes.len()); let mut modified: Vec = Vec::new(); let mut messages: Vec = Vec::new(); @@ -354,11 +565,15 @@ impl Fixer { /// Stage and commit the given files in the local repository. fn commit_changes(&self, files: &[PathBuf], messages: &[String]) -> Result<()> { + // Re-check immediately before touching the index in case another + // process staged work while fixes were being applied. + self.ensure_clean_index()?; let repo = git2::Repository::open(&self.repo_path)?; let mut index = repo.index()?; + let canonical_root = self.repo_path.canonicalize()?; for file in files { - let relative = file.strip_prefix(&self.repo_path).unwrap_or(file); + let relative = file.strip_prefix(&canonical_root).unwrap_or(file); if file.exists() { index.add_path(relative)?; } else { @@ -391,4 +606,29 @@ impl Fixer { Ok(()) } + + fn ensure_clean_index(&self) -> Result<()> { + let repo = git2::Repository::open(&self.repo_path)?; + let index = repo.index()?; + let head_tree = match repo.head() { + Ok(head) => Some(head.peel_to_tree()?), + Err(e) + if matches!( + e.code(), + git2::ErrorCode::UnbornBranch | git2::ErrorCode::NotFound + ) => + { + None + } + Err(e) => return Err(e.into()), + }; + let diff = repo.diff_tree_to_index(head_tree.as_ref(), Some(&index), None)?; + if diff.deltas().len() != 0 { + return Err(Error::Fix( + "Refusing to apply and commit fixes while the repository index contains staged changes" + .to_string(), + )); + } + Ok(()) + } } diff --git a/robot-repo-automaton/tests/fixer_tests.rs b/robot-repo-automaton/tests/fixer_tests.rs index 581801a7..f3663ed2 100644 --- a/robot-repo-automaton/tests/fixer_tests.rs +++ b/robot-repo-automaton/tests/fixer_tests.rs @@ -20,6 +20,16 @@ fn make_issue(id: &str) -> DetectedIssue { } } +fn make_fix(action: FixAction, target: &str) -> Fix { + Fix { + action, + target: target.to_string(), + reason: None, + modification: None, + fallback: None, + } +} + // ========================================================================= // DELETE FIX TESTS // ========================================================================= @@ -361,6 +371,134 @@ fn test_dry_run_does_not_delete() { assert!(file_path.exists()); // File still exists } +#[test] +fn test_dry_run_unchanged_modify_reports_no_op() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("config.txt"); + std::fs::write(&file_path, "already=current\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), true); + let issue = make_issue("DRY-004"); + let mut fix = make_fix(FixAction::Modify, "config.txt"); + fix.modification = Some("replace-pattern:missing:replacement".to_string()); + + let result = fixer.apply(&issue, &fix).unwrap(); + assert!(result.success); + assert!(result.files_modified.is_empty()); + assert!(result.action_taken.contains("already up to date")); + assert!(!result.action_taken.contains("would modify")); +} + +// ========================================================================= +// DISABLE FIX TESTS +// ========================================================================= + +#[test] +fn test_disable_fix_renames_workflow() { + let temp = TempDir::new().unwrap(); + let source = temp.path().join("workflow.yml"); + let disabled = temp.path().join("workflow.yml.disabled"); + std::fs::write(&source, "name: active\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let result = fixer + .apply( + &make_issue("DIS-001"), + &make_fix(FixAction::Disable, "workflow.yml"), + ) + .unwrap(); + + assert!(result.success); + assert!(!source.exists()); + assert_eq!(std::fs::read_to_string(&disabled).unwrap(), "name: active\n"); + assert_eq!(result.files_modified, vec![source, disabled]); +} + +#[test] +fn test_disable_fix_does_not_overwrite_existing_destination() { + let temp = TempDir::new().unwrap(); + let source = temp.path().join("workflow.yml"); + let disabled = temp.path().join("workflow.yml.disabled"); + std::fs::write(&source, "active\n").unwrap(); + std::fs::write(&disabled, "preserve me\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let result = fixer + .apply( + &make_issue("DIS-002"), + &make_fix(FixAction::Disable, "workflow.yml"), + ) + .unwrap(); + + assert!(!result.success); + assert_eq!(std::fs::read_to_string(source).unwrap(), "active\n"); + assert_eq!(std::fs::read_to_string(disabled).unwrap(), "preserve me\n"); +} + +// ========================================================================= +// MODIFICATION FORMAT AND LINE ENDING TESTS +// ========================================================================= + +#[test] +fn test_replace_pattern_supports_colons_and_capture_expansion() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("urls.txt"); + std::fs::write(&file_path, "http://old:8080/path\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Modify, "urls.txt"); + fix.modification = + Some(r"replace-pattern:(http\://old\:)(\d+):prefix-$1\:$2".to_string()); + let result = fixer.apply(&make_issue("MOD-006"), &fix).unwrap(); + + assert!(result.success); + assert_eq!( + std::fs::read_to_string(file_path).unwrap(), + "prefix-http://old::8080/path\n" + ); +} + +#[test] +fn test_replace_pattern_preserves_named_capture_expansion() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("value.txt"); + std::fs::write(&file_path, "item=42\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Modify, "value.txt"); + fix.modification = + Some(r"replace-pattern:item=(?P\d+):value=$number".to_string()); + let result = fixer.apply(&make_issue("MOD-007"), &fix).unwrap(); + + assert!(result.success); + assert_eq!(std::fs::read_to_string(file_path).unwrap(), "value=42\n"); +} + +#[test] +fn test_line_modifications_preserve_crlf() { + let cases = [ + ("replace-line:1:ONE", "ONE\r\ntwo\r\n"), + ("insert-before:2:middle", "one\r\nmiddle\r\ntwo\r\n"), + ("insert-after:1:middle", "one\r\nmiddle\r\ntwo\r\n"), + ]; + + for (index, (modification, expected)) in cases.into_iter().enumerate() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("crlf.txt"); + std::fs::write(&file_path, b"one\r\ntwo\r\n").unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Modify, "crlf.txt"); + fix.modification = Some(modification.to_string()); + let result = fixer + .apply(&make_issue(&format!("CRLF-{index}")), &fix) + .unwrap(); + + assert!(result.success); + assert_eq!(std::fs::read(&file_path).unwrap(), expected.as_bytes()); + } +} + // ========================================================================= // IDEMPOTENCY TESTS // ========================================================================= @@ -577,3 +715,189 @@ fn test_path_within_repo_is_not_rejected() { assert!(result.success, "Legitimate in-repo path was incorrectly rejected"); assert!(!file_path.exists()); } + +#[cfg(unix)] +#[test] +fn test_symlink_targets_are_rejected_for_every_action() { + use std::os::unix::fs::symlink; + + for (index, action) in [ + FixAction::Delete, + FixAction::Modify, + FixAction::Create, + FixAction::Disable, + ] + .into_iter() + .enumerate() + { + let temp = TempDir::new().unwrap(); + let outer = TempDir::new().unwrap(); + let victim = outer.path().join("victim.txt"); + std::fs::write(&victim, "untouched\n").unwrap(); + symlink(&victim, temp.path().join("target.yml")).unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(action, "target.yml"); + fix.modification = Some("replace-line:1:changed".to_string()); + fix.fallback = Some("created".to_string()); + let result = fixer + .apply(&make_issue(&format!("SYM-{index}")), &fix) + .unwrap(); + + assert!(!result.success); + assert!(result.error.as_deref().unwrap().contains("symlink")); + assert_eq!(std::fs::read_to_string(victim).unwrap(), "untouched\n"); + } +} + +#[cfg(unix)] +#[test] +fn test_dangling_symlink_targets_are_rejected_for_every_action() { + use std::os::unix::fs::symlink; + + for (index, action) in [ + FixAction::Delete, + FixAction::Modify, + FixAction::Create, + FixAction::Disable, + ] + .into_iter() + .enumerate() + { + let temp = TempDir::new().unwrap(); + symlink("missing", temp.path().join("target.yml")).unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(action, "target.yml"); + fix.modification = Some("replace-line:1:changed".to_string()); + fix.fallback = Some("created".to_string()); + let result = fixer + .apply(&make_issue(&format!("DANGLING-{index}")), &fix) + .unwrap(); + + assert!(!result.success); + assert!(result.error.as_deref().unwrap().contains("symlink")); + assert!(std::fs::symlink_metadata(temp.path().join("target.yml")).is_ok()); + } +} + +#[cfg(unix)] +#[test] +fn test_symlink_parent_escape_is_rejected() { + use std::os::unix::fs::symlink; + + let temp = TempDir::new().unwrap(); + let outer = TempDir::new().unwrap(); + symlink(outer.path(), temp.path().join("outside-link")).unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Create, "outside-link/injected.txt"); + fix.fallback = Some("injected".to_string()); + let result = fixer.apply(&make_issue("SYM-PARENT"), &fix).unwrap(); + + assert!(!result.success); + assert!( + result + .error + .as_deref() + .unwrap() + .contains("outside the repository") + ); + assert!(!outer.path().join("injected.txt").exists()); +} + +#[test] +fn test_relative_repository_path_accepts_in_repo_target() { + let current = std::env::current_dir().unwrap(); + let temp = tempfile::Builder::new() + .prefix("fixer-relative-") + .tempdir_in(¤t) + .unwrap(); + let relative_repo = temp.path().strip_prefix(¤t).unwrap().to_path_buf(); + let file_path = temp.path().join("safe.txt"); + std::fs::write(&file_path, "before\n").unwrap(); + + let fixer = Fixer::new(relative_repo, false); + let mut fix = make_fix(FixAction::Modify, "safe.txt"); + fix.modification = Some("replace-line:1:after".to_string()); + let result = fixer.apply(&make_issue("RELATIVE"), &fix).unwrap(); + + assert!(result.success); + assert_eq!(std::fs::read_to_string(file_path).unwrap(), "after\n"); +} + +#[test] +fn test_apply_and_commit_rejects_staged_changes_before_fixing() { + let temp = TempDir::new().unwrap(); + let repo = git2::Repository::init(temp.path()).unwrap(); + let staged_path = temp.path().join("staged.txt"); + std::fs::write(&staged_path, "base\n").unwrap(); + + { + let mut index = repo.index().unwrap(); + index.add_path(std::path::Path::new("staged.txt")).unwrap(); + index.write().unwrap(); + let tree_id = index.write_tree().unwrap(); + let tree = repo.find_tree(tree_id).unwrap(); + let signature = git2::Signature::now("test", "test@example.com").unwrap(); + repo.commit( + Some("HEAD"), + &signature, + &signature, + "initial", + &tree, + &[], + ) + .unwrap(); + } + + std::fs::write(&staged_path, "staged change\n").unwrap(); + let mut index = repo.index().unwrap(); + index.add_path(std::path::Path::new("staged.txt")).unwrap(); + index.write().unwrap(); + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Create, "new.txt"); + fix.fallback = Some("must not be created".to_string()); + let fixes = vec![(make_issue("INDEX-001"), fix)]; + let error = fixer.apply_and_commit(&[], &fixes).unwrap_err(); + + assert!(error.to_string().contains("staged changes")); + assert!(!temp.path().join("new.txt").exists()); + assert_eq!(std::fs::read_to_string(staged_path).unwrap(), "staged change\n"); +} + +#[test] +fn test_apply_and_commit_commits_when_index_is_clean() { + let temp = TempDir::new().unwrap(); + let repo = git2::Repository::init(temp.path()).unwrap(); + std::fs::write(temp.path().join("tracked.txt"), "base\n").unwrap(); + + { + let mut index = repo.index().unwrap(); + index.add_path(std::path::Path::new("tracked.txt")).unwrap(); + index.write().unwrap(); + let tree_id = index.write_tree().unwrap(); + let tree = repo.find_tree(tree_id).unwrap(); + let signature = git2::Signature::now("test", "test@example.com").unwrap(); + repo.commit( + Some("HEAD"), + &signature, + &signature, + "initial", + &tree, + &[], + ) + .unwrap(); + } + + let fixer = Fixer::new(temp.path().to_path_buf(), false); + let mut fix = make_fix(FixAction::Create, "new.txt"); + fix.fallback = Some("created\n".to_string()); + let fixes = vec![(make_issue("INDEX-002"), fix)]; + let results = fixer.apply_and_commit(&[], &fixes).unwrap(); + + assert!(results[0].success); + assert_eq!(std::fs::read_to_string(temp.path().join("new.txt")).unwrap(), "created\n"); + assert_eq!(repo.head().unwrap().peel_to_commit().unwrap().parent_count(), 1); +}