diff --git a/crates/webcodex-core/src/apply_patch_shared.rs b/crates/webcodex-core/src/apply_patch_shared.rs index 333eca59..b25e470a 100644 --- a/crates/webcodex-core/src/apply_patch_shared.rs +++ b/crates/webcodex-core/src/apply_patch_shared.rs @@ -7,12 +7,14 @@ use crate::apply_edits_shared::{ canonicalize_apply_text_line_endings, detect_apply_text_line_ending, restore_apply_text_line_endings, }; +use serde::{Deserialize, Serialize}; use std::fmt; pub const MAX_CODEX_PATCH_BYTES: usize = 256 * 1024; pub const MAX_CODEX_PATCH_FILE_CHANGES: usize = 64; pub const MAX_CODEX_PATCH_CHUNKS_PER_FILE: usize = 256; pub const MAX_CODEX_PATCH_RECOVERY_READ_LINES: usize = 64; +pub const MAX_CODEX_PATCH_CANDIDATE_POSITIONS: usize = 4; const BEGIN_PATCH_MARKER: &str = "*** Begin Patch"; const END_PATCH_MARKER: &str = "*** End Patch"; @@ -81,6 +83,7 @@ pub enum CodexPatchMatchMode { Exact, TrimEnd, Trim, + Normalized, } impl CodexPatchMatchMode { @@ -89,10 +92,35 @@ impl CodexPatchMatchMode { Self::Exact => "exact", Self::TrimEnd => "trim_end", Self::Trim => "trim", + Self::Normalized => "normalized", } } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum ApplyPatchMatchingMode { + FirstMatch, + Unique, + ExactUnique, +} + +impl ApplyPatchMatchingMode { + pub const fn as_str(self) -> &'static str { + match self { + Self::FirstMatch => "first_match", + Self::Unique => "unique", + Self::ExactUnique => "exact_unique", + } + } +} + +impl Default for ApplyPatchMatchingMode { + fn default() -> Self { + Self::Unique + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CodexPatchMatchSource { OldLines, @@ -110,14 +138,20 @@ impl CodexPatchMatchSource { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct CodexPatchStrictMatchRejection { +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CodexPatchMatchRejection { + pub requested_matching_mode: ApplyPatchMatchingMode, pub match_mode: CodexPatchMatchMode, pub match_source: CodexPatchMatchSource, - /// One-based source line where the rejected match candidate starts. + /// One-based selected position. This is authoritative only when the selected + /// tier has one candidate (for example an exact_unique fuzzy rejection). pub matched_start_line: usize, /// Number of candidates at this selected match tier. pub candidate_count: usize, + /// Bounded, ascending candidate positions for recovery observation. These + /// are equal candidates, never a winner/preference signal. + pub candidate_start_lines: Vec, + pub candidate_positions_truncated: bool, /// One-based first candidate position considered for this match tier. pub search_start_line: usize, /// Number of source lines in the canonicalized file used for matching. @@ -134,12 +168,18 @@ pub struct CodexPatchChunkMatch { /// Number of candidates at the selected match mode for match_source. /// Append operations do not perform text matching and report None. pub candidate_count: Option, + /// True when the actual mutation target is unique at its selected tier. + /// For replacement chunks, a repeated change_context does not by itself + /// make the target ambiguous when old_lines resolves to one target. + /// Anchored pure additions still require a unique change_context. + /// Unanchored append is unique-safe. + pub unique_match: bool, /// True only when every text match used to position this chunk was exact /// and unique. Unanchored append performs no text match and is strict-safe. pub strict_match: bool, - /// The specific non-strict component that caused strict positioning to fail. + /// The positioning decision that violates the requested matching mode. /// Ambiguity is preferred over a unique fuzzy component when both exist. - pub strict_rejection: Option, + pub match_rejection: Option, } /// Body-free structural hint for a failed apply_patch text search. @@ -504,72 +544,111 @@ pub fn parse_codex_patch(patch: &str) -> Result { Ok(CodexPatch { hunks }) } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] struct SequenceMatch { index: usize, mode: CodexPatchMatchMode, candidate_count: usize, + candidate_positions: Vec, + search_start: usize, } impl SequenceMatch { - fn is_exact_unique(self) -> bool { + fn is_unique(&self) -> bool { + self.candidate_count == 1 + } + + fn is_exact_unique(&self) -> bool { self.mode == CodexPatchMatchMode::Exact && self.candidate_count == 1 } } -fn strict_match_rejection_fact( - matched: SequenceMatch, +fn match_rejection_fact( + matched: &SequenceMatch, + requested_matching_mode: ApplyPatchMatchingMode, match_source: CodexPatchMatchSource, - search_start: usize, source_line_count: usize, pattern_len: usize, -) -> Option { - if matched.is_exact_unique() || pattern_len == 0 || pattern_len > source_line_count { +) -> Option { + let satisfied = match requested_matching_mode { + ApplyPatchMatchingMode::FirstMatch => true, + ApplyPatchMatchingMode::Unique => matched.is_unique(), + ApplyPatchMatchingMode::ExactUnique => matched.is_exact_unique(), + }; + if satisfied || pattern_len == 0 || pattern_len > source_line_count { return None; } - let last_start = source_line_count.checked_sub(pattern_len)?; - let effective_start = search_start.min(last_start); - Some(CodexPatchStrictMatchRejection { + Some(CodexPatchMatchRejection { + requested_matching_mode, match_mode: matched.mode, match_source, matched_start_line: matched.index.checked_add(1)?, candidate_count: matched.candidate_count, - search_start_line: effective_start.checked_add(1)?, + candidate_start_lines: matched + .candidate_positions + .iter() + .map(|index| index + 1) + .collect(), + candidate_positions_truncated: matched.candidate_count > matched.candidate_positions.len(), + search_start_line: matched.search_start.checked_add(1)?, source_line_count, }) } -fn select_strict_match_rejection( - first: Option, - second: Option, -) -> Option { +fn select_match_rejection( + first: Option, + second: Option, +) -> Option { let candidates = [first, second]; candidates .iter() .flatten() .find(|candidate| candidate.candidate_count > 1) - .copied() + .cloned() .or_else(|| candidates.into_iter().flatten().next()) } +fn normalize_codex_patch_match_text(value: &str) -> String { + value + .trim() + .chars() + .map(|character| match character { + '\u{2010}' | '\u{2011}' | '\u{2012}' | '\u{2013}' | '\u{2014}' | '\u{2015}' + | '\u{2212}' => '-', + '\u{2018}' | '\u{2019}' | '\u{201A}' | '\u{201B}' => '\'', + '\u{201C}' | '\u{201D}' | '\u{201E}' | '\u{201F}' => '"', + '\u{00A0}' | '\u{2002}' | '\u{2003}' | '\u{2004}' | '\u{2005}' | '\u{2006}' + | '\u{2007}' | '\u{2008}' | '\u{2009}' | '\u{200A}' | '\u{202F}' | '\u{205F}' + | '\u{3000}' => ' ', + other => other, + }) + .collect() +} + fn seek_sequence( lines: &[String], pattern: &[String], start: usize, eof: bool, + matching_mode: ApplyPatchMatchingMode, ) -> Option { if pattern.is_empty() { + let index = start.min(lines.len()); return Some(SequenceMatch { - index: start.min(lines.len()), + index, mode: CodexPatchMatchMode::Exact, candidate_count: 1, + candidate_positions: vec![index], + search_start: index, }); } if pattern.len() > lines.len() { return None; } let last_start = lines.len() - pattern.len(); - let start = start.min(last_start); + if start > last_start { + return None; + } let matches_at = |index: usize, mode: CodexPatchMatchMode| { lines[index..index + pattern.len()] @@ -579,6 +658,10 @@ fn seek_sequence( CodexPatchMatchMode::Exact => candidate == expected, CodexPatchMatchMode::TrimEnd => candidate.trim_end() == expected.trim_end(), CodexPatchMatchMode::Trim => candidate.trim() == expected.trim(), + CodexPatchMatchMode::Normalized => { + normalize_codex_patch_match_text(candidate) + == normalize_codex_patch_match_text(expected) + } }) }; @@ -586,12 +669,27 @@ fn seek_sequence( CodexPatchMatchMode::Exact, CodexPatchMatchMode::TrimEnd, CodexPatchMatchMode::Trim, + CodexPatchMatchMode::Normalized, ] { - let mut selected_index = (eof && matches_at(last_start, mode)).then_some(last_start); + // Unique treats the EOF marker as a structural eligibility fence: only + // the trailing candidate participates in uniqueness. FirstMatch keeps + // the legacy/Codex-like tail preference while retaining the ordinary + // search range; ExactUnique preserves the existing strict observable + // candidate-count semantics. + let structural_eof = eof && matching_mode == ApplyPatchMatchingMode::Unique; + let effective_start = if structural_eof { last_start } else { start }; + let mut selected_index = (eof + && matching_mode != ApplyPatchMatchingMode::Unique + && matches_at(last_start, mode)) + .then_some(last_start); let mut candidate_count = 0usize; - for index in start..=last_start { + let mut candidate_positions = Vec::with_capacity(MAX_CODEX_PATCH_CANDIDATE_POSITIONS); + for index in effective_start..=last_start { if matches_at(index, mode) { candidate_count = candidate_count.saturating_add(1); + if candidate_positions.len() < MAX_CODEX_PATCH_CANDIDATE_POSITIONS { + candidate_positions.push(index); + } if selected_index.is_none() { selected_index = Some(index); } @@ -602,6 +700,8 @@ fn seek_sequence( index, mode, candidate_count, + candidate_positions, + search_start: effective_start, }); } } @@ -691,6 +791,20 @@ pub fn derive_codex_patch_update_with_matches( original: &str, path: &str, chunks: &[CodexPatchChunk], +) -> Result { + derive_codex_patch_update_with_matching_mode( + original, + path, + chunks, + ApplyPatchMatchingMode::FirstMatch, + ) +} + +pub fn derive_codex_patch_update_with_matching_mode( + original: &str, + path: &str, + chunks: &[CodexPatchChunk], + matching_mode: ApplyPatchMatchingMode, ) -> Result { let line_ending = detect_apply_text_line_ending(original).map_err(|message| { CodexPatchError::new("unsupported_file", None, format!("{path}: {message}")) @@ -712,13 +826,13 @@ pub fn derive_codex_patch_update_with_matches( let mut line_index = 0usize; for (chunk_index, chunk) in chunks.iter().enumerate() { let mut context_match = None; - let context_search_start = line_index; if let Some(context) = chunk.change_context.as_ref() { let Some(matched_context) = seek_sequence( &original_lines, std::slice::from_ref(context), line_index, false, + matching_mode, ) else { return Err(CodexPatchError::new( "context_mismatch", @@ -733,8 +847,8 @@ pub fn derive_codex_patch_update_with_matches( CodexPatchMatchSource::ChangeContext, ))); }; - context_match = Some(matched_context); line_index = matched_context.index + 1; + context_match = Some(matched_context); } if chunk.old_lines.is_empty() { @@ -744,41 +858,59 @@ pub fn derive_codex_patch_update_with_matches( original_lines.len() }; replacements.push((insertion_index, 0, chunk.new_lines.clone())); - let strict_rejection = context_match.and_then(|matched| { - strict_match_rejection_fact( + let match_rejection = context_match.as_ref().and_then(|matched| { + match_rejection_fact( matched, + matching_mode, CodexPatchMatchSource::ChangeContext, - context_search_start, original_lines.len(), 1, ) }); + let unique_match = context_match.as_ref().is_none_or(SequenceMatch::is_unique); + let strict_match = context_match + .as_ref() + .is_none_or(SequenceMatch::is_exact_unique); chunk_matches.push(CodexPatchChunkMatch { chunk_index, - match_mode: context_match.map(|matched| matched.mode), + match_mode: context_match.as_ref().map(|matched| matched.mode), match_source: if chunk.change_context.is_some() { CodexPatchMatchSource::ChangeContext } else { CodexPatchMatchSource::Append }, matched_start_line: insertion_index + 1, - candidate_count: context_match.map(|matched| matched.candidate_count), - strict_match: strict_rejection.is_none(), - strict_rejection, + candidate_count: context_match + .as_ref() + .map(|matched| matched.candidate_count), + unique_match, + strict_match, + match_rejection, }); continue; } - let old_lines_search_start = line_index; let mut pattern = chunk.old_lines.as_slice(); let mut replacement = chunk.new_lines.as_slice(); - let mut found = seek_sequence(&original_lines, pattern, line_index, chunk.is_end_of_file); + let mut found = seek_sequence( + &original_lines, + pattern, + line_index, + chunk.is_end_of_file, + matching_mode, + ); if found.is_none() && pattern.last().is_some_and(String::is_empty) { pattern = &pattern[..pattern.len() - 1]; if replacement.last().is_some_and(String::is_empty) { replacement = &replacement[..replacement.len() - 1]; } - found = seek_sequence(&original_lines, pattern, line_index, chunk.is_end_of_file); + found = seek_sequence( + &original_lines, + pattern, + line_index, + chunk.is_end_of_file, + matching_mode, + ); } let Some(found) = found else { return Err(CodexPatchError::new( @@ -796,36 +928,52 @@ pub fn derive_codex_patch_update_with_matches( }; let start = found.index; replacements.push((start, pattern.len(), replacement.to_vec())); - let context_rejection = context_match.and_then(|matched| { - strict_match_rejection_fact( - matched, - CodexPatchMatchSource::ChangeContext, - context_search_start, - original_lines.len(), - 1, - ) - }); - let old_lines_rejection = strict_match_rejection_fact( - found, + // `change_context` narrows where old_lines search begins, but when a + // replacement has old_lines the mutation target is the old_lines + // candidate itself. Under the normal Unique mode, repeated anchors do + // not constitute real target ambiguity if that final candidate is + // unique. ExactUnique deliberately keeps the stronger requirement that + // every textual positioning decision be exact and unique. + let context_rejection = if matching_mode == ApplyPatchMatchingMode::ExactUnique { + context_match.as_ref().and_then(|matched| { + match_rejection_fact( + matched, + matching_mode, + CodexPatchMatchSource::ChangeContext, + original_lines.len(), + 1, + ) + }) + } else { + None + }; + let old_lines_rejection = match_rejection_fact( + &found, + matching_mode, CodexPatchMatchSource::OldLines, - old_lines_search_start, original_lines.len(), pattern.len(), ); - let strict_rejection = - select_strict_match_rejection(context_rejection, old_lines_rejection); + let match_rejection = select_match_rejection(context_rejection, old_lines_rejection); + let unique_match = found.is_unique(); + let strict_match = context_match + .as_ref() + .is_none_or(SequenceMatch::is_exact_unique) + && found.is_exact_unique(); chunk_matches.push(CodexPatchChunkMatch { chunk_index, match_mode: Some( context_match + .as_ref() .map(|matched_context| matched_context.mode.max(found.mode)) .unwrap_or(found.mode), ), match_source: CodexPatchMatchSource::OldLines, matched_start_line: start + 1, candidate_count: Some(found.candidate_count), - strict_match: strict_rejection.is_none(), - strict_rejection, + unique_match, + strict_match, + match_rejection, }); line_index = start + pattern.len(); } @@ -1025,7 +1173,7 @@ mod tests { #[test] fn strict_rejection_fact_prefers_ambiguity_across_context_and_old_lines() { - let context_ambiguous = derive_codex_patch_update_with_matches( + let context_ambiguous = derive_codex_patch_update_with_matching_mode( "ctx\n foo \nctx\nother\n", "file.txt", &[CodexPatchChunk { @@ -1034,9 +1182,13 @@ mod tests { new_lines: vec!["new".to_string()], is_end_of_file: false, }], + ApplyPatchMatchingMode::ExactUnique, ) .unwrap(); - let rejection = context_ambiguous.chunk_matches[0].strict_rejection.unwrap(); + let rejection = context_ambiguous.chunk_matches[0] + .match_rejection + .clone() + .unwrap(); assert_eq!(rejection.match_source, CodexPatchMatchSource::ChangeContext); assert_eq!(rejection.match_mode, CodexPatchMatchMode::Exact); assert_eq!(rejection.candidate_count, 2); @@ -1044,7 +1196,7 @@ mod tests { assert_eq!(rejection.search_start_line, 1); assert_eq!(rejection.source_line_count, 4); - let old_lines_ambiguous = derive_codex_patch_update_with_matches( + let old_lines_ambiguous = derive_codex_patch_update_with_matching_mode( " ctx \ndup\nother\ndup\n", "file.txt", &[CodexPatchChunk { @@ -1053,10 +1205,12 @@ mod tests { new_lines: vec!["new".to_string()], is_end_of_file: false, }], + ApplyPatchMatchingMode::ExactUnique, ) .unwrap(); let rejection = old_lines_ambiguous.chunk_matches[0] - .strict_rejection + .match_rejection + .clone() .unwrap(); assert_eq!(rejection.match_source, CodexPatchMatchSource::OldLines); assert_eq!(rejection.match_mode, CodexPatchMatchMode::Exact); @@ -1102,6 +1256,302 @@ mod tests { assert!(updated.chunk_matches[0].strict_match); } + #[test] + fn unique_accepts_each_matching_tier_when_the_eligible_candidate_is_unique() { + for (name, original, old_line, expected_mode) in [ + ("exact", "target\n", "target", CodexPatchMatchMode::Exact), + ( + "trim_end", + "target \n", + "target", + CodexPatchMatchMode::TrimEnd, + ), + ("trim", " target \n", "target", CodexPatchMatchMode::Trim), + ( + "normalized_dash", + "alpha—beta\n", + "alpha-beta", + CodexPatchMatchMode::Normalized, + ), + ( + "normalized_quote", + "it’s “quoted”\n", + "it's \"quoted\"", + CodexPatchMatchMode::Normalized, + ), + ( + "normalized_space", + "alpha\u{00a0}beta\u{3000}gamma\n", + "alpha beta gamma", + CodexPatchMatchMode::Normalized, + ), + ] { + let update = derive_codex_patch_update_with_matching_mode( + original, + "file.txt", + &[CodexPatchChunk { + old_lines: vec![old_line.to_string()], + new_lines: vec!["changed".to_string()], + ..Default::default() + }], + ApplyPatchMatchingMode::Unique, + ) + .unwrap_or_else(|error| panic!("{name}: {error:?}")); + let matched = &update.chunk_matches[0]; + assert_eq!(matched.match_mode, Some(expected_mode), "{name}"); + assert_eq!(matched.candidate_count, Some(1), "{name}"); + assert!(matched.unique_match, "{name}"); + assert!(matched.match_rejection.is_none(), "{name}"); + } + } + + #[test] + fn unique_rejects_duplicate_candidates_at_every_selected_tier() { + for (name, original, old_line, expected_mode) in [ + ( + "exact", + "target\nother\ntarget\n", + "target", + CodexPatchMatchMode::Exact, + ), + ( + "trim_end", + "target \nother\ntarget\t\n", + "target", + CodexPatchMatchMode::TrimEnd, + ), + ( + "trim", + " target \nother\n\ttarget\t\n", + "target", + CodexPatchMatchMode::Trim, + ), + ( + "normalized", + "alpha—beta\nother\nalpha–beta\n", + "alpha-beta", + CodexPatchMatchMode::Normalized, + ), + ] { + let update = derive_codex_patch_update_with_matching_mode( + original, + "file.txt", + &[CodexPatchChunk { + old_lines: vec![old_line.to_string()], + new_lines: vec!["changed".to_string()], + ..Default::default() + }], + ApplyPatchMatchingMode::Unique, + ) + .unwrap(); + let rejection = update.chunk_matches[0] + .match_rejection + .as_ref() + .unwrap_or_else(|| panic!("{name}: expected ambiguity")); + assert_eq!(rejection.match_mode, expected_mode, "{name}"); + assert_eq!(rejection.candidate_count, 2, "{name}"); + assert_eq!(rejection.candidate_start_lines.len(), 2, "{name}"); + assert!(!rejection.candidate_positions_truncated, "{name}"); + assert!(!update.chunk_matches[0].unique_match, "{name}"); + } + } + + #[test] + fn unique_exact_tier_beats_an_earlier_fuzzy_candidate() { + let update = derive_codex_patch_update_with_matching_mode( + " target \ntarget\n", + "file.txt", + &[CodexPatchChunk { + old_lines: vec!["target".into()], + new_lines: vec!["changed".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::Unique, + ) + .unwrap(); + assert_eq!(update.content, " target \nchanged\n"); + assert_eq!( + update.chunk_matches[0].match_mode, + Some(CodexPatchMatchMode::Exact) + ); + assert_eq!(update.chunk_matches[0].matched_start_line, 2); + assert!(update.chunk_matches[0].unique_match); + assert!(update.chunk_matches[0].match_rejection.is_none()); + } + + #[test] + fn unique_accepts_repeated_change_context_when_old_lines_target_is_unique() { + let update = derive_codex_patch_update_with_matching_mode( + "ctx\nold\nctx\nother\n", + "file.txt", + &[CodexPatchChunk { + change_context: Some("ctx".into()), + old_lines: vec!["old".into()], + new_lines: vec!["new".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::Unique, + ) + .unwrap(); + assert_eq!(update.content, "ctx\nnew\nctx\nother\n"); + assert_eq!(update.chunk_matches[0].matched_start_line, 2); + assert_eq!(update.chunk_matches[0].candidate_count, Some(1)); + assert!(update.chunk_matches[0].unique_match); + assert!(!update.chunk_matches[0].strict_match); + assert!(update.chunk_matches[0].match_rejection.is_none()); + } + + #[test] + fn unique_still_rejects_repeated_change_context_for_pure_addition() { + let update = derive_codex_patch_update_with_matching_mode( + "ctx\nfirst\nctx\nsecond\n", + "file.txt", + &[CodexPatchChunk { + change_context: Some("ctx".into()), + old_lines: Vec::new(), + new_lines: vec!["inserted".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::Unique, + ) + .unwrap(); + let rejection = update.chunk_matches[0].match_rejection.as_ref().unwrap(); + assert_eq!(rejection.match_source, CodexPatchMatchSource::ChangeContext); + assert_eq!(rejection.candidate_count, 2); + assert!(!update.chunk_matches[0].unique_match); + } + + #[test] + fn later_chunks_never_backtrack_before_the_prior_match() { + for matching_mode in [ + ApplyPatchMatchingMode::FirstMatch, + ApplyPatchMatchingMode::Unique, + ApplyPatchMatchingMode::ExactUnique, + ] { + let error = derive_codex_patch_update_with_matching_mode( + "head\nmid\ntail\n", + "file.txt", + &[ + CodexPatchChunk { + old_lines: vec!["tail".into()], + new_lines: vec!["TAIL".into()], + ..Default::default() + }, + CodexPatchChunk { + old_lines: vec!["mid".into(), "tail".into()], + new_lines: vec!["must-not-backtrack".into()], + ..Default::default() + }, + ], + matching_mode, + ) + .expect_err("a later chunk must not search before the prior match"); + assert_eq!(error.kind, "context_mismatch", "{matching_mode:?}"); + let diagnostic = error.match_diagnostic.expect("match diagnostic"); + assert_eq!(diagnostic.chunk_index, 1, "{matching_mode:?}"); + assert_eq!(diagnostic.search_start_line, 4, "{matching_mode:?}"); + } + } + + #[test] + fn eof_constraint_is_structurally_unique_only_for_unique_mode() { + let chunk = CodexPatchChunk { + old_lines: vec!["same".into()], + new_lines: vec!["last".into()], + is_end_of_file: true, + ..Default::default() + }; + let unique = derive_codex_patch_update_with_matching_mode( + "same\nmid\nsame\n", + "file.txt", + std::slice::from_ref(&chunk), + ApplyPatchMatchingMode::Unique, + ) + .unwrap(); + assert_eq!(unique.content, "same\nmid\nlast\n"); + assert_eq!(unique.chunk_matches[0].candidate_count, Some(1)); + assert!(unique.chunk_matches[0].unique_match); + assert!(unique.chunk_matches[0].match_rejection.is_none()); + + let exact_unique = derive_codex_patch_update_with_matching_mode( + "same\nmid\nsame\n", + "file.txt", + &[chunk], + ApplyPatchMatchingMode::ExactUnique, + ) + .unwrap(); + let rejection = exact_unique.chunk_matches[0] + .match_rejection + .as_ref() + .unwrap(); + assert_eq!(rejection.match_mode, CodexPatchMatchMode::Exact); + assert_eq!(rejection.candidate_count, 2); + } + + #[test] + fn exact_unique_rejects_unique_trim_and_normalized_candidates() { + for (original, old_line, expected_mode) in [ + (" target \n", "target", CodexPatchMatchMode::Trim), + ( + "alpha—beta\n", + "alpha-beta", + CodexPatchMatchMode::Normalized, + ), + ] { + let update = derive_codex_patch_update_with_matching_mode( + original, + "file.txt", + &[CodexPatchChunk { + old_lines: vec![old_line.into()], + new_lines: vec!["changed".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::ExactUnique, + ) + .unwrap(); + let rejection = update.chunk_matches[0].match_rejection.as_ref().unwrap(); + assert_eq!(rejection.match_mode, expected_mode); + assert_eq!(rejection.candidate_count, 1); + assert!(!update.chunk_matches[0].strict_match); + } + } + + #[test] + fn first_match_is_deterministic_and_keeps_tier_priority() { + let repeated = derive_codex_patch_update_with_matching_mode( + "same\nmid\nsame\n", + "file.txt", + &[CodexPatchChunk { + old_lines: vec!["same".into()], + new_lines: vec!["first".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::FirstMatch, + ) + .unwrap(); + assert_eq!(repeated.content, "first\nmid\nsame\n"); + assert_eq!(repeated.chunk_matches[0].candidate_count, Some(2)); + assert!(repeated.chunk_matches[0].match_rejection.is_none()); + + let tiered = derive_codex_patch_update_with_matching_mode( + " target \ntarget\n", + "file.txt", + &[CodexPatchChunk { + old_lines: vec!["target".into()], + new_lines: vec!["exact".into()], + ..Default::default() + }], + ApplyPatchMatchingMode::FirstMatch, + ) + .unwrap(); + assert_eq!(tiered.content, " target \nexact\n"); + assert_eq!( + tiered.chunk_matches[0].match_mode, + Some(CodexPatchMatchMode::Exact) + ); + assert_eq!(tiered.chunk_matches[0].matched_start_line, 2); + } + #[test] fn context_mismatch_reports_body_free_nearest_match_diagnostic() { let chunks = vec![CodexPatchChunk { diff --git a/crates/webcodex-core/src/runner_protocol.rs b/crates/webcodex-core/src/runner_protocol.rs index fece5e30..7553b834 100644 --- a/crates/webcodex-core/src/runner_protocol.rs +++ b/crates/webcodex-core/src/runner_protocol.rs @@ -168,10 +168,15 @@ pub const RUNNER_CAPABILITY_APPLY_PATCH: &str = "apply_patch"; /// current Server must reject apply_patch before dispatch rather than accepting a /// legacy success shape. pub const RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA: &str = "apply_patch_match_metadata"; +/// The Runner understands the 0.4 model-facing apply_patch matching_mode enum +/// (`first_match`, `unique`, `exact_unique`) and returns metadata bound to the +/// requested mode. Missing on older Runners is false; current Servers fail +/// closed instead of silently falling back to legacy permissive positioning. +pub const RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE: &str = "apply_patch_matching_mode"; /// The Runner understands `strict_matching=true` for apply_patch and rejects /// any update chunk whose positioning is not exact and unique before writing. -/// Missing on older Runners is false and is never inferred from apply_patch or -/// apply_patch_match_metadata. +/// This legacy wire capability is retained only so older Servers can roll +/// against a current Runner; current model-facing contracts use matching_mode. pub const RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING: &str = "apply_patch_strict_matching"; pub const RUNNER_CAPABILITY_GIT: &str = "git"; pub const RUNNER_CAPABILITY_JOBS: &str = "jobs"; @@ -404,6 +409,7 @@ pub const RUNNER_CAPABILITY_NAMES: &[&str] = &[ RUNNER_CAPABILITY_APPLY_TEXT_EDIT_LINE_SCOPE, RUNNER_CAPABILITY_APPLY_PATCH, RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA, + RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING, RUNNER_CAPABILITY_GIT, RUNNER_CAPABILITY_JOBS, @@ -527,9 +533,13 @@ pub struct RunnerCapabilities { /// results. Missing on older Runners is false and never follows from apply_patch. #[serde(default, skip_serializing_if = "is_false")] pub apply_patch_match_metadata: bool, + /// Current enum-based apply_patch positioning semantics. Missing on older + /// Runners is false and must fail closed for current model-facing requests. + #[serde(default, skip_serializing_if = "is_false")] + pub apply_patch_matching_mode: bool, /// Fail-closed exact-and-unique positioning for apply_patch requests that - /// explicitly opt into strict_matching. Missing on older Runners is false and - /// requires the current match-metadata success contract. + /// arrive from a legacy Server as strict_matching=true. New Servers do not + /// use this bool as model-facing authority. #[serde(default, skip_serializing_if = "is_false")] pub apply_patch_strict_matching: bool, #[serde(default)] @@ -892,6 +902,7 @@ impl Default for RunnerCapabilities { apply_text_edit_line_scope: false, apply_patch: false, apply_patch_match_metadata: false, + apply_patch_matching_mode: false, apply_patch_strict_matching: false, git: false, jobs: false, @@ -3655,6 +3666,7 @@ mod envelope_tests { apply_text_edit_line_scope: false, apply_patch: false, apply_patch_match_metadata: false, + apply_patch_matching_mode: false, apply_patch_strict_matching: false, git: false, jobs: true, diff --git a/crates/webcodex-core/tests/apply_patch_matching_benchmark.rs b/crates/webcodex-core/tests/apply_patch_matching_benchmark.rs new file mode 100644 index 00000000..ecc864c3 --- /dev/null +++ b/crates/webcodex-core/tests/apply_patch_matching_benchmark.rs @@ -0,0 +1,610 @@ +use std::collections::BTreeMap; + +use webcodex_core::apply_patch_shared::{ + derive_codex_patch_update_with_matching_mode, ApplyPatchMatchingMode, CodexPatchChunk, +}; + +#[derive(Clone)] +struct FileFixture { + path: &'static str, + original: &'static str, + chunks: Vec, + expected_content: &'static str, +} + +struct Case { + name: &'static str, + files: Vec, + baseline_accept: bool, + unique_accept: bool, +} + +fn chunk( + old: &'static str, + new: &'static str, + context: Option<&'static str>, + eof: bool, +) -> CodexPatchChunk { + CodexPatchChunk { + change_context: context.map(str::to_string), + old_lines: vec![old.to_string()], + new_lines: vec![new.to_string()], + is_end_of_file: eof, + } +} + +fn file( + path: &'static str, + original: &'static str, + chunks: Vec, + expected_content: &'static str, +) -> FileFixture { + FileFixture { + path, + original, + chunks, + expected_content, + } +} + +fn case( + name: &'static str, + fixture: FileFixture, + baseline_accept: bool, + unique_accept: bool, +) -> Case { + Case { + name, + files: vec![fixture], + baseline_accept, + unique_accept, + } +} + +#[derive(Default)] +struct Evaluation { + accepted: bool, + reason: &'static str, + wrong_location_writes: usize, + partial_writes: usize, +} + +fn evaluate(case: &Case, mode: ApplyPatchMatchingMode) -> Evaluation { + let mut planned = Vec::with_capacity(case.files.len()); + for fixture in &case.files { + let update = match derive_codex_patch_update_with_matching_mode( + fixture.original, + fixture.path, + &fixture.chunks, + mode, + ) { + Ok(update) => update, + Err(error) => { + return Evaluation { + accepted: false, + reason: if error.kind == "context_mismatch" { + "context_mismatch" + } else { + "other_rejection" + }, + ..Default::default() + }; + } + }; + if let Some(rejection) = update + .chunk_matches + .iter() + .find_map(|matched| matched.match_rejection.as_ref()) + { + return Evaluation { + accepted: false, + reason: if rejection.candidate_count > 1 { + "ambiguous_candidate" + } else { + "unique_non_exact_candidate" + }, + ..Default::default() + }; + } + planned.push((fixture, update.content)); + } + + // The benchmark simulates the real transaction boundary: no workspace state + // changes until every file/hunk has passed preflight. Therefore rejected + // cases can never produce a partial write in this corpus. + let wrong_location_writes = planned + .iter() + .filter(|(fixture, content)| content.as_str() != fixture.expected_content) + .count(); + Evaluation { + accepted: true, + reason: "accepted", + wrong_location_writes, + partial_writes: 0, + } +} + +fn corpus() -> Vec { + vec![ + case( + "ordinary_exact_rust", + file( + "src/lib.rs", + "let x = 1;\n", + vec![chunk("let x = 1;", "let x = 2;", None, false)], + "let x = 2;\n", + ), + true, + true, + ), + case( + "ordinary_exact_python", + file( + "app.py", + "value = 1\n", + vec![chunk("value = 1", "value = 2", None, false)], + "value = 2\n", + ), + true, + true, + ), + case( + "ordinary_exact_ts", + file( + "ui.ts", + "const a = 1;\n", + vec![chunk("const a = 1;", "const a = 2;", None, false)], + "const a = 2;\n", + ), + true, + true, + ), + case( + "docs_exact", + file( + "README.md", + "old heading\n", + vec![chunk("old heading", "new heading", None, false)], + "new heading\n", + ), + true, + true, + ), + case( + "comment_exact", + file( + "src/a.rs", + "// old\n", + vec![chunk("// old", "// new", None, false)], + "// new\n", + ), + true, + true, + ), + case( + "assert_exact", + file( + "tests/a.rs", + "assert_eq!(x, 1);\n", + vec![chunk("assert_eq!(x, 1);", "assert_eq!(x, 2);", None, false)], + "assert_eq!(x, 2);\n", + ), + true, + true, + ), + case( + "trim_end_spaces", + file( + "a.txt", + "target \n", + vec![chunk("target", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "trim_end_tab", + file( + "a.txt", + "target\t\n", + vec![chunk("target", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "trim_end_comment", + file( + "a.rs", + "// target \n", + vec![chunk("// target", "// changed", None, false)], + "// changed\n", + ), + false, + true, + ), + case( + "trim_end_assert", + file( + "a.rs", + "assert!(ready); \n", + vec![chunk("assert!(ready);", "assert!(done);", None, false)], + "assert!(done);\n", + ), + false, + true, + ), + case( + "trim_both_spaces", + file( + "a.txt", + " target \n", + vec![chunk("target", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "trim_both_tabs", + file( + "a.txt", + "\ttarget\t\n", + vec![chunk("target", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "trim_indented_helper", + file( + "a.py", + " helper() \n", + vec![chunk("helper()", "changed()", None, false)], + "changed()\n", + ), + false, + true, + ), + case( + "trim_indented_assert", + file( + "a.py", + " assert value \n", + vec![chunk("assert value", "assert changed", None, false)], + "assert changed\n", + ), + false, + true, + ), + case( + "normalized_em_dash", + file( + "docs.md", + "alpha—beta\n", + vec![chunk("alpha-beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_en_dash", + file( + "docs.md", + "alpha–beta\n", + vec![chunk("alpha-beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_minus", + file( + "docs.md", + "alpha−beta\n", + vec![chunk("alpha-beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_smart_single", + file( + "docs.md", + "it’s ready\n", + vec![chunk("it's ready", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_smart_double", + file( + "docs.md", + "say “ready”\n", + vec![chunk("say \"ready\"", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_nbsp", + file( + "docs.md", + "alpha\u{00a0}beta\n", + vec![chunk("alpha beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "normalized_ideographic_space", + file( + "docs.md", + "alpha\u{3000}beta\n", + vec![chunk("alpha beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "tier_exact_beats_fuzzy", + file( + "a.txt", + " target \ntarget\n", + vec![chunk("target", "changed", None, false)], + " target \nchanged\n", + ), + true, + true, + ), + case( + "anchored_repeated_helper", + file( + "a.py", + "def a():\nhelper()\ndef b():\nhelper()\n", + vec![chunk("helper()", "changed()", Some("def b():"), false)], + "def a():\nhelper()\ndef b():\nchanged()\n", + ), + true, + true, + ), + case( + "anchored_repeated_assert_with_drift", + file( + "a.py", + "def a():\nassert value\ndef b():\n assert value \n", + vec![chunk( + "assert value", + "assert changed", + Some("def b():"), + false, + )], + "def a():\nassert value\ndef b():\nassert changed\n", + ), + false, + true, + ), + case( + "eof_duplicate_structural", + file( + "a.txt", + "same\nmid\nsame\n", + vec![chunk("same", "last", None, true)], + "same\nmid\nlast\n", + ), + false, + true, + ), + case( + "ambiguous_exact", + file( + "a.txt", + "dup\nmid\ndup\n", + vec![chunk("dup", "changed", None, false)], + "dup\nmid\ndup\n", + ), + false, + false, + ), + case( + "ambiguous_trim_end", + file( + "a.txt", + "dup \nmid\ndup\t\n", + vec![chunk("dup", "changed", None, false)], + "dup \nmid\ndup\t\n", + ), + false, + false, + ), + case( + "ambiguous_trim", + file( + "a.txt", + " dup \nmid\n\tdup\t\n", + vec![chunk("dup", "changed", None, false)], + " dup \nmid\n\tdup\t\n", + ), + false, + false, + ), + case( + "ambiguous_normalized", + file( + "a.txt", + "alpha—beta\nmid\nalpha–beta\n", + vec![chunk("alpha-beta", "changed", None, false)], + "alpha—beta\nmid\nalpha–beta\n", + ), + false, + false, + ), + case( + "repeated_parent_context_unique_target", + file( + "a.txt", + "ctx\nold\nctx\nother\n", + vec![chunk("old", "new", Some("ctx"), false)], + "ctx\nnew\nctx\nother\n", + ), + false, + true, + ), + case( + "context_mismatch", + file( + "a.txt", + "actual\n", + vec![chunk("missing", "changed", None, false)], + "actual\n", + ), + false, + false, + ), + Case { + name: "multi_hunk_mixed_exact_trim", + files: vec![file( + "a.txt", + "one\n two \n", + vec![ + chunk("one", "ONE", None, false), + chunk("two", "TWO", None, false), + ], + "ONE\nTWO\n", + )], + baseline_accept: false, + unique_accept: true, + }, + Case { + name: "multi_file_exact_normalized", + files: vec![ + file( + "a.txt", + "old\n", + vec![chunk("old", "new", None, false)], + "new\n", + ), + file( + "b.txt", + "alpha—beta\n", + vec![chunk("alpha-beta", "changed", None, false)], + "changed\n", + ), + ], + baseline_accept: false, + unique_accept: true, + }, + Case { + name: "multi_file_later_true_ambiguity", + files: vec![ + file( + "safe.txt", + "old\n", + vec![chunk("old", "new", None, false)], + "new\n", + ), + file( + "risky.txt", + "dup\nmid\ndup\n", + vec![chunk("dup", "changed", None, false)], + "dup\nmid\ndup\n", + ), + ], + baseline_accept: false, + unique_accept: false, + }, + case( + "unicode_narrow_nbsp", + file( + "docs.md", + "alpha\u{202f}beta\n", + vec![chunk("alpha beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + case( + "unicode_medium_space", + file( + "docs.md", + "alpha\u{205f}beta\n", + vec![chunk("alpha beta", "changed", None, false)], + "changed\n", + ), + false, + true, + ), + ] +} + +#[test] +fn deterministic_current_main_strict_vs_unique_corpus() { + let corpus = corpus(); + assert!( + (25..=40).contains(&corpus.len()), + "benchmark corpus must remain representative and bounded" + ); + + let mut baseline_accepted = 0usize; + let mut unique_accepted = 0usize; + let mut baseline_reasons = BTreeMap::<&'static str, usize>::new(); + let mut unique_reasons = BTreeMap::<&'static str, usize>::new(); + let mut wrong_location_writes = 0usize; + let mut partial_writes = 0usize; + + for case in &corpus { + // Baseline is the exact+unique behavior of current-main + // `strict_matching=true`, which is the deployed model/reviewer policy + // this P0 is replacing as the normal path. This intentionally does not + // pretend current-main's schema-default permissive false path was the + // observed dogfood strategy. + let baseline = evaluate(case, ApplyPatchMatchingMode::ExactUnique); + let unique = evaluate(case, ApplyPatchMatchingMode::Unique); + assert_eq!( + baseline.accepted, case.baseline_accept, + "baseline: {}", + case.name + ); + assert_eq!(unique.accepted, case.unique_accept, "unique: {}", case.name); + baseline_accepted += usize::from(baseline.accepted); + unique_accepted += usize::from(unique.accepted); + *baseline_reasons.entry(baseline.reason).or_default() += 1; + *unique_reasons.entry(unique.reason).or_default() += 1; + wrong_location_writes += baseline.wrong_location_writes + unique.wrong_location_writes; + partial_writes += baseline.partial_writes + unique.partial_writes; + } + + assert_eq!( + wrong_location_writes, 0, + "matcher must never accept a wrong target in corpus" + ); + assert_eq!( + partial_writes, 0, + "preflight simulation must never partially write" + ); + assert!(unique_accepted > baseline_accepted); + println!( + "apply_patch matcher corpus cases={} current_main_strict accepted={} rejected={} reasons={:?}; unique accepted={} rejected={} reasons={:?}; wrong_location_writes={}; partial_writes={}", + corpus.len(), + baseline_accepted, + corpus.len() - baseline_accepted, + baseline_reasons, + unique_accepted, + corpus.len() - unique_accepted, + unique_reasons, + wrong_location_writes, + partial_writes, + ); +} diff --git a/crates/webcodex-runner-config/src/lib.rs b/crates/webcodex-runner-config/src/lib.rs index 1bd01ccd..2d86c6e9 100644 --- a/crates/webcodex-runner-config/src/lib.rs +++ b/crates/webcodex-runner-config/src/lib.rs @@ -228,8 +228,11 @@ pub fn generated_runner_config_toml(opts: &RunnerInitOptions) -> Result wire::RUNNER_CAPABILITY_APPLY_TEXT_EDIT_LINE_SCOPE, Self::ApplyPatch => wire::RUNNER_CAPABILITY_APPLY_PATCH, Self::ApplyPatchMatchMetadata => wire::RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA, + Self::ApplyPatchMatchingMode => wire::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, Self::ApplyPatchStrictMatching => wire::RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING, Self::Git => wire::RUNNER_CAPABILITY_GIT, Self::Jobs => wire::RUNNER_CAPABILITY_JOBS, @@ -224,6 +227,7 @@ impl RunnerFeature { wire::RUNNER_CAPABILITY_APPLY_TEXT_EDIT_LINE_SCOPE => Self::ApplyTextEditLineScope, wire::RUNNER_CAPABILITY_APPLY_PATCH => Self::ApplyPatch, wire::RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA => Self::ApplyPatchMatchMetadata, + wire::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE => Self::ApplyPatchMatchingMode, wire::RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING => Self::ApplyPatchStrictMatching, wire::RUNNER_CAPABILITY_GIT => Self::Git, wire::RUNNER_CAPABILITY_JOBS => Self::Jobs, @@ -311,6 +315,7 @@ impl RunnerFeature { | Self::ApplyTextEditLineScope | Self::ApplyPatch | Self::ApplyPatchMatchMetadata + | Self::ApplyPatchMatchingMode | Self::ApplyPatchStrictMatching | Self::SshShell | Self::PersistentShell @@ -355,6 +360,7 @@ impl RunnerFeature { Self::ApplyTextEditLineScope => capabilities.apply_text_edit_line_scope, Self::ApplyPatch => capabilities.apply_patch, Self::ApplyPatchMatchMetadata => capabilities.apply_patch_match_metadata, + Self::ApplyPatchMatchingMode => capabilities.apply_patch_matching_mode, Self::ApplyPatchStrictMatching => capabilities.apply_patch_strict_matching, Self::Git => capabilities.git, Self::Jobs => capabilities.jobs, diff --git a/crates/webcodex-runner-registry/src/requests.rs b/crates/webcodex-runner-registry/src/requests.rs index 4c09c4a2..2772d226 100644 --- a/crates/webcodex-runner-registry/src/requests.rs +++ b/crates/webcodex-runner-registry/src/requests.rs @@ -35,7 +35,7 @@ use webcodex_core::runner_protocol::{ RunnerConfigOperationRequest, RunnerRequest, ShellFileOpRequest, ShellJobContext, ShellProcessArgv, ShellRunRequest, ShellRunResponse, ShellScriptPayload, RAW_SHELL_COMMAND_MAX_BYTES, RUNNER_CAPABILITY_APPLY_PATCH, - RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA, RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING, + RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, RUNNER_CAPABILITY_APPLY_PATCH_MATCH_METADATA, RUNNER_CAPABILITY_APPLY_TEXT_EDIT_LINE_SCOPE, RUNNER_CAPABILITY_APPLY_TEXT_EDIT_OCCURRENCE, RUNNER_CAPABILITY_ARTIFACT_EXPORT_CHUNK_READ, RUNNER_CAPABILITY_ARTIFACT_EXPORT_STREAMING_METADATA, RUNNER_CAPABILITY_FILE_READ, @@ -606,7 +606,6 @@ impl RunnerRegistry { pub async fn enqueue_apply_patch( &self, body: ShellFileOpRequest, - strict_matching: bool, requested_by: String, ) -> Result<(String, oneshot::Receiver), String> { validate_file_request(&body)?; @@ -666,13 +665,12 @@ impl RunnerRegistry { body.client_id )); } - if strict_matching - && !runner - .runner_features - .supports(RunnerFeature::ApplyPatchStrictMatching) + if !runner + .runner_features + .supports(RunnerFeature::ApplyPatchMatchingMode) { return Err(format!( - "capability_unavailable: runner {} does not support {RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING}", + "capability_unavailable: runner {} does not support {RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE}", body.client_id )); } diff --git a/crates/webcodex-runner-registry/src/runners.rs b/crates/webcodex-runner-registry/src/runners.rs index c9e83853..6120dc7d 100644 --- a/crates/webcodex-runner-registry/src/runners.rs +++ b/crates/webcodex-runner-registry/src/runners.rs @@ -260,6 +260,14 @@ impl RunnerRegistry { .to_string(), ); } + if runner_features.supports(RunnerFeature::ApplyPatchMatchingMode) + && !runner_features.supports(RunnerFeature::ApplyPatchMatchMetadata) + { + return Err( + "apply_patch_matching_mode capability requires apply_patch_match_metadata capability" + .to_string(), + ); + } let job_inventory = body.job_inventory.clone(); let coding_agent_providers = body.coding_agent_providers.clone(); let coding_agent_inventory = body.coding_agent_inventory.clone(); diff --git a/crates/webcodex-runner-registry/src/tests/apply_patch.rs b/crates/webcodex-runner-registry/src/tests/apply_patch.rs index 5a439733..984b5307 100644 --- a/crates/webcodex-runner-registry/src/tests/apply_patch.rs +++ b/crates/webcodex-runner-registry/src/tests/apply_patch.rs @@ -1,13 +1,11 @@ use super::*; -fn patch_request(client_id: &str, strict_matching: bool) -> ShellFileOpRequest { - let mut content = serde_json::json!({ +fn patch_request(client_id: &str) -> ShellFileOpRequest { + let content = serde_json::json!({ "patch": "*** Begin Patch\n*** Update File: src/lib.rs\n-old\n+new\n*** End Patch", "dry_run": false, + "matching_mode": "unique", }); - if strict_matching { - content["strict_matching"] = serde_json::json!(true); - } ShellFileOpRequest { op: "apply_patch".to_string(), client_id: client_id.to_string(), @@ -32,6 +30,7 @@ async fn register_patch_instance( client_id: &str, supported: bool, metadata_supported: bool, + matching_mode_supported: bool, strict_supported: bool, ) -> Result { register_instance_with_capabilities( @@ -42,6 +41,7 @@ async fn register_patch_instance( file_write: true, apply_patch: supported, apply_patch_match_metadata: metadata_supported, + apply_patch_matching_mode: matching_mode_supported, apply_patch_strict_matching: strict_supported, ..Default::default() }, @@ -52,15 +52,11 @@ async fn register_patch_instance( #[tokio::test] async fn enqueue_apply_patch_requires_explicit_capability_and_queues_atomically() { let registry = RunnerRegistry::default(); - register_patch_instance(®istry, "patch-off", false, false, false) + register_patch_instance(®istry, "patch-off", false, false, false, false) .await .unwrap(); let error = registry - .enqueue_apply_patch( - patch_request("patch-off", false), - false, - "tester".to_string(), - ) + .enqueue_apply_patch(patch_request("patch-off"), "tester".to_string()) .await .unwrap_err(); assert!(error.contains("capability_unavailable"), "{error}"); @@ -74,15 +70,11 @@ async fn enqueue_apply_patch_requires_explicit_capability_and_queues_atomically( .unwrap() .is_none()); - register_patch_instance(®istry, "legacy-patch", true, false, false) + register_patch_instance(®istry, "legacy-patch", true, false, false, false) .await .unwrap(); let error = registry - .enqueue_apply_patch( - patch_request("legacy-patch", false), - false, - "tester".to_string(), - ) + .enqueue_apply_patch(patch_request("legacy-patch"), "tester".to_string()) .await .unwrap_err(); assert!(error.contains("capability_unavailable"), "{error}"); @@ -96,75 +88,51 @@ async fn enqueue_apply_patch_requires_explicit_capability_and_queues_atomically( .unwrap() .is_none()); - register_patch_instance(®istry, "patch-on", true, true, false) - .await - .unwrap(); - let (request_id, _rx) = registry - .enqueue_apply_patch( - patch_request("patch-on", false), - false, - "tester".to_string(), - ) - .await - .expect("current Runner should accept ordinary apply_patch"); - let queued = registry - .poll(RunnerPollRequest { - client_id: "patch-on".to_string(), - runner_instance_id: "inst".to_string(), - }) - .await - .unwrap() - .expect("apply_patch request should be queued"); - assert_eq!(queued.request_id, request_id); - assert_eq!(queued.kind, "file_apply_patch"); - assert!(queued - .content - .as_deref() - .unwrap() - .contains("*** Begin Patch")); - - register_patch_instance(®istry, "strict-off", true, true, false) + register_patch_instance(®istry, "mode-off", true, true, false, false) .await .unwrap(); let error = registry - .enqueue_apply_patch( - patch_request("strict-off", true), - true, - "tester".to_string(), - ) + .enqueue_apply_patch(patch_request("mode-off"), "tester".to_string()) .await .unwrap_err(); assert!(error.contains("capability_unavailable"), "{error}"); - assert!(error.contains("apply_patch_strict_matching"), "{error}"); + assert!(error.contains("apply_patch_matching_mode"), "{error}"); assert!(registry .poll(RunnerPollRequest { - client_id: "strict-off".to_string(), + client_id: "mode-off".to_string(), runner_instance_id: "inst".to_string(), }) .await .unwrap() .is_none()); - register_patch_instance(®istry, "strict-on", true, true, true) + register_patch_instance(®istry, "patch-on", true, true, true, false) .await .unwrap(); - let (_, _) = registry - .enqueue_apply_patch(patch_request("strict-on", true), true, "tester".to_string()) + let (request_id, _rx) = registry + .enqueue_apply_patch(patch_request("patch-on"), "tester".to_string()) .await - .expect("strict-capable Runner should accept strict apply_patch"); + .expect("current Runner should accept ordinary apply_patch"); let queued = registry .poll(RunnerPollRequest { - client_id: "strict-on".to_string(), + client_id: "patch-on".to_string(), runner_instance_id: "inst".to_string(), }) .await .unwrap() - .expect("strict apply_patch request should be queued"); + .expect("apply_patch request should be queued"); + assert_eq!(queued.request_id, request_id); + assert_eq!(queued.kind, "file_apply_patch"); + assert!(queued + .content + .as_deref() + .unwrap() + .contains("*** Begin Patch")); assert!(queued .content .as_deref() .unwrap() - .contains("\"strict_matching\":true")); + .contains("\"matching_mode\":\"unique\"")); } #[test] @@ -175,9 +143,11 @@ fn apply_patch_missing_capability_defaults_false_and_is_omitted() { .unwrap(); assert!(!legacy.apply_patch); assert!(!legacy.apply_patch_match_metadata); + assert!(!legacy.apply_patch_matching_mode); assert!(!legacy.apply_patch_strict_matching); let serialized = serde_json::to_value(RunnerCapabilities::default()).unwrap(); assert!(serialized.get("apply_patch").is_none()); assert!(serialized.get("apply_patch_match_metadata").is_none()); + assert!(serialized.get("apply_patch_matching_mode").is_none()); assert!(serialized.get("apply_patch_strict_matching").is_none()); } diff --git a/crates/webcodex-runner-registry/src/tests/capabilities.rs b/crates/webcodex-runner-registry/src/tests/capabilities.rs index dcd8e7d2..0ce0dc74 100644 --- a/crates/webcodex-runner-registry/src/tests/capabilities.rs +++ b/crates/webcodex-runner-registry/src/tests/capabilities.rs @@ -87,6 +87,7 @@ fn capability_classification_keeps_environment_dependent_features_registration_r RunnerFeature::StructuredCargoTestExecutionPolicy, RunnerFeature::ApplyTextEditLineScope, RunnerFeature::ApplyPatchMatchMetadata, + RunnerFeature::ApplyPatchMatchingMode, RunnerFeature::ApplyPatchStrictMatching, RunnerFeature::SshShell, RunnerFeature::PersistentShell, @@ -151,6 +152,19 @@ async fn patch_contract_capabilities_require_their_prerequisites() { error, "apply_patch_strict_matching capability requires apply_patch_match_metadata capability" ); + + let mut registration = + runner_registration("matching-mode-without-metadata", "inst-c", Vec::new()); + registration.capabilities = with_wire_feature( + &with_wire_feature(&v2_baseline_capabilities(), RunnerFeature::ApplyPatch, true), + RunnerFeature::ApplyPatchMatchingMode, + true, + ); + let error = registry.register(registration).await.unwrap_err(); + assert_eq!( + error, + "apply_patch_matching_mode capability requires apply_patch_match_metadata capability" + ); } #[test] diff --git a/crates/webcodex-runner-registry/src/tests/protocol.rs b/crates/webcodex-runner-registry/src/tests/protocol.rs index 618f8174..56f0c037 100644 --- a/crates/webcodex-runner-registry/src/tests/protocol.rs +++ b/crates/webcodex-runner-registry/src/tests/protocol.rs @@ -454,6 +454,7 @@ async fn runner_supports_recognizes_all_protocol_capability_names() { apply_text_edit_line_scope: true, apply_patch: true, apply_patch_match_metadata: true, + apply_patch_matching_mode: true, apply_patch_strict_matching: true, git: true, jobs: true, diff --git a/crates/webcodex-runner/src/main.rs b/crates/webcodex-runner/src/main.rs index 7f2dd124..5913ab8b 100644 --- a/crates/webcodex-runner/src/main.rs +++ b/crates/webcodex-runner/src/main.rs @@ -1903,8 +1903,10 @@ fn runner_register_capabilities(cfg: &RunnerConfig) -> RunnerCapabilities { // patch-plan/match metadata consumed by Server validation. Older apply_patch // implementations omit this capability and are rejected before dispatch. capabilities.apply_patch_match_metadata = true; - // Strict patch positioning is an additive extension to apply_patch. Older - // Runners omit it, so Servers must not send strict_matching to them. + // Enum-based matching is the 0.4 model-facing authority. Older Runners omit + // it, so current Servers fail closed instead of falling back to old defaults. + capabilities.apply_patch_matching_mode = true; + // Retain the legacy bit only so an older Server can roll against this Runner. capabilities.apply_patch_strict_matching = true; capabilities.async_jobs = true; capabilities.async_shell_jobs = true; diff --git a/crates/webcodex-runner/src/main_tests/apply_patch.rs b/crates/webcodex-runner/src/main_tests/apply_patch.rs index ea8dd570..c88771ec 100644 --- a/crates/webcodex-runner/src/main_tests/apply_patch.rs +++ b/crates/webcodex-runner/src/main_tests/apply_patch.rs @@ -1,22 +1,20 @@ use super::*; fn apply_patch_request(cwd: &Path, patch: &str, dry_run: bool) -> RunnerRequest { - apply_patch_request_with_strict(cwd, patch, dry_run, false) + apply_patch_request_with_mode(cwd, patch, dry_run, "unique") } -fn apply_patch_request_with_strict( +fn apply_patch_request_with_mode( cwd: &Path, patch: &str, dry_run: bool, - strict_matching: bool, + matching_mode: &str, ) -> RunnerRequest { - let mut payload = serde_json::json!({ + let payload = serde_json::json!({ "patch": patch, "dry_run": dry_run, + "matching_mode": matching_mode, }); - if strict_matching { - payload["strict_matching"] = serde_json::json!(true); - } RunnerRequest { request_id: "req-apply-patch".to_string(), client_id: "agent-1".to_string(), @@ -48,8 +46,19 @@ fn apply_patch_request_with_strict( } } +fn apply_patch_request_legacy_strict(cwd: &Path, patch: &str, dry_run: bool) -> RunnerRequest { + let payload = serde_json::json!({ + "patch": patch, + "dry_run": dry_run, + "strict_matching": true, + }); + let mut request = apply_patch_request_with_mode(cwd, patch, dry_run, "unique"); + request.content = Some(payload.to_string()); + request +} + #[test] -fn file_apply_patch_strict_matching_accepts_exact_unique_and_append() { +fn file_apply_patch_exact_unique_accepts_exact_unique_and_append() { let tmp = tempfile::tempdir().unwrap(); let policy = project_policy(tmp.path()); std::fs::write(tmp.path().join("exact.txt"), "old\n").unwrap(); @@ -58,11 +67,13 @@ fn file_apply_patch_strict_matching_accepts_exact_unique_and_append() { let out = line_edit_json(handle_file_request( &policy, - &apply_patch_request_with_strict(tmp.path(), patch, false, true), + &apply_patch_request_with_mode(tmp.path(), patch, false, "exact_unique"), )); assert_eq!(out["changed"], true); assert_eq!(out["execution_state"], "completed"); + assert_eq!(out["requested_matching_mode"], "exact_unique"); + assert_eq!(out["files"][0]["edits"][0]["unique_match"], true); assert_eq!(out["files"][0]["edits"][0]["strict_match"], true); assert_eq!(out["files"][1]["edits"][0]["strict_match"], true); assert_eq!( @@ -76,7 +87,7 @@ fn file_apply_patch_strict_matching_accepts_exact_unique_and_append() { } #[test] -fn file_apply_patch_strict_matching_rejects_fuzzy_and_ambiguous_before_write() { +fn file_apply_patch_exact_unique_rejects_fuzzy_and_ambiguous_before_write() { for (original, patch, expected_mode, expected_candidates) in [ ( " old \n", @@ -96,25 +107,22 @@ fn file_apply_patch_strict_matching_rejects_fuzzy_and_ambiguous_before_write() { std::fs::write(tmp.path().join("target.txt"), original).unwrap(); let out = line_edit_json(handle_file_request( &policy, - &apply_patch_request_with_strict(tmp.path(), patch, false, true), + &apply_patch_request_with_mode(tmp.path(), patch, false, "exact_unique"), )); - assert_eq!(out["error_kind"], "strict_match_rejected"); + assert_eq!(out["error_kind"], "matching_mode_rejected"); assert_eq!(out["state_changed"], false); assert_eq!(out["execution_state"], "not_started"); + assert_eq!(out["requested_matching_mode"], "exact_unique"); assert_eq!(out["match_mode"], expected_mode); assert_eq!(out["candidate_count"], expected_candidates); assert_eq!(out["search_start_line"], 1); assert!(out["source_line_count"].as_u64().unwrap() >= expected_candidates); - assert_eq!(out["strict_match"], false); - assert_eq!(out["recovery_action"], "refine_strict_patch"); + assert_eq!(out["matching_mode_satisfied"], false); + assert_eq!(out["recovery_action"], "refine_patch_context"); assert!(out["retry_guidance"] .as_str() .unwrap() - .contains("strict_matching=true")); - assert!(!out["retry_guidance"] - .as_str() - .unwrap() - .contains("strict_matching=false")); + .contains("matching_mode=exact_unique")); assert_eq!( std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), original @@ -123,7 +131,7 @@ fn file_apply_patch_strict_matching_rejects_fuzzy_and_ambiguous_before_write() { } #[test] -fn file_apply_patch_strict_matching_reports_the_ambiguous_component() { +fn file_apply_patch_unique_accepts_repeated_context_with_one_mutation_target() { let tmp = tempfile::tempdir().unwrap(); let policy = project_policy(tmp.path()); let original = "ctx\n foo \nctx\nother\n"; @@ -132,17 +140,44 @@ fn file_apply_patch_strict_matching_reports_the_ambiguous_component() { let out = line_edit_json(handle_file_request( &policy, - &apply_patch_request_with_strict(tmp.path(), patch, false, true), + &apply_patch_request_with_mode(tmp.path(), patch, false, "unique"), + )); + + assert_eq!(out["changed"], true); + assert_eq!(out["execution_state"], "completed"); + assert_eq!(out["requested_matching_mode"], "unique"); + let edit = &out["files"][0]["edits"][0]; + assert_eq!(edit["match_source"], "old_lines"); + assert_eq!(edit["match_mode"], "trim"); + assert_eq!(edit["matched_start_line"], 2); + assert_eq!(edit["candidate_count"], 1); + assert_eq!(edit["unique_match"], true); + assert_eq!(edit["strict_match"], false); + assert_eq!( + std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), + "ctx\nnew\nctx\nother\n" + ); +} + +#[test] +fn file_apply_patch_unique_rejects_repeated_context_for_pure_addition() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + let original = "ctx\nfirst\nctx\nsecond\n"; + std::fs::write(tmp.path().join("target.txt"), original).unwrap(); + let patch = "*** Begin Patch\n*** Update File: target.txt\n@@ ctx\n+inserted\n*** End Patch"; + + let out = line_edit_json(handle_file_request( + &policy, + &apply_patch_request_with_mode(tmp.path(), patch, false, "unique"), )); - assert_eq!(out["error_kind"], "strict_match_rejected"); + assert_eq!(out["error_kind"], "matching_mode_rejected"); + assert_eq!(out["requested_matching_mode"], "unique"); assert_eq!(out["match_source"], "change_context"); - assert_eq!(out["match_mode"], "exact"); assert_eq!(out["candidate_count"], 2); - assert_eq!(out["matched_start_line"], 1); - assert_eq!(out["search_start_line"], 1); - assert_eq!(out["source_line_count"], 4); - assert_eq!(out["strict_match"], false); + assert_eq!(out["candidate_start_lines"], serde_json::json!([1, 3])); + assert!(out["matched_start_line"].is_null()); assert_eq!( std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), original @@ -150,7 +185,7 @@ fn file_apply_patch_strict_matching_reports_the_ambiguous_component() { } #[test] -fn file_apply_patch_strict_matching_rejects_later_risk_before_any_batch_write() { +fn file_apply_patch_exact_unique_rejects_later_risk_before_any_batch_write() { let tmp = tempfile::tempdir().unwrap(); let policy = project_policy(tmp.path()); std::fs::write(tmp.path().join("safe.txt"), "old\n").unwrap(); @@ -159,10 +194,10 @@ fn file_apply_patch_strict_matching_rejects_later_risk_before_any_batch_write() let out = line_edit_json(handle_file_request( &policy, - &apply_patch_request_with_strict(tmp.path(), patch, false, true), + &apply_patch_request_with_mode(tmp.path(), patch, false, "exact_unique"), )); - assert_eq!(out["error_kind"], "strict_match_rejected"); + assert_eq!(out["error_kind"], "matching_mode_rejected"); assert_eq!(out["change_index"], 1); assert_eq!(out["path"], "risky.txt"); assert_eq!(out["state_changed"], false); @@ -224,6 +259,7 @@ fn file_apply_patch_dry_run_reports_plan_without_writing() { )); assert_eq!(out["dry_run"], true); + assert_eq!(out["requested_matching_mode"], "unique"); assert_eq!(out["changed"], false); assert_eq!(out["state_changed"], false); assert_eq!(out["would_change"], true); @@ -232,6 +268,7 @@ fn file_apply_patch_dry_run_reports_plan_without_writing() { assert_eq!(edit["match_source"], "old_lines"); assert_eq!(edit["matched_start_line"], 1); assert_eq!(edit["candidate_count"], 1); + assert_eq!(edit["unique_match"], true); assert_eq!(edit["strict_match"], true); assert_eq!( std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), @@ -258,6 +295,7 @@ fn file_apply_patch_reports_fuzzy_and_append_match_metadata() { assert_eq!(trim_end["match_source"], "old_lines"); assert_eq!(trim_end["matched_start_line"], 1); assert_eq!(trim_end["candidate_count"], 1); + assert_eq!(trim_end["unique_match"], true); assert_eq!(trim_end["strict_match"], false); let trim = &out["files"][1]["edits"][0]; @@ -265,6 +303,7 @@ fn file_apply_patch_reports_fuzzy_and_append_match_metadata() { assert_eq!(trim["match_source"], "old_lines"); assert_eq!(trim["matched_start_line"], 1); assert_eq!(trim["candidate_count"], 1); + assert_eq!(trim["unique_match"], true); assert_eq!(trim["strict_match"], false); let append = &out["files"][2]["edits"][0]; @@ -272,11 +311,12 @@ fn file_apply_patch_reports_fuzzy_and_append_match_metadata() { assert_eq!(append["match_source"], "append"); assert_eq!(append["matched_start_line"], 2); assert!(append["candidate_count"].is_null()); + assert_eq!(append["unique_match"], true); assert_eq!(append["strict_match"], true); } #[test] -fn file_apply_patch_reports_ambiguous_candidate_count_without_changing_selection() { +fn file_apply_patch_unique_rejects_ambiguous_candidate_without_writing() { let tmp = tempfile::tempdir().unwrap(); let policy = project_policy(tmp.path()); std::fs::write(tmp.path().join("target.txt"), "dup\nmiddle\ndup\n").unwrap(); @@ -287,12 +327,14 @@ fn file_apply_patch_reports_ambiguous_candidate_count_without_changing_selection &apply_patch_request(tmp.path(), patch, true), )); - let edit = &out["files"][0]["edits"][0]; - assert_eq!(edit["match_mode"], "exact"); - assert_eq!(edit["match_source"], "old_lines"); - assert_eq!(edit["matched_start_line"], 1); - assert_eq!(edit["candidate_count"], 2); - assert_eq!(edit["strict_match"], false); + assert_eq!(out["error_kind"], "matching_mode_rejected"); + assert_eq!(out["requested_matching_mode"], "unique"); + assert_eq!(out["match_mode"], "exact"); + assert_eq!(out["match_source"], "old_lines"); + assert!(out["matched_start_line"].is_null()); + assert_eq!(out["candidate_count"], 2); + assert_eq!(out["candidate_start_lines"], serde_json::json!([1, 3])); + assert_eq!(out["state_changed"], false); assert_eq!( std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), "dup\nmiddle\ndup\n" @@ -361,6 +403,114 @@ fn file_apply_patch_context_conflict_does_not_echo_patch_or_source_body() { ); } +#[test] +fn file_apply_patch_unique_accepts_normalized_unicode_and_reports_the_tier() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + std::fs::write(tmp.path().join("target.txt"), "alpha—beta\n").unwrap(); + let patch = + "*** Begin Patch\n*** Update File: target.txt\n-alpha-beta\n+changed\n*** End Patch"; + + let out = line_edit_json(handle_file_request( + &policy, + &apply_patch_request_with_mode(tmp.path(), patch, false, "unique"), + )); + assert_eq!(out["changed"], true); + assert_eq!(out["requested_matching_mode"], "unique"); + assert_eq!(out["files"][0]["edits"][0]["match_mode"], "normalized"); + assert_eq!(out["files"][0]["edits"][0]["candidate_count"], 1); + assert_eq!(out["files"][0]["edits"][0]["unique_match"], true); + assert_eq!( + std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), + "changed\n" + ); +} + +#[test] +fn file_apply_patch_unique_eof_constraint_ignores_earlier_duplicate() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + std::fs::write(tmp.path().join("target.txt"), "same\nmid\nsame\n").unwrap(); + let patch = "*** Begin Patch\n*** Update File: target.txt\n-same\n+last\n*** End of File\n*** End Patch"; + + let out = line_edit_json(handle_file_request( + &policy, + &apply_patch_request_with_mode(tmp.path(), patch, false, "unique"), + )); + assert_eq!(out["changed"], true); + assert_eq!(out["files"][0]["edits"][0]["matched_start_line"], 3); + assert_eq!(out["files"][0]["edits"][0]["candidate_count"], 1); + assert_eq!(out["files"][0]["edits"][0]["unique_match"], true); + assert_eq!( + std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), + "same\nmid\nlast\n" + ); +} + +#[test] +fn file_apply_patch_first_match_is_deterministic_for_repeated_candidates() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + std::fs::write(tmp.path().join("target.txt"), "dup\nmid\ndup\n").unwrap(); + let patch = "*** Begin Patch\n*** Update File: target.txt\n-dup\n+first\n*** End Patch"; + + let out = line_edit_json(handle_file_request( + &policy, + &apply_patch_request_with_mode(tmp.path(), patch, false, "first_match"), + )); + assert_eq!(out["changed"], true); + assert_eq!(out["requested_matching_mode"], "first_match"); + assert_eq!(out["files"][0]["edits"][0]["candidate_count"], 2); + assert_eq!(out["files"][0]["edits"][0]["unique_match"], false); + assert_eq!( + std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), + "first\nmid\ndup\n" + ); +} + +#[test] +fn file_apply_patch_rejects_contradictory_enum_and_legacy_bool() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + std::fs::write(tmp.path().join("target.txt"), "old\n").unwrap(); + let patch = "*** Begin Patch\n*** Update File: target.txt\n-old\n+new\n*** End Patch"; + let mut request = apply_patch_request_with_mode(tmp.path(), patch, false, "unique"); + request.content = Some( + serde_json::json!({ + "patch": patch, + "dry_run": false, + "matching_mode": "unique", + "strict_matching": true, + }) + .to_string(), + ); + + let out = line_edit_json(handle_file_request(&policy, &request)); + assert_eq!(out["error_kind"], "invalid_payload"); + assert_eq!(out["state_changed"], false); + assert_eq!( + std::fs::read_to_string(tmp.path().join("target.txt")).unwrap(), + "old\n" + ); +} + +#[test] +fn file_apply_patch_legacy_strict_true_maps_to_exact_unique() { + let tmp = tempfile::tempdir().unwrap(); + let policy = project_policy(tmp.path()); + std::fs::write(tmp.path().join("target.txt"), " old \n").unwrap(); + let patch = "*** Begin Patch\n*** Update File: target.txt\n-old\n+new\n*** End Patch"; + let out = line_edit_json(handle_file_request( + &policy, + &apply_patch_request_legacy_strict(tmp.path(), patch, false), + )); + assert_eq!(out["error_kind"], "matching_mode_rejected"); + assert_eq!(out["requested_matching_mode"], "exact_unique"); + assert_eq!(out["match_mode"], "trim"); + assert_eq!(out["candidate_count"], 1); + assert_eq!(out["state_changed"], false); +} + #[test] fn file_apply_patch_rejects_sensitive_paths_before_write() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/webcodex-runner/src/main_tests/registration.rs b/crates/webcodex-runner/src/main_tests/registration.rs index 0b012e74..a5c2ec76 100644 --- a/crates/webcodex-runner/src/main_tests/registration.rs +++ b/crates/webcodex-runner/src/main_tests/registration.rs @@ -65,6 +65,11 @@ fn current_runner_registration_advertises_v2_and_complete_generation_baseline() .contains(&"apply_patch_match_metadata"), "the 0.4 patch success contract is additive and must not become a generation-2 baseline" ); + assert!( + !RUNNER_PROTOCOL_GENERATION_V2_BASELINE_CAPABILITY_NAMES + .contains(&"apply_patch_matching_mode"), + "matching_mode is additive and must not become a generation-2 registration baseline" + ); assert!( !RUNNER_PROTOCOL_GENERATION_V2_BASELINE_CAPABILITY_NAMES .contains(&"apply_patch_strict_matching"), @@ -77,6 +82,13 @@ fn current_runner_registration_advertises_v2_and_complete_generation_baseline() Some(true), "current Runner must explicitly advertise the current apply_patch success contract" ); + assert_eq!( + capabilities + .get("apply_patch_matching_mode") + .and_then(serde_json::Value::as_bool), + Some(true), + "current Runner must explicitly advertise enum-based apply_patch matching" + ); assert_eq!( capabilities .get("apply_patch_strict_matching") diff --git a/crates/webcodex-runner/src/webcodex_runner/patches.rs b/crates/webcodex-runner/src/webcodex_runner/patches.rs index 3ae2d6df..18952a1d 100644 --- a/crates/webcodex-runner/src/webcodex_runner/patches.rs +++ b/crates/webcodex-runner/src/webcodex_runner/patches.rs @@ -332,8 +332,8 @@ use crate::apply_edits_shared::{ MAX_APPLY_TEXT_EDIT_FIELD_BYTES as APPLY_TEXT_EDITS_MAX_FIELD_BYTES, }; use crate::apply_patch_shared::{ - derive_codex_patch_update_with_matches, parse_codex_patch, CodexPatchChunkMatch, - CodexPatchError, CodexPatchHunk, CodexPatchMatchDiagnostic, + derive_codex_patch_update_with_matching_mode, parse_codex_patch, ApplyPatchMatchingMode, + CodexPatchChunkMatch, CodexPatchError, CodexPatchHunk, CodexPatchMatchDiagnostic, }; #[derive(Debug, Deserialize)] @@ -352,9 +352,28 @@ struct ApplyPatchPayload { #[serde(default)] dry_run: Option, #[serde(default)] + matching_mode: Option, + /// Rolling-wire compatibility for older Servers only. Current model-facing + /// requests use matching_mode and never emit this field. + #[serde(default)] strict_matching: Option, } +fn apply_patch_matching_mode( + payload: &ApplyPatchPayload, +) -> Result { + if payload.matching_mode.is_some() && payload.strict_matching.is_some() { + return Err("matching_mode and legacy strict_matching cannot be combined".to_string()); + } + Ok(match (payload.matching_mode, payload.strict_matching) { + (Some(mode), None) => mode, + (None, Some(true)) => ApplyPatchMatchingMode::ExactUnique, + // Preserve the old Server wire default when rolling a new Runner first. + (None, Some(false) | None) => ApplyPatchMatchingMode::FirstMatch, + (Some(_), Some(_)) => unreachable!(), + }) +} + #[derive(Debug)] enum EditPlanConflict { Match(ApplyTextMatchConflict), @@ -1054,6 +1073,7 @@ fn apply_change(plan: &PlannedFileChange) -> Result, ApplyChangeFai fn execute_planned_file_changes( plans: Vec, dry_run: bool, + requested_matching_mode: Option, start: Instant, ) -> CommandResult { let mut changed_paths = Vec::new(); @@ -1128,19 +1148,20 @@ fn execute_planned_file_changes( }) }) .collect::>(); - line_edit_stdout( - serde_json::json!({ - "dry_run": dry_run, - "applied_count": plans.len(), - "changed": !dry_run && would_change, - "state_changed": !dry_run && would_change, - "execution_state": "completed", - "would_change": would_change, - "files": files, - "changed_paths": changed_paths, - }), - start, - ) + let mut output = serde_json::json!({ + "dry_run": dry_run, + "applied_count": plans.len(), + "changed": !dry_run && would_change, + "state_changed": !dry_run && would_change, + "execution_state": "completed", + "would_change": would_change, + "files": files, + "changed_paths": changed_paths, + }); + if let Some(mode) = requested_matching_mode { + output["requested_matching_mode"] = serde_json::json!(mode.as_str()); + } + line_edit_stdout(output, start) } fn resolve_unique_patch_path( @@ -1234,34 +1255,59 @@ fn apply_patch_match_diagnostic_json(diagnostic: &CodexPatchMatchDiagnostic) -> }) } -fn apply_patch_strict_match_rejection( +fn apply_patch_matching_mode_rejection( index: usize, path: &str, matched: &CodexPatchChunkMatch, start: Instant, ) -> CommandResult { - let rejection = matched.strict_rejection; + let Some(rejection) = matched.match_rejection.as_ref() else { + return batch_error( + Some(index), + Some("edit"), + Some(path), + "invalid_match_metadata", + "matching-mode rejection was missing its bounded rejection fact", + start, + ); + }; + let ambiguous = rejection.candidate_count > 1; + let retry_guidance = match (rejection.requested_matching_mode, ambiguous) { + (ApplyPatchMatchingMode::Unique, true) => { + "add a stable parent/function/test/module anchor or small surrounding context and retry with matching_mode=unique; candidate positions are equal observation targets, never a winner" + } + (ApplyPatchMatchingMode::ExactUnique, true) => { + "expand exact context until every textual positioning decision is exact and unique, then retry with matching_mode=exact_unique" + } + (ApplyPatchMatchingMode::ExactUnique, false) => { + "reread the current source and regenerate exact context, then retry with matching_mode=exact_unique" + } + _ => "regenerate the patch with unambiguous context and retry", + }; line_edit_stdout( serde_json::json!({ "changed": false, "state_changed": false, "execution_state": "not_started", - "error_kind": "strict_match_rejected", + "error_kind": "matching_mode_rejected", "change_index": index, "path": path, "chunk_index": matched.chunk_index, - "match_mode": rejection.map(|fact| fact.match_mode.as_str()), - "match_source": rejection.map(|fact| fact.match_source.as_str()), - "matched_start_line": rejection.map(|fact| fact.matched_start_line), - "candidate_count": rejection.map(|fact| fact.candidate_count), - "search_start_line": rejection.map(|fact| fact.search_start_line), - "source_line_count": rejection.map(|fact| fact.source_line_count), - "strict_match": false, - "recovery_action": "refine_strict_patch", - "retry_guidance": "add exact unique context and retry with strict_matching=true; do not relax strict matching to recover from this rejection", + "requested_matching_mode": rejection.requested_matching_mode.as_str(), + "match_mode": rejection.match_mode.as_str(), + "match_source": rejection.match_source.as_str(), + "matched_start_line": (!ambiguous).then_some(rejection.matched_start_line), + "candidate_count": rejection.candidate_count, + "candidate_start_lines": rejection.candidate_start_lines, + "candidate_positions_truncated": rejection.candidate_positions_truncated, + "search_start_line": rejection.search_start_line, + "source_line_count": rejection.source_line_count, + "matching_mode_satisfied": false, + "recovery_action": "refine_patch_context", + "retry_guidance": retry_guidance, "error": format!( - "Rejected strict Codex patch before write: {path} chunk {} was not positioned by exact unique matching. No files were modified.", - matched.chunk_index + "Rejected Codex patch before write: {path} chunk {} did not satisfy matching_mode={}. No files were modified.", + matched.chunk_index, rejection.requested_matching_mode.as_str() ), }), start, @@ -1306,7 +1352,10 @@ pub(crate) fn handle_apply_patch_file_request( } }; let dry_run = payload.dry_run.unwrap_or(false); - let strict_matching = payload.strict_matching.unwrap_or(false); + let matching_mode = match apply_patch_matching_mode(&payload) { + Ok(mode) => mode, + Err(error) => return batch_error(None, None, None, "invalid_payload", error, start), + }; let mut touched = HashSet::new(); let mut plans = Vec::with_capacity(patch.hunks.len()); @@ -1413,18 +1462,21 @@ pub(crate) fn handle_apply_patch_file_request( let (replacement, chunk_matches) = if chunks.is_empty() { (original.clone(), Vec::new()) } else { - match derive_codex_patch_update_with_matches(&original, path, chunks) { + match derive_codex_patch_update_with_matching_mode( + &original, + path, + chunks, + matching_mode, + ) { Ok(update) => { - if strict_matching { - if let Some(matched) = update - .chunk_matches - .iter() - .find(|matched| !matched.strict_match) - { - return apply_patch_strict_match_rejection( - index, path, matched, start, - ); - } + if let Some(matched) = update + .chunk_matches + .iter() + .find(|matched| matched.match_rejection.is_some()) + { + return apply_patch_matching_mode_rejection( + index, path, matched, start, + ); } (update.content, update.chunk_matches) } @@ -1457,6 +1509,7 @@ pub(crate) fn handle_apply_patch_file_request( "match_source": chunk_match.match_source.as_str(), "matched_start_line": chunk_match.matched_start_line, "candidate_count": chunk_match.candidate_count, + "unique_match": chunk_match.unique_match, "strict_match": chunk_match.strict_match, }) }) @@ -1527,7 +1580,7 @@ pub(crate) fn handle_apply_patch_file_request( plans.push(planned); } - execute_planned_file_changes(plans, dry_run, start) + execute_planned_file_changes(plans, dry_run, Some(matching_mode), start) } pub(crate) fn handle_apply_text_edits_file_request( @@ -1918,7 +1971,7 @@ pub(crate) fn handle_apply_text_edits_file_request( plans.push(planned); } - execute_planned_file_changes(plans, dry_run, start) + execute_planned_file_changes(plans, dry_run, None, start) } #[cfg(test)] diff --git a/crates/webcodex-tool-contracts/src/registry/input_schemas/patches.rs b/crates/webcodex-tool-contracts/src/registry/input_schemas/patches.rs index aba3bfa3..95677dac 100644 --- a/crates/webcodex-tool-contracts/src/registry/input_schemas/patches.rs +++ b/crates/webcodex-tool-contracts/src/registry/input_schemas/patches.rs @@ -18,9 +18,9 @@ pub fn apply_patch_input_schema() -> Value { false, ), ( - "strict_matching", - "boolean", - "If true, require every text positioning match to be exact and unique before any write; unanchored append remains allowed. Requires Runner apply_patch_strict_matching capability. Defaults false.", + "matching_mode", + "string", + "Positioning policy. unique (default) tries Exact, TrimEnd, Trim, then Normalized and requires exactly one final mutation target at the selected tier; a repeated @@ anchor is allowed when old_lines still resolves to one target, while anchored pure additions require a unique anchor. exact_unique additionally requires Exact and unique at every textual positioning decision and is intended for an explicit stale-context/concurrency fence after reading exact current source. first_match is only for explicitly requested permissive compatibility and deterministically selects the first eligible candidate in the highest-priority tier.", false, ), ])); @@ -28,7 +28,9 @@ pub fn apply_patch_input_schema() -> Value { schema["properties"]["patch"]["maxLength"] = json!(webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_BYTES); schema["properties"]["dry_run"]["default"] = json!(false); - schema["properties"]["strict_matching"]["default"] = json!(false); + schema["properties"]["matching_mode"]["enum"] = + json!(["first_match", "unique", "exact_unique"]); + schema["properties"]["matching_mode"]["default"] = json!("unique"); schema } diff --git a/crates/webcodex-tool-contracts/src/registry/output_schemas/edits.rs b/crates/webcodex-tool-contracts/src/registry/output_schemas/edits.rs index 5cc3f351..280ffeb4 100644 --- a/crates/webcodex-tool-contracts/src/registry/output_schemas/edits.rs +++ b/crates/webcodex-tool-contracts/src/registry/output_schemas/edits.rs @@ -16,7 +16,7 @@ fn apply_patch_edit_summary_schema() -> Value { "match_mode": { "description": "Validated current apply_patch positioning mode; null only for unanchored append.", "anyOf": [ - {"type": "string", "enum": ["exact", "trim_end", "trim"]}, + {"type": "string", "enum": ["exact", "trim_end", "trim", "normalized"]}, {"type": "null"} ] }, @@ -37,15 +37,19 @@ fn apply_patch_edit_summary_schema() -> Value { {"type": "null"} ] }, + "unique_match": { + "type": "boolean", + "description": "Validated fact that the final mutation target was unique at its selected tier; for anchored pure additions this means the change_context itself was unique." + }, "strict_match": { "type": "boolean", - "description": "Validated exact-and-unique positioning fact." + "description": "Validated exact-and-unique positioning fact retained as match metadata; matching_mode is the request authority." } }, "required": [ "chunk_index", "change_context_present", "old_line_count", "new_line_count", "end_of_file", "match_mode", "match_source", "matched_start_line", - "candidate_count", "strict_match" + "candidate_count", "unique_match", "strict_match" ] }) } @@ -86,15 +90,16 @@ fn apply_patch_match_diagnostic_schema() -> Value { }) } -fn apply_patch_strict_match_diagnostic_schema() -> Value { +fn apply_patch_match_rejection_diagnostic_schema() -> Value { json!({ "type": "object", "additionalProperties": false, - "description": "Server-validated, body-free classification of a deterministic strict-match rejection. Runner target metadata is checked against the original parsed patch before projection. Ambiguous matches intentionally omit the selected Runner location by returning matched_start_line=null.", + "description": "Server-validated, body-free classification of a deterministic matching_mode rejection. Candidate positions are equal structural observation targets, never a winner/preference signal; ambiguous matches require matched_start_line=null.", "properties": { "classification": {"type": "string", "enum": ["unique_fuzzy_candidate", "ambiguous_candidate"]}, + "requested_matching_mode": {"type": "string", "enum": ["unique", "exact_unique"]}, "chunk_index": {"type": "integer", "minimum": 0}, - "match_mode": {"type": "string", "enum": ["exact", "trim_end", "trim"]}, + "match_mode": {"type": "string", "enum": ["exact", "trim_end", "trim", "normalized"]}, "match_source": {"type": "string", "enum": ["old_lines", "change_context"]}, "matched_start_line": { "description": "Validated 1-based candidate location only for a unique fuzzy candidate; null for ambiguous candidates so no first match is presented as authoritative.", @@ -104,12 +109,21 @@ fn apply_patch_strict_match_diagnostic_schema() -> Value { ] }, "candidate_count": {"type": "integer", "minimum": 1}, + "candidate_start_lines": { + "type": "array", + "minItems": 1, + "maxItems": webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS, + "items": {"type": "integer", "minimum": 1}, + "description": "Ascending bounded candidate starts. Ordering is positional only and never preference." + }, + "candidate_positions_truncated": {"type": "boolean"}, "expected_line_count": {"type": "integer", "minimum": 1}, - "strict_match": {"type": "boolean", "const": false} + "matching_mode_satisfied": {"type": "boolean", "const": false} }, "required": [ - "classification", "chunk_index", "match_mode", "match_source", - "matched_start_line", "candidate_count", "expected_line_count", "strict_match" + "classification", "requested_matching_mode", "chunk_index", "match_mode", "match_source", + "matched_start_line", "candidate_count", "candidate_start_lines", + "candidate_positions_truncated", "expected_line_count", "matching_mode_satisfied" ] }) } @@ -121,11 +135,11 @@ fn apply_patch_recovery_schema() -> Value { "description": "Server-derived, body-free reread hint for a validated deterministic no-write apply_patch rejection. Copy `items` into the direct `read_files` tool for the same project. It is emitted only when the failed target and structural facts prove a bounded reread is safe; the Runner cannot choose the tool, path, or arguments.", "properties": { "action": {"type": "string", "enum": ["read_files"]}, - "reason": {"type": "string", "enum": ["context_mismatch", "strict_match_rejected_unique_fuzzy"]}, + "reason": {"type": "string", "enum": ["context_mismatch", "matching_mode_rejected_unique_fuzzy", "matching_mode_rejected_ambiguous"]}, "items": { "type": "array", "minItems": 1, - "maxItems": 1, + "maxItems": webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS, "items": { "type": "object", "additionalProperties": false, @@ -336,6 +350,7 @@ pub(super) fn output_schema_for_tool(name: &str) -> Option { ])), "apply_patch" => Some(wrapped_output_schema(vec![ ("dry_run", schema_type("boolean", "Whether this was a dry-run with no file writes.")), + ("requested_matching_mode", json!({"type":"string","enum":["first_match","unique","exact_unique"],"description":"Server-validated positioning mode requested for this apply_patch invocation."})), ("applied_count", schema_type("integer", "Number of parsed file operations in the patch.")), ("changed", schema_type("boolean", "Whether the worktree was confirmed changed by this request.")), ("would_change", schema_type("boolean", "Whether the fully preflighted patch plan would change the worktree.")), @@ -345,7 +360,7 @@ pub(super) fn output_schema_for_tool(name: &str) -> Option { ("execution_state", json!({"type":"string","enum":["not_started","completed","outcome_unknown"],"description":"Transactional patch mutation effect state."})), ("error_kind", nullable_schema("string", "Stable parse, preflight, conflict, capability, transaction, or uncertainty classification.")), ("failure_kind", nullable_schema("string", "not_started, capability_unavailable, or outcome_unknown for delivery/admission failures.")), - ("recovery_action", nullable_schema("string", "Bounded next action such as regenerate_patch, reread_or_regenerate_patch, reread_and_regenerate_strict_patch, add_exact_unique_context, upgrade_or_reconnect_runner, or inspect_workspace_before_retry.")), + ("recovery_action", nullable_schema("string", "Bounded next action such as reread_and_regenerate_patch, read_equal_candidates_and_refine_context, read_equal_candidates_and_add_exact_context, or inspect_workspace_before_retry.")), ("rollback_complete", nullable_schema("boolean", "Whether a failed transactional apply fully restored all earlier changes.")), ("change_index", nullable_schema("integer", "Zero-based failed file-operation index when known.")), ("kind", nullable_schema("string", "Failed patch file-operation kind when known.")), @@ -353,7 +368,7 @@ pub(super) fn output_schema_for_tool(name: &str) -> Option { ("patch_line", nullable_schema("integer", "One-based patch line for a syntax error when known.")), ("expected_format", nullable_schema("string", "codex_patch for parse-format recovery; null otherwise.")), ("match_diagnostic", apply_patch_match_diagnostic_schema()), - ("strict_match_diagnostic", apply_patch_strict_match_diagnostic_schema()), + ("match_rejection_diagnostic", apply_patch_match_rejection_diagnostic_schema()), ("recovery", apply_patch_recovery_schema()), ("retry_guidance", schema_type("string", "Bounded recovery guidance for deterministic no-mutation rejection.")), ])), diff --git a/crates/webcodex-tool-contracts/src/tests/registry_specs.rs b/crates/webcodex-tool-contracts/src/tests/registry_specs.rs index 46c01164..6b6925a1 100644 --- a/crates/webcodex-tool-contracts/src/tests/registry_specs.rs +++ b/crates/webcodex-tool-contracts/src/tests/registry_specs.rs @@ -97,9 +97,11 @@ fn tool_specs_describe_default_coding_loop_preferences() { "sha rechecks", "rollback", "dry_run", - "strict_match", - "strict_matching=true", - "exact-unique positioning", + "matching_mode=unique", + "stable parent/function/test/module", + "matching_mode=exact_unique", + "stale-context", + "matching_mode=first_match", "apply_text_edits", "small exact edits", "external diffs", @@ -356,30 +358,43 @@ fn edit_tool_surface_keeps_canonical_tools_visible_and_schemas_stable() { ); } let codex_patch = &spec_named(&specs, "apply_patch").input_schema["properties"]; - for field in ["project", "patch", "dry_run", "strict_matching"] { + for field in ["project", "patch", "dry_run", "matching_mode"] { assert!( codex_patch.get(field).is_some(), "apply_patch must keep field {field}" ); } + assert_eq!(codex_patch["matching_mode"]["default"], "unique"); + assert_eq!( + codex_patch["matching_mode"]["enum"], + json!(["first_match", "unique", "exact_unique"]) + ); + assert!( + codex_patch.get("strict_matching").is_none(), + "legacy strict_matching must not remain model-facing" + ); let patch_spec = spec_named(&specs, "apply_patch"); let patch_output = &patch_spec.output_schema["properties"]["output"]["properties"]; assert!( patch_output.get("match_diagnostic").is_some(), "apply_patch failures must expose body-free match diagnostics" ); - let strict_diagnostic = patch_output - .get("strict_match_diagnostic") - .expect("apply_patch strict failures must expose validated body-free diagnostics"); - assert_eq!(strict_diagnostic["additionalProperties"], false); + let match_rejection = patch_output + .get("match_rejection_diagnostic") + .expect("apply_patch matching failures must expose validated body-free diagnostics"); + assert_eq!(match_rejection["additionalProperties"], false); assert_eq!( - strict_diagnostic["properties"]["classification"]["enum"], + match_rejection["properties"]["classification"]["enum"], json!(["unique_fuzzy_candidate", "ambiguous_candidate"]) ); assert_eq!( - strict_diagnostic["properties"]["matched_start_line"]["anyOf"][1]["type"], + match_rejection["properties"]["matched_start_line"]["anyOf"][1]["type"], "null" ); + assert_eq!( + match_rejection["properties"]["candidate_start_lines"]["maxItems"], + webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS + ); let recovery = patch_output .get("recovery") .expect("apply_patch must publish bounded reread recovery"); @@ -390,9 +405,16 @@ fn edit_tool_surface_keeps_canonical_tools_visible_and_schemas_stable() { ); assert_eq!( recovery["properties"]["reason"]["enum"], - json!(["context_mismatch", "strict_match_rejected_unique_fuzzy"]) + json!([ + "context_mismatch", + "matching_mode_rejected_unique_fuzzy", + "matching_mode_rejected_ambiguous" + ]) + ); + assert_eq!( + recovery["properties"]["items"]["maxItems"], + webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS ); - assert_eq!(recovery["properties"]["items"]["maxItems"], 1); assert_eq!( recovery["properties"]["items"]["items"]["properties"]["limit"]["maximum"], webcodex_core::apply_patch_shared::MAX_CODEX_PATCH_RECOVERY_READ_LINES @@ -431,6 +453,7 @@ fn edit_tool_surface_keeps_canonical_tools_visible_and_schemas_stable() { "match_source", "matched_start_line", "candidate_count", + "unique_match", "strict_match", ] { assert!( diff --git a/crates/webcodex-tool-contracts/src/tool_definition/patches.rs b/crates/webcodex-tool-contracts/src/tool_definition/patches.rs index 69598ced..2fa0f8ad 100644 --- a/crates/webcodex-tool-contracts/src/tool_definition/patches.rs +++ b/crates/webcodex-tool-contracts/src/tool_definition/patches.rs @@ -31,7 +31,7 @@ pub(super) const DEFINITIONS: &[ToolDefinition] = &[ true, false, ), - "Primary model edit path for contextual/multi-file Codex patches. Transactional with SHA rechecks, rollback, dry_run, recovery. Put multiple edits to the same file as multiple chunks inside one `*** Update File` operation; duplicate file operations for the same path are rejected. A deterministic zero-write context mismatch or validated unique-fuzzy strict rejection may include body-free diagnostics plus Server-derived recovery; when recovery.action=read_files, pass recovery.items to read_files for the same project, then regenerate the whole patch. Ambiguous strict rejection never selects the Runner's first candidate: expand exact unique context instead. outcome_unknown requires workspace inspection first. Set strict_matching=true for exact-unique positioning and never relax it as a recovery shortcut. Use apply_text_edits for small exact edits; unified diff for external diffs.", + "Primary model edit path for contextual/multi-file Codex patches. Transactional with SHA rechecks, rollback, dry_run. matching_mode=unique (default) allows bounded whitespace/Unicode drift but writes only when final target is unique; repeated `@@` is okay when old lines still identify one target, while pure additions need a unique anchor. For repetitive targets, add a stable parent/function/test/module anchor. Use matching_mode=exact_unique only after reading exact current source for a stale-context fence; never relax it after rejection. matching_mode=first_match is permissive compatibility only. Put multiple chunks for one file in one Update File; duplicate file operations are rejected. Ambiguity may return equal read_files windows. outcome_unknown requires workspace inspection before retry. Prefer apply_text_edits for small exact edits and unified diff for external diffs.", apply_patch_input_schema, ), PERMISSION_RISK_PATCH, diff --git a/crates/webcodex-tool-runtime-contracts/src/tool_audit.rs b/crates/webcodex-tool-runtime-contracts/src/tool_audit.rs index 07038d7a..1fa7b92a 100644 --- a/crates/webcodex-tool-runtime-contracts/src/tool_audit.rs +++ b/crates/webcodex-tool-runtime-contracts/src/tool_audit.rs @@ -1188,7 +1188,7 @@ pub fn session_log_arguments_for_tool_request(tool_name: &str, arguments: &Value "patch_present".to_string(), Value::Bool(obj.contains_key("patch")), ); - copy_keys(obj, &mut out, &["dry_run", "strict_matching"]); + copy_keys(obj, &mut out, &["dry_run", "matching_mode"]); } "apply_unified_diff" => { out.insert( @@ -5138,14 +5138,14 @@ impl ToolCall { project, patch, dry_run, - strict_matching, + matching_mode, .. } => serde_json::json!({ "project": project, "patch_present": !patch.is_empty(), "patch_bytes": patch.len(), "dry_run": dry_run, - "strict_matching": strict_matching, + "matching_mode": matching_mode.map(|mode| mode.as_str()), }), Self::ApplyTextEdits { project, diff --git a/crates/webcodex-tool-runtime-contracts/src/tool_call.rs b/crates/webcodex-tool-runtime-contracts/src/tool_call.rs index 13268b9a..ae993aaf 100644 --- a/crates/webcodex-tool-runtime-contracts/src/tool_call.rs +++ b/crates/webcodex-tool-runtime-contracts/src/tool_call.rs @@ -11,6 +11,7 @@ use super::tool_inputs::{ use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::{BTreeMap, HashSet}; +use webcodex_core::apply_patch_shared::ApplyPatchMatchingMode; use webcodex_core::job_observation::MAX_JOB_OBSERVATION_TOKEN_LEN; use webcodex_core::lsp_bridge::{ CallHierarchyDirection, DEFAULT_CALL_HIERARCHY_DEPTH, DEFAULT_CALL_HIERARCHY_LIMIT, @@ -699,7 +700,7 @@ pub enum ToolCall { #[serde(default)] dry_run: Option, #[serde(default)] - strict_matching: Option, + matching_mode: Option, #[serde(default)] session_id: Option, }, @@ -2401,6 +2402,16 @@ impl ToolCall { })?; validate_model_facing_assertion_name(name, &arguments)?; validate_model_facing_result_expectation(name, &arguments)?; + if name == "apply_patch" + && arguments + .as_object() + .is_some_and(|object| object.contains_key("strict_matching")) + { + return Err( + "invalid arguments for tool 'apply_patch': field 'strict_matching' is no longer supported; use matching_mode='exact_unique' for former strict_matching=true, matching_mode='first_match' for former strict_matching=false, or omit matching_mode for the current unique default" + .to_string(), + ); + } let recorder_metadata = ToolCallRecorderMetadata::from_arguments(&arguments); let arguments = strip_tool_call_expectation_metadata(arguments); if name == "read_project_artifact" diff --git a/crates/webcodex-tool-runtime-contracts/src/tool_call_tests.rs b/crates/webcodex-tool-runtime-contracts/src/tool_call_tests.rs index 3d8131d3..917b2875 100644 --- a/crates/webcodex-tool-runtime-contracts/src/tool_call_tests.rs +++ b/crates/webcodex-tool-runtime-contracts/src/tool_call_tests.rs @@ -1315,19 +1315,33 @@ fn from_tool_name_parses_apply_patch_and_rejects_retired_patch_helpers() { json!({ "project": "agent:c:p", "patch": "*** Begin Patch\n*** Add File: new.txt\n+hello\n*** End Patch", - "strict_matching": true + "matching_mode": "exact_unique" }), ) .expect("current apply_patch DSL tool must parse"); assert!(matches!( patch, - ToolCall::ApplyPatch { project, patch, dry_run, strict_matching, .. } + ToolCall::ApplyPatch { project, patch, dry_run, matching_mode, .. } if project == "agent:c:p" && patch.contains("*** Add File: new.txt") && dry_run.is_none() - && strict_matching == Some(true) + && matching_mode == Some(webcodex_core::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique) )); + for legacy_strict in [true, false] { + let error = ToolCall::from_tool_name( + "apply_patch", + json!({ + "project": "agent:c:p", + "patch": "*** Begin Patch\n*** Add File: legacy.txt\n+hello\n*** End Patch", + "strict_matching": legacy_strict + }), + ) + .expect_err("legacy strict_matching must fail closed at current Server ingress"); + assert!(error.contains("strict_matching"), "{error}"); + assert!(error.contains("matching_mode"), "{error}"); + } + for removed in ["apply_patch_checked", "validate_patch"] { let error = ToolCall::from_tool_name(removed, json!({"project":"agent:c:p"})) .expect_err("retired patch helper names must not parse"); diff --git a/docs/CODING_WORKFLOW.md b/docs/CODING_WORKFLOW.md index f21b1c98..3cbdc882 100644 --- a/docs/CODING_WORKFLOW.md +++ b/docs/CODING_WORKFLOW.md @@ -67,7 +67,7 @@ For branch/PR review, start with the bounded review/change-summary tools exposed ## Editing -Use `apply_patch` as the default model-generated editing path. Use `apply_text_edits` for small exact SHA-guarded edits and `apply_unified_diff` when the input is already a unified diff. +Use `apply_patch` as the default model-generated editing path. Its default `matching_mode=unique` tolerates bounded whitespace/Unicode drift but writes only when the actual mutation target is unique; a repeated `@@` anchor alone is not an ambiguity when the old lines still identify one target. Use `matching_mode=exact_unique` only as an explicit stale-context/concurrency fence after reading exact current source. Use `apply_text_edits` for small exact SHA-guarded edits and `apply_unified_diff` when the input is already a unified diff. Guard failures are **zero-write conflicts**, not reasons to weaken the guard. Re-read the current source and regenerate the intended edit against that state. diff --git a/docs/CODING_WORKFLOW.zh-CN.md b/docs/CODING_WORKFLOW.zh-CN.md index 79a1ce7f..1118f2a9 100644 --- a/docs/CODING_WORKFLOW.zh-CN.md +++ b/docs/CODING_WORKFLOW.zh-CN.md @@ -55,7 +55,7 @@ Bootstrap 只读取固定的几个指令入口，不会扫描所有子目录规 ## 编辑 -模型生成的普通编辑优先使用 `apply_patch`。小型、精确且有 SHA guard 的修改使用 `apply_text_edits`；输入本身已经是 unified diff 时使用 `apply_unified_diff`。 +模型生成的普通编辑优先使用 `apply_patch`。默认 `matching_mode=unique` 可以容忍有界的空白/Unicode 漂移，但只有实际 mutation target 唯一时才会写入；如果 old lines 仍只指向一个目标，单独重复的 `@@` anchor 不算真实歧义。只有在读取过精确当前源码并明确需要 stale-context/concurrency fence 时才使用 `matching_mode=exact_unique`。小型、精确且有 SHA guard 的修改使用 `apply_text_edits`；输入本身已经是 unified diff 时使用 `apply_unified_diff`。 Guard failure 是 **zero-write conflict**，不是削弱 guard 的理由。重新读取当前源码，并基于最新状态重新生成原本的编辑。 diff --git a/src/runner_quic.rs b/src/runner_quic.rs index 81d1edb5..d9317e68 100644 --- a/src/runner_quic.rs +++ b/src/runner_quic.rs @@ -573,6 +573,7 @@ mod tests { apply_text_edit_line_scope: false, apply_patch: false, apply_patch_match_metadata: false, + apply_patch_matching_mode: false, apply_patch_strict_matching: false, git: false, jobs: true, diff --git a/src/runner_ws.rs b/src/runner_ws.rs index 7fef475a..dca8a438 100644 --- a/src/runner_ws.rs +++ b/src/runner_ws.rs @@ -378,6 +378,7 @@ mod tests { apply_text_edit_line_scope: false, apply_patch: false, apply_patch_match_metadata: false, + apply_patch_matching_mode: false, apply_patch_strict_matching: false, git: false, jobs: true, diff --git a/src/tool_runtime/dispatch.rs b/src/tool_runtime/dispatch.rs index efd651b2..154a352f 100644 --- a/src/tool_runtime/dispatch.rs +++ b/src/tool_runtime/dispatch.rs @@ -726,9 +726,10 @@ impl ToolRuntime { context_request: Vec, material_capabilities: super::context_projection::ContextMaterialCapabilities, ) -> ToolResult { - // Phase-1 edit usage telemetry: argument-free structured log only. - // Does not alter execution, session ledger, Action Audit, or schemas. - let mut edit_usage = edit_tool_telemetry::start_edit_tool_usage(call.tool_name()); + // Edit usage telemetry retains only fixed safe classifications. For + // apply_patch it captures the requested matching enum before the call is + // moved, never the patch/path/content arguments. + let mut edit_usage = edit_tool_telemetry::start_edit_tool_usage_for_call(&call); let mut result = self .dispatch_with_auth_transport_options_and_metadata_inner( call, diff --git a/src/tool_runtime/edit_tool_telemetry.rs b/src/tool_runtime/edit_tool_telemetry.rs index be156b44..894fd5ed 100644 --- a/src/tool_runtime/edit_tool_telemetry.rs +++ b/src/tool_runtime/edit_tool_telemetry.rs @@ -12,7 +12,7 @@ //! - Reuses existing `tracing` infrastructure (same family as `tool_request_trace`). //! - Does not change tool execution semantics, permissions, or session behavior. -use super::ToolResult; +use super::{ToolCall, ToolResult}; use std::time::Instant; /// High-level tool family for this telemetry stream. @@ -61,6 +61,12 @@ pub(crate) struct EditToolUsageRecord { pub(crate) duration_ms: u64, /// Optional coarse error classification (never free-form user content). pub(crate) error_kind: Option<&'static str>, + pub(crate) requested_matching_mode: Option<&'static str>, + pub(crate) selected_match_mode: Option<&'static str>, + pub(crate) candidate_count_bucket: Option<&'static str>, + pub(crate) classification: Option<&'static str>, + pub(crate) execution_state: Option<&'static str>, + pub(crate) recovery_action: Option<&'static str>, } /// Start a usage timer when `tool_name` is an edit-surface tool. @@ -71,9 +77,18 @@ pub(crate) fn start_edit_tool_usage(tool_name: &'static str) -> Option Option { + let mut guard = start_edit_tool_usage(call.tool_name())?; + if let ToolCall::ApplyPatch { matching_mode, .. } = call { + guard.requested_matching_mode = Some(matching_mode.unwrap_or_default().as_str()); + } + Some(guard) +} + /// RAII guard: records one structured log line when finished (or on drop if /// the dispatch path aborts without an explicit finish). pub(crate) struct EditToolUsageGuard { @@ -81,6 +96,7 @@ pub(crate) struct EditToolUsageGuard { edit_surface: EditToolSurface, started: Instant, finished: bool, + requested_matching_mode: Option<&'static str>, } impl EditToolUsageGuard { @@ -96,6 +112,14 @@ impl EditToolUsageGuard { success: result.success, duration_ms: self.started.elapsed().as_millis().min(u64::MAX as u128) as u64, error_kind: safe_error_kind(result), + requested_matching_mode: self + .requested_matching_mode + .or_else(|| safe_requested_matching_mode(result)), + selected_match_mode: safe_selected_match_mode(result), + candidate_count_bucket: safe_candidate_count_bucket(result), + classification: safe_classification(result), + execution_state: safe_execution_state(result), + recovery_action: safe_recovery_action(result), }; emit_edit_tool_usage(&record); } @@ -112,6 +136,12 @@ impl EditToolUsageGuard { success: false, duration_ms: self.started.elapsed().as_millis().min(u64::MAX as u128) as u64, error_kind: Some("incomplete"), + requested_matching_mode: self.requested_matching_mode, + selected_match_mode: None, + candidate_count_bucket: None, + classification: None, + execution_state: None, + recovery_action: None, }; emit_edit_tool_usage(&record); } @@ -136,10 +166,171 @@ pub(crate) fn emit_edit_tool_usage(record: &EditToolUsageRecord) { success = record.success, duration_ms = record.duration_ms, error_kind = record.error_kind.unwrap_or("-"), + requested_matching_mode = record.requested_matching_mode.unwrap_or("-"), + selected_match_mode = record.selected_match_mode.unwrap_or("-"), + candidate_count_bucket = record.candidate_count_bucket.unwrap_or("-"), + classification = record.classification.unwrap_or("-"), + execution_state = record.execution_state.unwrap_or("-"), + recovery_action = record.recovery_action.unwrap_or("-"), "{EDIT_TOOL_USAGE_EVENT}" ); } +fn safe_requested_matching_mode(result: &ToolResult) -> Option<&'static str> { + safe_matching_mode( + result + .output + .get("requested_matching_mode") + .and_then(|value| value.as_str())?, + ) +} + +fn safe_matching_mode(value: &str) -> Option<&'static str> { + match value { + "first_match" => Some("first_match"), + "unique" => Some("unique"), + "exact_unique" => Some("exact_unique"), + _ => None, + } +} + +fn safe_match_mode(value: &str) -> Option<&'static str> { + match value { + "exact" => Some("exact"), + "trim_end" => Some("trim_end"), + "trim" => Some("trim"), + "normalized" => Some("normalized"), + _ => None, + } +} + +fn match_mode_rank(value: &str) -> u8 { + match value { + "exact" => 0, + "trim_end" => 1, + "trim" => 2, + "normalized" => 3, + _ => 0, + } +} + +fn safe_selected_match_mode(result: &ToolResult) -> Option<&'static str> { + if let Some(mode) = result + .output + .get("match_rejection_diagnostic") + .and_then(|value| value.get("match_mode")) + .and_then(|value| value.as_str()) + .and_then(safe_match_mode) + { + return Some(mode); + } + result + .output + .get("files") + .and_then(|value| value.as_array()) + .into_iter() + .flatten() + .filter_map(|file| file.get("edits").and_then(|value| value.as_array())) + .flatten() + .filter_map(|edit| edit.get("match_mode").and_then(|value| value.as_str())) + .filter_map(safe_match_mode) + .max_by_key(|mode| match_mode_rank(mode)) +} + +fn safe_candidate_count_bucket(result: &ToolResult) -> Option<&'static str> { + let diagnostic_count = result + .output + .get("match_rejection_diagnostic") + .and_then(|value| value.get("candidate_count")) + .and_then(|value| value.as_u64()); + let success_max = result + .output + .get("files") + .and_then(|value| value.as_array()) + .into_iter() + .flatten() + .filter_map(|file| file.get("edits").and_then(|value| value.as_array())) + .flatten() + .filter_map(|edit| edit.get("candidate_count").and_then(|value| value.as_u64())) + .max(); + let count = diagnostic_count.or(success_max)?; + Some(match count { + 0 => "0", + 1 => "1", + 2 => "2", + 3..=4 => "3-4", + _ => "5+", + }) +} + +fn safe_classification(result: &ToolResult) -> Option<&'static str> { + if let Some(classification) = result + .output + .get("match_rejection_diagnostic") + .and_then(|value| value.get("classification")) + .and_then(|value| value.as_str()) + { + return match classification { + "unique_fuzzy_candidate" => Some("unique_fuzzy_candidate"), + "ambiguous_candidate" => Some("ambiguous_candidate"), + _ => None, + }; + } + match result + .output + .get("error_kind") + .and_then(|value| value.as_str()) + { + Some("context_mismatch") => Some("context_mismatch"), + Some("matching_mode_rejected") => Some("matching_metadata_suppressed"), + _ => None, + } +} + +fn safe_execution_state(result: &ToolResult) -> Option<&'static str> { + match result + .output + .get("execution_state") + .and_then(|value| value.as_str()) + { + Some("not_started") => Some("not_started"), + Some("completed") => Some("completed"), + Some("outcome_unknown") => Some("outcome_unknown"), + _ => None, + } +} + +fn safe_recovery_action(result: &ToolResult) -> Option<&'static str> { + let raw = result + .output + .get("recovery_action") + .and_then(|value| value.as_str()) + .or_else(|| { + result + .output + .get("recovery") + .and_then(|value| value.get("action")) + .and_then(|value| value.as_str()) + })?; + match raw { + "read_files" => Some("read_files"), + "read_equal_candidates_and_refine_context" => { + Some("read_equal_candidates_and_refine_context") + } + "reread_and_regenerate_exact_unique_patch" => { + Some("reread_and_regenerate_exact_unique_patch") + } + "read_equal_candidates_and_add_exact_context" => { + Some("read_equal_candidates_and_add_exact_context") + } + "reread_and_regenerate_patch" => Some("reread_and_regenerate_patch"), + "inspect_workspace_before_retry" => Some("inspect_workspace_before_retry"), + "upgrade_or_reconnect_runner" => Some("upgrade_or_reconnect_runner"), + "retry_same_after_runner_recovery" => Some("retry_same_after_runner_recovery"), + _ => None, + } +} + /// Extract only coarse, allowlisted error kinds from tool results. /// /// Never returns free-form error messages (which may include paths or snippets). @@ -173,6 +364,9 @@ fn sanitize_error_kind(kind: &str) -> Option<&'static str> { "timeout" => Some("timeout"), "not_started" => Some("not_started"), "outcome_unknown" => Some("outcome_unknown"), + "context_mismatch" => Some("context_mismatch"), + "matching_mode_rejected" => Some("matching_mode_rejected"), + "agent_capability_unavailable" => Some("agent_capability_unavailable"), "not_found" => Some("not_found"), "runtime_error" => Some("runtime_error"), "incomplete" => Some("incomplete"), @@ -189,7 +383,18 @@ pub(crate) fn record_contains_sensitive_keys(record: &EditToolUsageRecord) -> bo // from test fixtures that should never appear in telemetry. let surface = record.edit_surface.as_str(); let kind = record.error_kind.unwrap_or(""); - let haystacks = [record.tool_name, record.category, surface, kind]; + let haystacks = [ + record.tool_name, + record.category, + surface, + kind, + record.requested_matching_mode.unwrap_or(""), + record.selected_match_mode.unwrap_or(""), + record.candidate_count_bucket.unwrap_or(""), + record.classification.unwrap_or(""), + record.execution_state.unwrap_or(""), + record.recovery_action.unwrap_or(""), + ]; for h in haystacks { if h.contains('/') || h.contains('\\') || h.contains('\n') { return true; @@ -303,6 +508,12 @@ mod tests { success: false, duration_ms: 12, error_kind: Some("runtime_error"), + requested_matching_mode: None, + selected_match_mode: None, + candidate_count_bucket: None, + classification: None, + execution_state: None, + recovery_action: None, }; assert!(!record_contains_sensitive_keys(&record)); assert_eq!(record.category, "edit"); @@ -347,6 +558,52 @@ mod tests { assert!(!record_contains_sensitive_keys(&events[0])); } + #[test] + fn apply_patch_telemetry_records_default_unique_and_bounded_match_facts() { + clear_test_edit_tool_usage(); + let call = ToolCall::ApplyPatch { + project: "agent:test:project".to_string(), + patch: "*** Begin Patch\n*** Update File: secret.rs\n-old\n+new\n*** End Patch" + .to_string(), + dry_run: None, + matching_mode: None, + session_id: None, + }; + let mut guard = start_edit_tool_usage_for_call(&call).expect("apply_patch edit tool"); + guard.finish_with_result(&ToolResult::err_with_output( + "ambiguous", + json!({ + "requested_matching_mode": "unique", + "execution_state": "not_started", + "error_kind": "matching_mode_rejected", + "recovery_action": "read_equal_candidates_and_refine_context", + "match_rejection_diagnostic": { + "classification": "ambiguous_candidate", + "match_mode": "exact", + "candidate_count": 2 + }, + "path": "/must/not/appear", + "patch": "must not appear" + }), + )); + drop(guard); + + let events = take_test_edit_tool_usage(); + assert_eq!(events.len(), 1); + let event = &events[0]; + assert_eq!(event.requested_matching_mode, Some("unique")); + assert_eq!(event.selected_match_mode, Some("exact")); + assert_eq!(event.candidate_count_bucket, Some("2")); + assert_eq!(event.classification, Some("ambiguous_candidate")); + assert_eq!(event.execution_state, Some("not_started")); + assert_eq!( + event.recovery_action, + Some("read_equal_candidates_and_refine_context") + ); + assert_eq!(event.error_kind, Some("matching_mode_rejected")); + assert!(!record_contains_sensitive_keys(event)); + } + #[test] fn start_returns_none_for_non_edit_tools() { assert!(start_edit_tool_usage("read_file").is_none()); diff --git a/src/tool_runtime/files/mutations.rs b/src/tool_runtime/files/mutations.rs index 6fdc2d89..92cdde68 100644 --- a/src/tool_runtime/files/mutations.rs +++ b/src/tool_runtime/files/mutations.rs @@ -224,26 +224,6 @@ fn apply_patch_capability_rejection( .with_recovery(crate::tool_runtime::RecoveryKind::RetrySame, None) } -fn apply_patch_strict_matching_capability_rejection(reason: impl AsRef) -> ToolResult { - let reason = reason.as_ref(); - ToolResult::err_with_output( - format!( - "Rejected before write: {reason}.\nNo files were modified.\nRetry guidance: reconnect a Runner that explicitly supports apply_patch_strict_matching, or disable strict_matching only if ordinary Codex fuzzy/first-match positioning is acceptable." - ), - json!({ - "changed": false, - "state_changed": false, - "execution_state": "not_started", - "error_kind": "agent_capability_unavailable", - "failure_kind": "capability_unavailable", - "capability": crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING, - "recovery_action": "upgrade_or_reconnect_runner", - "retry_guidance": "reconnect or upgrade the Runner so it explicitly advertises apply_patch_strict_matching; never silently downgrade a strict patch" - }), - ) - .with_recovery(crate::tool_runtime::RecoveryKind::RetrySame, None) -} - /// Maximum decoded size for whole-payload/model-facing artifact operations. /// These paths aggregate content or return it as base64/JSON, so they remain at /// 10 MiB even though data-plane upload/export paths admit larger files. @@ -581,7 +561,7 @@ fn apply_patch_nullable_sha256(value: Option<&Value>, required: bool) -> bool { } } -const APPLY_PATCH_SUCCESS_TOP_LEVEL_FIELDS: [&str; 8] = [ +const APPLY_PATCH_SUCCESS_TOP_LEVEL_FIELDS: [&str; 9] = [ "dry_run", "applied_count", "changed", @@ -590,6 +570,7 @@ const APPLY_PATCH_SUCCESS_TOP_LEVEL_FIELDS: [&str; 8] = [ "would_change", "files", "changed_paths", + "requested_matching_mode", ]; const APPLY_PATCH_SUCCESS_FILE_FIELDS: [&str; 9] = [ @@ -604,7 +585,7 @@ const APPLY_PATCH_SUCCESS_FILE_FIELDS: [&str; 9] = [ "edits", ]; -const APPLY_PATCH_SUCCESS_EDIT_FIELDS: [&str; 10] = [ +const APPLY_PATCH_SUCCESS_EDIT_FIELDS: [&str; 11] = [ "chunk_index", "change_context_present", "old_line_count", @@ -614,6 +595,7 @@ const APPLY_PATCH_SUCCESS_EDIT_FIELDS: [&str; 10] = [ "match_source", "matched_start_line", "candidate_count", + "unique_match", "strict_match", ]; @@ -656,14 +638,17 @@ const APPLY_PATCH_RECOVERY_MARGIN_BEFORE: usize = 8; const APPLY_PATCH_RECOVERY_MARGIN_AFTER: usize = 8; #[derive(Debug)] -struct ValidatedApplyPatchStrictRejection { +struct ValidatedApplyPatchMatchRejection { change_index: usize, chunk_index: usize, path: String, + requested_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, match_mode: &'static str, match_source: &'static str, - matched_start_line: usize, + matched_start_line: Option, candidate_count: usize, + candidate_start_lines: Vec, + candidate_positions_truncated: bool, expected_line_count: usize, source_line_count: usize, classification: &'static str, @@ -689,20 +674,28 @@ fn expected_apply_patch_failure_pattern_len( } } -fn validated_apply_patch_strict_rejection( +fn validated_apply_patch_match_rejection( patch: &crate::apply_patch_shared::CodexPatch, failure_output: &Value, - expected_strict_matching: bool, -) -> Option { - if !expected_strict_matching + expected_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, +) -> Option { + if expected_matching_mode == crate::apply_patch_shared::ApplyPatchMatchingMode::FirstMatch || failure_output.get("changed").and_then(Value::as_bool) != Some(false) || failure_output.get("state_changed").and_then(Value::as_bool) != Some(false) || failure_output .get("execution_state") .and_then(Value::as_str) != Some("not_started") - || failure_output.get("error_kind").and_then(Value::as_str) != Some("strict_match_rejected") - || failure_output.get("strict_match").and_then(Value::as_bool) != Some(false) + || failure_output.get("error_kind").and_then(Value::as_str) + != Some("matching_mode_rejected") + || failure_output + .get("requested_matching_mode") + .and_then(Value::as_str) + != Some(expected_matching_mode.as_str()) + || failure_output + .get("matching_mode_satisfied") + .and_then(Value::as_bool) + != Some(false) { return None; } @@ -725,10 +718,18 @@ fn validated_apply_patch_strict_rejection( let chunk = chunks.get(chunk_index)?; let match_source = match failure_output.get("match_source").and_then(Value::as_str)? { "old_lines" if !chunk.old_lines.is_empty() => "old_lines", - "change_context" if chunk.change_context.is_some() => "change_context", + "change_context" + if chunk.change_context.is_some() + && (expected_matching_mode + == crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique + || chunk.old_lines.is_empty()) => + { + "change_context" + } _ => { // Unanchored append performs no text matching and is strict-safe; - // other sources contradict the parsed chunk shape. + // other sources contradict the parsed chunk shape or the selected + // matching mode's positioning semantics. return None; } }; @@ -738,12 +739,9 @@ fn validated_apply_patch_strict_rejection( "exact" => "exact", "trim_end" => "trim_end", "trim" => "trim", + "normalized" => "normalized", _ => return None, }; - let matched_start_line = failure_output - .get("matched_start_line")? - .as_u64() - .and_then(|value| usize::try_from(value).ok())?; let search_start_line = failure_output .get("search_start_line")? .as_u64() @@ -752,17 +750,23 @@ fn validated_apply_patch_strict_rejection( .get("source_line_count")? .as_u64() .and_then(|value| usize::try_from(value).ok())?; - if matched_start_line == 0 || search_start_line == 0 || source_line_count < expected_line_count - { + if search_start_line == 0 || source_line_count < expected_line_count { return None; } let last_start_line = source_line_count .checked_sub(expected_line_count)? .checked_add(1)?; - if search_start_line > last_start_line - || matched_start_line < search_start_line - || matched_start_line > last_start_line + if search_start_line > last_start_line { + return None; + } + if expected_matching_mode == crate::apply_patch_shared::ApplyPatchMatchingMode::Unique + && match_source == "old_lines" + && chunk.is_end_of_file + && search_start_line != last_start_line { + // Under Unique, *** End of File is a structural eligibility fence for + // old_lines. A Runner cannot expand that eligible range back toward the + // beginning of the file by forging search_start_line metadata. return None; } let max_candidate_count = last_start_line @@ -776,83 +780,139 @@ fn validated_apply_patch_strict_rejection( return None; } + let candidate_positions_truncated = failure_output + .get("candidate_positions_truncated")? + .as_bool()?; + let candidate_start_lines = failure_output + .get("candidate_start_lines")? + .as_array()? + .iter() + .map(|value| value.as_u64().and_then(|value| usize::try_from(value).ok())) + .collect::>>()?; + if candidate_start_lines.is_empty() + || candidate_start_lines.len() + > crate::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS + || candidate_start_lines + .windows(2) + .any(|pair| pair[0] >= pair[1]) + || candidate_start_lines + .iter() + .any(|line| *line < search_start_line || *line > last_start_line) + { + return None; + } + let position_cap = crate::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS; + if candidate_count <= position_cap { + if candidate_positions_truncated || candidate_start_lines.len() != candidate_count { + return None; + } + } else if !candidate_positions_truncated || candidate_start_lines.len() != position_cap { + return None; + } + let classification = if candidate_count == 1 { - if match_mode == "exact" { - // Exact + unique is strict-safe and therefore contradicts the - // reported rejection. + if expected_matching_mode != crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique + || match_mode == "exact" + { return None; } "unique_fuzzy_candidate" } else { "ambiguous_candidate" }; - Some(ValidatedApplyPatchStrictRejection { + let matched_start_line = match classification { + "unique_fuzzy_candidate" => { + let line = failure_output + .get("matched_start_line")? + .as_u64() + .and_then(|value| usize::try_from(value).ok())?; + if candidate_start_lines.as_slice() != [line] { + return None; + } + Some(line) + } + "ambiguous_candidate" => { + if failure_output.get("matched_start_line") != Some(&Value::Null) { + return None; + } + None + } + _ => return None, + }; + Some(ValidatedApplyPatchMatchRejection { change_index, chunk_index, path: hunk.path().to_string(), + requested_matching_mode: expected_matching_mode, match_mode, match_source, matched_start_line, candidate_count, + candidate_start_lines, + candidate_positions_truncated, expected_line_count, classification, source_line_count, }) } -fn apply_patch_strict_rejection_recovery( - rejection: &ValidatedApplyPatchStrictRejection, +fn apply_patch_match_rejection_recovery( + rejection: &ValidatedApplyPatchMatchRejection, ) -> Option { - if rejection.classification != "unique_fuzzy_candidate" { - return None; - } - let start_line = rejection - .matched_start_line - .saturating_sub(APPLY_PATCH_RECOVERY_MARGIN_BEFORE) - .max(1); let requested_limit = rejection .expected_line_count .saturating_add(APPLY_PATCH_RECOVERY_MARGIN_BEFORE) .saturating_add(APPLY_PATCH_RECOVERY_MARGIN_AFTER) .min(crate::apply_patch_shared::MAX_CODEX_PATCH_RECOVERY_READ_LINES) .max(1); - let available_from_start = rejection - .source_line_count - .checked_sub(start_line)? - .checked_add(1)?; - let limit = requested_limit.min(available_from_start); - if limit == 0 { + let mut items = Vec::with_capacity(rejection.candidate_start_lines.len()); + for candidate_start_line in &rejection.candidate_start_lines { + let start_line = candidate_start_line + .saturating_sub(APPLY_PATCH_RECOVERY_MARGIN_BEFORE) + .max(1); + let available_from_start = rejection + .source_line_count + .checked_sub(start_line)? + .checked_add(1)?; + let limit = requested_limit.min(available_from_start); + if limit == 0 { + return None; + } + items.push(json!({ + "path": rejection.path.as_str(), + "start_line": start_line, + "limit": limit, + })); + } + if items.is_empty() { return None; } Some(json!({ "action": "read_files", - "reason": "strict_match_rejected_unique_fuzzy", - "items": [{ - "path": rejection.path.as_str(), - "start_line": start_line, - "limit": limit, - }], + "reason": if rejection.classification == "ambiguous_candidate" { + "matching_mode_rejected_ambiguous" + } else { + "matching_mode_rejected_unique_fuzzy" + }, + "items": items, "change_index": rejection.change_index, "chunk_index": rejection.chunk_index, })) } -fn apply_patch_strict_rejection_diagnostic( - rejection: &ValidatedApplyPatchStrictRejection, -) -> Value { +fn apply_patch_match_rejection_diagnostic(rejection: &ValidatedApplyPatchMatchRejection) -> Value { json!({ "classification": rejection.classification, + "requested_matching_mode": rejection.requested_matching_mode.as_str(), "chunk_index": rejection.chunk_index, "match_mode": rejection.match_mode, "match_source": rejection.match_source, - "matched_start_line": if rejection.classification == "unique_fuzzy_candidate" { - Some(rejection.matched_start_line) - } else { - None - }, + "matched_start_line": rejection.matched_start_line, "candidate_count": rejection.candidate_count, + "candidate_start_lines": rejection.candidate_start_lines, + "candidate_positions_truncated": rejection.candidate_positions_truncated, "expected_line_count": rejection.expected_line_count, - "strict_match": false, + "matching_mode_satisfied": false, }) } @@ -1059,29 +1119,41 @@ fn apply_patch_context_mismatch_recovery( })) } -fn sanitize_apply_patch_strict_rejection( +fn sanitize_apply_patch_match_rejection( result: &mut ToolResult, patch: &crate::apply_patch_shared::CodexPatch, + expected_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, ) -> bool { - if result.output.get("error_kind").and_then(Value::as_str) != Some("strict_match_rejected") { + if result.output.get("error_kind").and_then(Value::as_str) != Some("matching_mode_rejected") { return false; } - let validated = validated_apply_patch_strict_rejection(patch, &result.output, true); + let validated = + validated_apply_patch_match_rejection(patch, &result.output, expected_matching_mode); let (message, recovery_action, retry_guidance) = match validated.as_ref() { + Some(rejection) + if rejection.requested_matching_mode + == crate::apply_patch_shared::ApplyPatchMatchingMode::Unique => + { + ( + "Rejected Codex patch before write: Server-validated positioning is ambiguous under matching_mode=unique. No files were modified.".to_string(), + "read_equal_candidates_and_refine_context", + "read every recovery.items window as an equal candidate, then add a stable parent/function/test/module anchor or small surrounding context and retry with matching_mode=unique; do not choose a candidate from its order", + ) + } Some(rejection) if rejection.classification == "unique_fuzzy_candidate" => ( - "Rejected strict Codex patch before write: Server-validated positioning found one fuzzy candidate. No files were modified.".to_string(), - "reread_and_regenerate_strict_patch", - "read recovery.items, regenerate this chunk with exact unique context against the current source, and retry with strict_matching=true; do not relax strict matching", + "Rejected Codex patch before write: matching_mode=exact_unique found one non-exact candidate. No files were modified.".to_string(), + "reread_and_regenerate_exact_unique_patch", + "read recovery.items, regenerate this chunk from exact current source, and retry with matching_mode=exact_unique; do not downgrade the requested fence", ), Some(_) => ( - "Rejected strict Codex patch before write: Server-validated positioning is ambiguous, so no authoritative target location was selected. No files were modified.".to_string(), - "add_exact_unique_context", - "the current patch context matches multiple candidates; expand exact unique context and retry with strict_matching=true; do not select a candidate position or relax strict matching", + "Rejected Codex patch before write: Server-validated positioning is ambiguous under matching_mode=exact_unique. No files were modified.".to_string(), + "read_equal_candidates_and_add_exact_context", + "read every recovery.items window as an equal candidate, expand exact context until the target is unique, and retry with matching_mode=exact_unique; do not choose a candidate from its order", ), None => ( - "Rejected strict Codex patch before write: Runner strict-match metadata was invalid or contradictory and was suppressed. No files were modified.".to_string(), - "regenerate_strict_patch", - "do not trust the rejected target metadata; reread current source through normal read tooling as needed, regenerate exact unique context, and retry with strict_matching=true", + "Rejected Codex patch before write: Runner matching metadata was invalid or contradictory and was suppressed. No files were modified.".to_string(), + "reread_and_regenerate_patch", + "do not trust the rejected target metadata; reread current source through normal read tooling, regenerate context, and retry with the same matching_mode", ), }; @@ -1103,6 +1175,10 @@ fn sanitize_apply_patch_strict_rejection( ] { fields.remove(key); } + fields.insert( + "requested_matching_mode".to_string(), + json!(expected_matching_mode.as_str()), + ); fields.insert("recovery_action".to_string(), json!(recovery_action)); fields.insert("retry_guidance".to_string(), json!(retry_guidance)); fields.insert("error".to_string(), json!(message.as_str())); @@ -1110,10 +1186,10 @@ fn sanitize_apply_patch_strict_rejection( fields.insert("change_index".to_string(), json!(rejection.change_index)); fields.insert("path".to_string(), json!(rejection.path.as_str())); fields.insert( - "strict_match_diagnostic".to_string(), - apply_patch_strict_rejection_diagnostic(rejection), + "match_rejection_diagnostic".to_string(), + apply_patch_match_rejection_diagnostic(rejection), ); - if let Some(recovery) = apply_patch_strict_rejection_recovery(rejection) { + if let Some(recovery) = apply_patch_match_rejection_recovery(rejection) { fields.insert("recovery".to_string(), recovery); } } @@ -1125,33 +1201,36 @@ fn sanitize_apply_patch_strict_rejection( fn sanitize_apply_patch_failure_metadata( result: &mut ToolResult, patch: &crate::apply_patch_shared::CodexPatch, - expected_strict_matching: bool, + expected_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, ) { + if result.output.get("error_kind").and_then(Value::as_str) == Some("matching_mode_rejected") { + let _ = sanitize_apply_patch_match_rejection(result, patch, expected_matching_mode); + return; + } if result.output.get("error_kind").and_then(Value::as_str) == Some("strict_match_rejected") { - if expected_strict_matching { - let _ = sanitize_apply_patch_strict_rejection(result, patch); - } else { - let message = "Rejected apply_patch result: Runner reported a strict-match rejection for a non-strict request; target metadata was suppressed.".to_string(); - if let Some(fields) = result.output.as_object_mut() { - fields.retain(|key, _| { - matches!( - key.as_str(), - "changed" - | "state_changed" - | "execution_state" - | "error_kind" - | "tool_failure" - ) - }); - fields.insert("recovery_action".to_string(), json!("regenerate_patch")); - fields.insert( - "retry_guidance".to_string(), - json!("do not trust the rejected target metadata; regenerate the patch from current source before another write"), - ); - fields.insert("error".to_string(), json!(message.as_str())); - } - result.error = Some(message); + let message = "Rejected apply_patch result: a current matching_mode request received legacy strict-match metadata; target metadata was suppressed.".to_string(); + if let Some(fields) = result.output.as_object_mut() { + fields.retain(|key, _| { + matches!( + key.as_str(), + "changed" | "state_changed" | "execution_state" | "error_kind" | "tool_failure" + ) + }); + fields.insert( + "requested_matching_mode".to_string(), + json!(expected_matching_mode.as_str()), + ); + fields.insert( + "recovery_action".to_string(), + json!("reread_and_regenerate_patch"), + ); + fields.insert( + "retry_guidance".to_string(), + json!("do not trust legacy target metadata; reread current source and retry with the same matching_mode"), + ); + fields.insert("error".to_string(), json!(message.as_str())); } + result.error = Some(message); return; } @@ -1164,6 +1243,10 @@ fn sanitize_apply_patch_failure_metadata( return; }; fields.retain(|key, _| APPLY_PATCH_FAILURE_TOP_LEVEL_FIELDS.contains(&key.as_str())); + fields.insert( + "requested_matching_mode".to_string(), + json!(expected_matching_mode.as_str()), + ); if !diagnostic_valid { fields.remove("match_diagnostic"); } @@ -1200,7 +1283,7 @@ fn validate_apply_patch_edit_summary( value: &Value, chunk_index: usize, chunk: &crate::apply_patch_shared::CodexPatchChunk, - strict_matching: bool, + matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, ) -> bool { let Some(edit) = value.as_object() else { return false; @@ -1237,7 +1320,7 @@ fn validate_apply_patch_edit_summary( } else { match_mode .and_then(Value::as_str) - .is_some_and(|mode| matches!(mode, "exact" | "trim_end" | "trim")) + .is_some_and(|mode| matches!(mode, "exact" | "trim_end" | "trim" | "normalized")) && candidate_count .and_then(Value::as_u64) .is_some_and(|count| count >= 1) @@ -1246,29 +1329,48 @@ fn validate_apply_patch_edit_summary( return false; } + let Some(unique_match) = edit.get("unique_match").and_then(Value::as_bool) else { + return false; + }; let Some(strict_match) = edit.get("strict_match").and_then(Value::as_bool) else { return false; }; - if expected_source == "append" && !strict_match { + if expected_source == "append" { + return unique_match && strict_match; + } + let candidate_is_unique = candidate_count.and_then(Value::as_u64) == Some(1); + if unique_match && !candidate_is_unique { return false; } - if strict_matching && !strict_match { + if unique_match != candidate_is_unique { return false; } - if strict_match && expected_source != "append" { - return match_mode.and_then(Value::as_str) == Some("exact") - && candidate_count.and_then(Value::as_u64) == Some(1); + if strict_match + && (match_mode.and_then(Value::as_str) != Some("exact") + || !candidate_is_unique + || !unique_match) + { + return false; + } + match matching_mode { + crate::apply_patch_shared::ApplyPatchMatchingMode::FirstMatch => true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique => unique_match, + crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique => strict_match, } - true } fn validate_apply_patch_success_metadata( output: &Value, patch: &crate::apply_patch_shared::CodexPatch, expected_dry_run: bool, - expected_strict_matching: bool, + expected_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, ) -> bool { - if !output.is_object() { + if !output.is_object() + || output + .get("requested_matching_mode") + .and_then(Value::as_str) + != Some(expected_matching_mode.as_str()) + { return false; } let Some(files) = output.get("files").and_then(Value::as_array) else { @@ -1360,7 +1462,7 @@ fn validate_apply_patch_success_metadata( edit, chunk_index, chunk, - expected_strict_matching, + expected_matching_mode, ) }) { @@ -1388,7 +1490,7 @@ fn apply_patch_agent_stdout_result( stdout: &str, patch: &crate::apply_patch_shared::CodexPatch, expected_dry_run: bool, - expected_strict_matching: bool, + expected_matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, ) -> ToolResult { let mut result = transactional_edit_agent_stdout_result( "apply_patch", @@ -1397,7 +1499,7 @@ fn apply_patch_agent_stdout_result( expected_dry_run, ); if !result.success { - sanitize_apply_patch_failure_metadata(&mut result, patch, expected_strict_matching); + sanitize_apply_patch_failure_metadata(&mut result, patch, expected_matching_mode); return result; } sanitize_apply_patch_success_metadata(&mut result.output); @@ -1405,7 +1507,7 @@ fn apply_patch_agent_stdout_result( &result.output, patch, expected_dry_run, - expected_strict_matching, + expected_matching_mode, ) { return result; } @@ -2106,7 +2208,7 @@ impl ToolRuntime { project: String, patch: String, dry_run: Option, - strict_matching: Option, + matching_mode: Option, ) -> ToolResult { let parsed = match crate::apply_patch_shared::parse_codex_patch(&patch) { Ok(parsed) => parsed, @@ -2160,14 +2262,12 @@ impl ToolRuntime { } let expected_dry_run = dry_run.unwrap_or(false); - let expected_strict_matching = strict_matching.unwrap_or(false); - let mut payload = json!({ + let expected_matching_mode = matching_mode.unwrap_or_default(); + let payload = json!({ "patch": patch, "dry_run": expected_dry_run, + "matching_mode": expected_matching_mode.as_str(), }); - if expected_strict_matching { - payload["strict_matching"] = json!(true); - } let serialized = match serde_json::to_string(&payload) { Ok(serialized) if serialized.len() <= MAX_APPLY_FILE_CHANGES_BYTES => serialized, Ok(_) => { @@ -2226,21 +2326,20 @@ impl ToolRuntime { }; let (request_id, rx) = match self .runner_registry - .enqueue_apply_patch( - request, - expected_strict_matching, - "tool_runtime".to_string(), - ) + .enqueue_apply_patch(request, "tool_runtime".to_string()) .await { Ok(request) => request, Err(error) if error.starts_with("capability_unavailable:") && error.contains( - crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING, + crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, ) => { - return apply_patch_strict_matching_capability_rejection(error) + return apply_patch_capability_rejection( + error, + crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, + ) } Err(error) if error.starts_with("capability_unavailable:") @@ -2285,7 +2384,7 @@ impl ToolRuntime { &response.stdout.unwrap_or_default(), &parsed, expected_dry_run, - expected_strict_matching, + expected_matching_mode, ) } @@ -2528,12 +2627,15 @@ mod tests { use super::*; fn apply_patch_success_payload( + matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, match_mode: &str, candidate_count: u64, + unique_match: bool, strict_match: bool, ) -> Value { json!({ "dry_run": true, + "requested_matching_mode": matching_mode.as_str(), "applied_count": 1, "changed": false, "state_changed": false, @@ -2558,6 +2660,7 @@ mod tests { "match_source": "old_lines", "matched_start_line": 1, "candidate_count": candidate_count, + "unique_match": unique_match, "strict_match": strict_match, }] }], @@ -2611,26 +2714,36 @@ mod tests { }) } - fn strict_rejection_payload( + fn matching_rejection_payload( + matching_mode: crate::apply_patch_shared::ApplyPatchMatchingMode, match_mode: &str, + candidate_start_lines: &[usize], candidate_count: usize, - matched_start_line: usize, + source_line_count: usize, ) -> Value { + let ambiguous = candidate_count > 1; json!({ "changed": false, "state_changed": false, "execution_state": "not_started", - "error_kind": "strict_match_rejected", + "error_kind": "matching_mode_rejected", "change_index": 0, "path": "file.txt", "chunk_index": 0, + "requested_matching_mode": matching_mode.as_str(), "match_mode": match_mode, "match_source": "old_lines", - "matched_start_line": matched_start_line, + "matched_start_line": if ambiguous { + Value::Null + } else { + json!(candidate_start_lines.first().copied()) + }, "candidate_count": candidate_count, - "strict_match": false, + "candidate_start_lines": candidate_start_lines, + "candidate_positions_truncated": candidate_count > crate::apply_patch_shared::MAX_CODEX_PATCH_CANDIDATE_POSITIONS, + "matching_mode_satisfied": false, "search_start_line": 1, - "source_line_count": 100, + "source_line_count": source_line_count, "recovery_action": "RUNNER_MUST_NOT_CHOOSE_RECOVERY", "retry_guidance": "RUNNER_MUST_NOT_CHOOSE_GUIDANCE", "error": "RUNNER_MUST_NOT_CHOOSE_ERROR", @@ -2638,9 +2751,10 @@ mod tests { } #[test] - fn apply_patch_strict_capability_rejection_names_exact_additive_capability() { - let result = apply_patch_strict_matching_capability_rejection( - "capability_unavailable: demo lacks apply_patch_strict_matching", + fn apply_patch_matching_mode_capability_rejection_fails_closed() { + let result = apply_patch_capability_rejection( + "capability_unavailable: demo lacks apply_patch_matching_mode", + crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE, ); assert!(!result.success); @@ -2649,13 +2763,9 @@ mod tests { assert_eq!(result.output["error_kind"], "agent_capability_unavailable"); assert_eq!( result.output["capability"], - crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_STRICT_MATCHING + crate::runner_protocol::RUNNER_CAPABILITY_APPLY_PATCH_MATCHING_MODE ); assert_eq!(result.output["recovery_kind"], "retry_same"); - assert!(result.output["retry_guidance"] - .as_str() - .unwrap() - .contains("never silently downgrade")); } #[test] @@ -2683,7 +2793,12 @@ mod tests { "first_exact_mismatch_offset": 1 } }); - let result = apply_patch_agent_stdout_result(&valid.to_string(), &patch, false, true); + let result = apply_patch_agent_stdout_result( + &valid.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["match_diagnostic"]["closest_start_line"], 5); let recovery_action = result.output["recovery"]["action"].as_str().unwrap(); @@ -2731,7 +2846,12 @@ mod tests { cases.push(out_of_range_candidate); for invalid in cases { - let result = apply_patch_agent_stdout_result(&invalid.to_string(), &patch, false, true); + let result = apply_patch_agent_stdout_result( + &invalid.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["execution_state"], "not_started"); assert_eq!(result.output["state_changed"], false); @@ -2750,7 +2870,7 @@ mod tests { &context_mismatch_payload(5, 120, 50, Some(130)).to_string(), &patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); let schema = crate::tool_runtime::registry::output_schema_for_tool("apply_patch"); crate::tool_runtime::startup_brief::validate_schema_instance_for_test( @@ -2765,7 +2885,7 @@ mod tests { &context_mismatch_payload(5, 1, 20, Some(1)).to_string(), &patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); assert_eq!(near_start.output["recovery"]["items"][0]["start_line"], 1); assert_eq!(near_start.output["recovery"]["items"][0]["limit"], 20); @@ -2774,7 +2894,7 @@ mod tests { &context_mismatch_payload(5, 11, 2, Some(12)).to_string(), &patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); let recovery = &eof_partial.output["recovery"]["items"][0]; assert_eq!(recovery["start_line"], 4); @@ -2789,7 +2909,7 @@ mod tests { &context_mismatch_payload(100, 1, 300, Some(100)).to_string(), &large_patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); assert_eq!( large.output["recovery"]["items"][0]["limit"], @@ -2805,7 +2925,7 @@ mod tests { &distant_mismatch_payload.to_string(), &large_patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); let recovery = &distant_mismatch.output["recovery"]["items"][0]; assert_eq!(recovery["start_line"], 134); @@ -2826,7 +2946,7 @@ mod tests { &context_mismatch_payload(3, 5, 0, None).to_string(), &patch, false, - false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); assert!(no_candidate.output.get("match_diagnostic").is_some()); assert!(no_candidate.output.get("recovery").is_none()); @@ -2846,8 +2966,12 @@ mod tests { "change_index": 0, "chunk_index": 0 }); - let result = - apply_patch_agent_stdout_result(&payload.to_string(), &private_patch, false, false); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &private_patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); let serialized = serde_json::to_string(&result.output).unwrap(); assert!(!serialized.contains("SOURCE_PRIVATE_TOKEN")); assert!(!serialized.contains("PATCH_PRIVATE_TOKEN")); @@ -2867,7 +2991,12 @@ mod tests { payload["changed"] = json!(true); payload["recovery"] = json!({"action": "read_file", "path": "other.txt"}); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, false, false); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["execution_state"], "outcome_unknown"); assert_eq!( @@ -2887,73 +3016,80 @@ mod tests { } #[test] - fn apply_patch_strict_unique_fuzzy_rejection_gets_validated_bounded_reread() { + fn apply_patch_exact_unique_fuzzy_rejection_gets_validated_bounded_reread() { let patch = one_update_patch(); let result = apply_patch_agent_stdout_result( - &strict_rejection_payload("trim", 1, 20).to_string(), + &matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique, + "trim", + &[20], + 1, + 100, + ) + .to_string(), &patch, false, - true, + crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique, ); assert!(!result.success); assert_eq!(result.output["execution_state"], "not_started"); assert_eq!(result.output["state_changed"], false); assert_eq!( - result.output["strict_match_diagnostic"]["classification"], + result.output["match_rejection_diagnostic"]["classification"], "unique_fuzzy_candidate" ); assert_eq!( - result.output["strict_match_diagnostic"]["match_mode"], + result.output["match_rejection_diagnostic"]["match_mode"], "trim" ); assert_eq!( - result.output["strict_match_diagnostic"]["match_source"], + result.output["match_rejection_diagnostic"]["match_source"], "old_lines" ); assert_eq!( - result.output["strict_match_diagnostic"]["matched_start_line"], + result.output["match_rejection_diagnostic"]["matched_start_line"], 20 ); assert_eq!( - result.output["strict_match_diagnostic"]["candidate_count"], + result.output["match_rejection_diagnostic"]["candidate_count"], 1 ); assert_eq!( - result.output["strict_match_diagnostic"]["expected_line_count"], + result.output["match_rejection_diagnostic"]["candidate_start_lines"], + json!([20]) + ); + assert_eq!( + result.output["match_rejection_diagnostic"]["expected_line_count"], 1 ); assert_eq!( - result.output["strict_match_diagnostic"]["strict_match"], + result.output["match_rejection_diagnostic"]["matching_mode_satisfied"], false ); assert_eq!(result.output["recovery"]["action"], "read_files"); assert_eq!( result.output["recovery"]["reason"], - "strict_match_rejected_unique_fuzzy" + "matching_mode_rejected_unique_fuzzy" ); assert_eq!(result.output["recovery"]["items"][0]["path"], "file.txt"); assert_eq!(result.output["recovery"]["items"][0]["start_line"], 12); assert_eq!(result.output["recovery"]["items"][0]["limit"], 17); assert_eq!( result.output["recovery_action"], - "reread_and_regenerate_strict_patch" + "reread_and_regenerate_exact_unique_patch" ); assert!(result.output["retry_guidance"] .as_str() .unwrap() - .contains("strict_matching=true")); - assert!(!result.output["retry_guidance"] - .as_str() - .unwrap() - .contains("strict_matching=false")); + .contains("matching_mode=exact_unique")); for raw_runner_field in [ "chunk_index", "match_mode", "match_source", "matched_start_line", "candidate_count", - "strict_match", + "matching_mode_satisfied", ] { assert!(result.output.get(raw_runner_field).is_none()); } @@ -2963,103 +3099,238 @@ mod tests { &serde_json::to_value(&result).unwrap(), &schema, ) - .unwrap_or_else(|error| panic!("strict recovery must match output schema: {error}")); + .unwrap_or_else(|error| panic!("matching recovery must match output schema: {error}")); } #[test] - fn apply_patch_strict_ambiguous_rejection_never_selects_runner_target() { + fn apply_patch_unique_ambiguous_rejection_returns_equal_candidate_windows() { let patch = one_update_patch(); let result = apply_patch_agent_stdout_result( - &strict_rejection_payload("exact", 2, 20).to_string(), + &matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[20, 60], + 2, + 100, + ) + .to_string(), &patch, false, - true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); assert!(!result.success); assert_eq!( - result.output["strict_match_diagnostic"]["classification"], + result.output["match_rejection_diagnostic"]["classification"], "ambiguous_candidate" ); assert_eq!( - result.output["strict_match_diagnostic"]["candidate_count"], + result.output["match_rejection_diagnostic"]["candidate_count"], 2 ); - assert!(result.output["strict_match_diagnostic"]["matched_start_line"].is_null()); - assert!(result.output.get("recovery").is_none()); - assert_eq!(result.output["recovery_action"], "add_exact_unique_context"); - assert!(!result.output["error"].as_str().unwrap().contains("20")); + assert_eq!( + result.output["match_rejection_diagnostic"]["candidate_start_lines"], + json!([20, 60]) + ); + assert!(result.output["match_rejection_diagnostic"]["matched_start_line"].is_null()); + assert_eq!( + result.output["recovery"]["items"].as_array().unwrap().len(), + 2 + ); + assert_eq!(result.output["recovery"]["items"][0]["path"], "file.txt"); + assert_eq!(result.output["recovery"]["items"][1]["path"], "file.txt"); + assert_eq!( + result.output["recovery_action"], + "read_equal_candidates_and_refine_context" + ); + assert!(result.output["retry_guidance"] + .as_str() + .unwrap() + .contains("equal candidate")); assert!(!result.output["retry_guidance"] .as_str() .unwrap() - .contains("strict_matching=false")); + .contains("preferred")); } #[test] - fn apply_patch_strict_ambiguous_context_fact_is_validated_against_chunk_shape() { + fn apply_patch_unique_large_ambiguity_returns_four_truncated_equal_windows() { + let patch = one_update_patch(); + let result = apply_patch_agent_stdout_result( + &matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[10, 20, 30, 40], + 5, + 100, + ) + .to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); + + assert!(!result.success); + let diagnostic = &result.output["match_rejection_diagnostic"]; + assert_eq!(diagnostic["classification"], "ambiguous_candidate"); + assert_eq!(diagnostic["candidate_count"], 5); + assert_eq!(diagnostic["candidate_start_lines"], json!([10, 20, 30, 40])); + assert_eq!(diagnostic["candidate_positions_truncated"], true); + assert_eq!( + result.output["recovery"]["items"].as_array().unwrap().len(), + 4 + ); + assert!(diagnostic.get("winner").is_none()); + assert!(diagnostic.get("preferred").is_none()); + assert!(result.output["recovery"].get("winner").is_none()); + assert!(result.output["recovery"].get("preferred").is_none()); + } + + #[test] + fn apply_patch_unique_eof_rejection_cannot_expand_effective_search_range() { let patch = crate::apply_patch_shared::parse_codex_patch( - "*** Begin Patch\n*** Update File: file.txt\n@@ ctx\n-old\n+new\n*** End Patch", + "*** Begin Patch\n*** Update File: file.txt\n-old\n+new\n*** End of File\n*** End Patch", + ) + .unwrap(); + let mut payload = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[1, 3], + 2, + 3, + ); + payload["search_start_line"] = json!(1); + + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); + assert!(!result.success); + assert!(result.output.get("match_rejection_diagnostic").is_none()); + assert!(result.output.get("recovery").is_none()); + } + + #[test] + fn apply_patch_unique_ambiguous_context_fact_is_valid_for_pure_addition() { + let patch = crate::apply_patch_shared::parse_codex_patch( + "*** Begin Patch\n*** Update File: file.txt\n@@ ctx\n+new\n*** End Patch", ) .unwrap(); - let mut payload = strict_rejection_payload("exact", 2, 1); + let mut payload = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[1, 3], + 2, + 4, + ); payload["match_source"] = json!("change_context"); - payload["source_line_count"] = json!(4); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, false, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!( - result.output["strict_match_diagnostic"]["classification"], + result.output["match_rejection_diagnostic"]["classification"], "ambiguous_candidate" ); assert_eq!( - result.output["strict_match_diagnostic"]["match_source"], + result.output["match_rejection_diagnostic"]["match_source"], "change_context" ); - assert!(result.output["strict_match_diagnostic"]["matched_start_line"].is_null()); + assert!(result.output["match_rejection_diagnostic"]["matched_start_line"].is_null()); + assert_eq!( + result.output["recovery"]["items"].as_array().unwrap().len(), + 2 + ); + } + + #[test] + fn apply_patch_unique_rejects_context_only_ambiguity_for_replacement_chunk() { + let patch = crate::apply_patch_shared::parse_codex_patch( + "*** Begin Patch\n*** Update File: file.txt\n@@ ctx\n-old\n+new\n*** End Patch", + ) + .unwrap(); + let mut payload = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[1, 3], + 2, + 4, + ); + payload["match_source"] = json!("change_context"); + + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); + assert!(!result.success); + assert!(result.output.get("match_rejection_diagnostic").is_none()); assert!(result.output.get("recovery").is_none()); + assert!(result.output.get("path").is_none()); + assert!(result.output.get("change_index").is_none()); } #[test] - fn apply_patch_strict_recovery_suppresses_spoofed_or_contradictory_metadata() { + fn apply_patch_matching_recovery_suppresses_spoofed_or_contradictory_metadata() { let patch = one_update_patch(); let mut cases = Vec::new(); - let mut wrong_path = strict_rejection_payload("trim", 1, 20); + let base = || { + matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[20, 60], + 2, + 100, + ) + }; + let mut wrong_path = base(); wrong_path["path"] = json!("other.txt"); - cases.push((wrong_path, true)); - let mut wrong_change = strict_rejection_payload("trim", 1, 20); + cases.push(wrong_path); + let mut wrong_change = base(); wrong_change["change_index"] = json!(1); - cases.push((wrong_change, true)); - let mut wrong_chunk = strict_rejection_payload("trim", 1, 20); + cases.push(wrong_change); + let mut wrong_chunk = base(); wrong_chunk["chunk_index"] = json!(1); - cases.push((wrong_chunk, true)); - let mut wrong_source = strict_rejection_payload("trim", 1, 20); + cases.push(wrong_chunk); + let mut wrong_source = base(); wrong_source["match_source"] = json!("change_context"); - cases.push((wrong_source, true)); - cases.push((strict_rejection_payload("exact", 1, 20), true)); - let mut claimed_strict = strict_rejection_payload("trim", 1, 20); - claimed_strict["strict_match"] = json!(true); - cases.push((claimed_strict, true)); - let mut out_of_range_line = strict_rejection_payload("trim", 1, 101); - out_of_range_line["source_line_count"] = json!(100); - cases.push((out_of_range_line, true)); - let mut impossible_candidates = strict_rejection_payload("trim", 101, 20); - impossible_candidates["source_line_count"] = json!(100); - cases.push((impossible_candidates, true)); - let mut before_search_start = strict_rejection_payload("trim", 1, 20); - before_search_start["search_start_line"] = json!(21); - cases.push((before_search_start, true)); - cases.push((strict_rejection_payload("trim", 1, 20), false)); - - for (payload, strict_request) in cases { + cases.push(wrong_source); + let mut equal_positions = base(); + equal_positions["candidate_start_lines"] = json!([20, 20]); + cases.push(equal_positions); + let mut out_of_range = base(); + out_of_range["candidate_start_lines"] = json!([20, 101]); + cases.push(out_of_range); + let mut bad_truncation = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[10, 20, 30, 40], + 5, + 100, + ); + bad_truncation["candidate_positions_truncated"] = json!(false); + cases.push(bad_truncation); + let mut wrong_mode = base(); + wrong_mode["requested_matching_mode"] = json!("exact_unique"); + cases.push(wrong_mode); + + for payload in cases { let result = apply_patch_agent_stdout_result( &payload.to_string(), &patch, false, - strict_request, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, ); assert!(!result.success); - assert!(result.output.get("strict_match_diagnostic").is_none()); + assert!(result.output.get("match_rejection_diagnostic").is_none()); assert!(result.output.get("recovery").is_none()); assert!(result.output.get("path").is_none()); assert!(result.output.get("change_index").is_none()); @@ -3067,41 +3338,63 @@ mod tests { } #[test] - fn apply_patch_strict_recovery_is_suppressed_for_outcome_unknown() { + fn apply_patch_matching_recovery_is_suppressed_for_outcome_unknown() { let patch = one_update_patch(); - let mut payload = strict_rejection_payload("trim", 1, 20); + let mut payload = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[20, 60], + 2, + 100, + ); payload["changed"] = json!(true); payload["state_changed"] = json!(true); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, false, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["execution_state"], "outcome_unknown"); assert_eq!( result.output["recovery_action"], "inspect_workspace_before_retry" ); - assert!(result.output.get("strict_match_diagnostic").is_none()); + assert!(result.output.get("match_rejection_diagnostic").is_none()); assert!(result.output.get("recovery").is_none()); } #[test] - fn apply_patch_strict_recovery_never_leaks_source_or_patch_bodies() { + fn apply_patch_matching_recovery_never_leaks_source_or_patch_bodies() { let patch = crate::apply_patch_shared::parse_codex_patch( "*** Begin Patch\n*** Update File: file.txt\n-PATCH_PRIVATE_TOKEN\n+new\n*** End Patch", ) .unwrap(); - let mut payload = strict_rejection_payload("trim_end", 1, 4); + let mut payload = matching_rejection_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + &[4, 8], + 2, + 20, + ); payload["error"] = json!("SOURCE_PRIVATE_TOKEN"); payload["future_body_field"] = json!("SOURCE_PRIVATE_TOKEN"); payload["recovery"] = json!({ "action": "read_files", "items": [{"path": "SOURCE_PRIVATE_TOKEN", "start_line": 1, "limit": 999999}] }); - payload["strict_match_diagnostic"] = json!({"source": "SOURCE_PRIVATE_TOKEN"}); + payload["match_rejection_diagnostic"] = json!({"source": "SOURCE_PRIVATE_TOKEN"}); payload["recovery_kind"] = json!("reobserve"); payload["recovery_tool"] = json!("list_jobs"); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, false, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + false, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); let serialized = serde_json::to_string(&result).unwrap(); assert!(!serialized.contains("SOURCE_PRIVATE_TOKEN")); assert!(!serialized.contains("PATCH_PRIVATE_TOKEN")); @@ -3111,18 +3404,56 @@ mod tests { } #[test] - fn apply_patch_success_metadata_accepts_strict_exact_and_non_strict_fuzzy() { + fn apply_patch_success_metadata_is_bound_to_requested_matching_mode() { let patch = one_update_patch(); - for (mode, count, strict_match, strict_request) in [ - ("exact", 1, true, true), - ("trim", 1, false, false), - ("exact", 2, false, false), + for (matching_mode, mode, count, unique_match, strict_match) in [ + ( + crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique, + "exact", + 1, + true, + true, + ), + ( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "trim_end", + 1, + true, + false, + ), + ( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "trim", + 1, + true, + false, + ), + ( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "normalized", + 1, + true, + false, + ), + ( + crate::apply_patch_shared::ApplyPatchMatchingMode::FirstMatch, + "exact", + 2, + false, + false, + ), ] { - let payload = apply_patch_success_payload(mode, count, strict_match).to_string(); - let result = apply_patch_agent_stdout_result(&payload, &patch, true, strict_request); + let payload = + apply_patch_success_payload(matching_mode, mode, count, unique_match, strict_match) + .to_string(); + let result = apply_patch_agent_stdout_result(&payload, &patch, true, matching_mode); assert!(result.success, "{:?}", result.error); assert_eq!(result.output["execution_state"], "completed"); assert_eq!(result.output["files"][0]["edits"][0]["match_mode"], mode); + assert_eq!( + result.output["requested_matching_mode"], + matching_mode.as_str() + ); } } @@ -3131,26 +3462,65 @@ mod tests { let patch = one_update_patch(); let mut cases = Vec::new(); - let mut strict_violation = apply_patch_success_payload("exact", 1, false); - cases.push(strict_violation.take()); + let mut unique_violation = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + 2, + false, + false, + ); + cases.push(unique_violation.take()); - let mut wrong_source = apply_patch_success_payload("exact", 1, true); + let mut wrong_source = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + 1, + true, + true, + ); wrong_source["files"][0]["edits"][0]["match_source"] = json!("append"); - cases.push(wrong_source); + cases.push(wrong_source.clone()); - let mut wrong_chunk = apply_patch_success_payload("exact", 1, true); + let mut wrong_chunk = wrong_source; + wrong_chunk["files"][0]["edits"][0]["match_source"] = json!("old_lines"); wrong_chunk["files"][0]["edits"][0]["chunk_index"] = json!(1); cases.push(wrong_chunk); - let mut wrong_path = apply_patch_success_payload("exact", 1, true); + let mut wrong_path = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + 1, + true, + true, + ); wrong_path["files"][0]["path"] = json!("other.txt"); cases.push(wrong_path); - let contradictory_strict = apply_patch_success_payload("trim", 1, true); + let contradictory_strict = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "trim", + 1, + true, + true, + ); cases.push(contradictory_strict); + let wrong_requested_mode = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::ExactUnique, + "exact", + 1, + true, + true, + ); + cases.push(wrong_requested_mode); + for payload in cases { - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, true, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["execution_state"], "outcome_unknown"); assert!(result.output["state_changed"].is_null()); @@ -3170,13 +3540,24 @@ mod tests { #[test] fn apply_patch_success_metadata_strips_unknown_fields_without_losing_known_result() { let patch = one_update_patch(); - let mut payload = apply_patch_success_payload("exact", 1, true); + let mut payload = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + 1, + true, + true, + ); payload["future_top_level"] = json!("NEVER_SURVIVE_PATCH_METADATA"); payload["files"][0]["future_file_field"] = json!("NEVER_SURVIVE_PATCH_METADATA"); payload["files"][0]["edits"][0]["future_edit_field"] = json!("NEVER_SURVIVE_PATCH_METADATA"); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, true, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(result.success, "{:?}", result.error); assert_eq!(result.output["execution_state"], "completed"); let serialized = serde_json::to_string(&result.output).unwrap(); @@ -3194,6 +3575,7 @@ mod tests { .unwrap(); let payload = json!({ "dry_run": true, + "requested_matching_mode": "unique", "applied_count": 4, "changed": false, "state_changed": false, @@ -3241,6 +3623,7 @@ mod tests { "match_source": "old_lines", "matched_start_line": 1, "candidate_count": 1, + "unique_match": true, "strict_match": true }] }, @@ -3263,6 +3646,7 @@ mod tests { "match_source": "append", "matched_start_line": 2, "candidate_count": null, + "unique_match": true, "strict_match": true }] } @@ -3270,7 +3654,12 @@ mod tests { "changed_paths": ["new.txt", "old.txt", "move.txt", "moved.txt", "append.txt"] }); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, true, true); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(result.success, "{:?}", result.error); assert_eq!(result.output["files"].as_array().unwrap().len(), 4); assert_eq!(result.output["changed_paths"].as_array().unwrap().len(), 5); @@ -3279,13 +3668,24 @@ mod tests { #[test] fn apply_patch_missing_current_match_metadata_is_outcome_unknown() { let patch = one_update_patch(); - let mut payload = apply_patch_success_payload("exact", 1, true); + let mut payload = apply_patch_success_payload( + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + "exact", + 1, + true, + true, + ); payload["files"][0]["edits"][0] .as_object_mut() .expect("current edit object") - .remove("strict_match"); + .remove("unique_match"); - let result = apply_patch_agent_stdout_result(&payload.to_string(), &patch, true, false); + let result = apply_patch_agent_stdout_result( + &payload.to_string(), + &patch, + true, + crate::apply_patch_shared::ApplyPatchMatchingMode::Unique, + ); assert!(!result.success); assert_eq!(result.output["execution_state"], "outcome_unknown"); assert!(result.output.get("files").is_none()); diff --git a/src/tool_runtime/patch_tools.rs b/src/tool_runtime/patch_tools.rs index 670cac01..df95c70b 100644 --- a/src/tool_runtime/patch_tools.rs +++ b/src/tool_runtime/patch_tools.rs @@ -9,10 +9,10 @@ impl ToolRuntime { project, patch, dry_run, - strict_matching, + matching_mode, session_id: _, } => { - self.apply_patch(project, patch, dry_run, strict_matching) + self.apply_patch(project, patch, dry_run, matching_mode) .await } ToolCall::ApplyUnifiedDiff { diff --git a/src/tool_runtime/tests/files.rs b/src/tool_runtime/tests/files.rs index 85eb85b3..7ae464f5 100644 --- a/src/tool_runtime/tests/files.rs +++ b/src/tool_runtime/tests/files.rs @@ -745,14 +745,14 @@ fn conversation_import_session_log_arguments_do_not_store_host_file_refs() { } #[test] -fn apply_patch_audit_records_strict_flags_without_patch_body() { +fn apply_patch_audit_records_matching_mode_without_patch_body() { let private_patch = "*** Begin Patch\n*** Add File: NEVER_LOG_PATCH_BODY.txt\n+secret\n*** End Patch"; let arguments = serde_json::json!({ "project": "agent:test:demo", "patch": private_patch, "dry_run": true, - "strict_matching": true, + "matching_mode": "exact_unique", }); let raw_summary = @@ -760,7 +760,7 @@ fn apply_patch_audit_records_strict_flags_without_patch_body() { assert_eq!(raw_summary["project"], "agent:test:demo"); assert_eq!(raw_summary["patch_present"], true); assert_eq!(raw_summary["dry_run"], true); - assert_eq!(raw_summary["strict_matching"], true); + assert_eq!(raw_summary["matching_mode"], "exact_unique"); assert!(!serde_json::to_string(&raw_summary) .unwrap() .contains("NEVER_LOG_PATCH_BODY")); @@ -770,7 +770,7 @@ fn apply_patch_audit_records_strict_flags_without_patch_body() { assert_eq!(typed_summary["project"], "agent:test:demo"); assert_eq!(typed_summary["patch_present"], true); assert_eq!(typed_summary["dry_run"], true); - assert_eq!(typed_summary["strict_matching"], true); + assert_eq!(typed_summary["matching_mode"], "exact_unique"); assert!(!serde_json::to_string(&typed_summary) .unwrap() .contains("NEVER_LOG_PATCH_BODY")); diff --git a/src/tool_runtime/tests/metadata.rs b/src/tool_runtime/tests/metadata.rs index 410ebfad..12975dd1 100644 --- a/src/tool_runtime/tests/metadata.rs +++ b/src/tool_runtime/tests/metadata.rs @@ -357,6 +357,7 @@ async fn register_agent_projects_for_auth( apply_text_edit_line_scope: false, apply_patch: false, apply_patch_match_metadata: false, + apply_patch_matching_mode: false, apply_patch_strict_matching: false, git: true, jobs: true, diff --git a/src/tool_runtime/tests/schema/spot_checks.rs b/src/tool_runtime/tests/schema/spot_checks.rs index b22732e8..f163da17 100644 --- a/src/tool_runtime/tests/schema/spot_checks.rs +++ b/src/tool_runtime/tests/schema/spot_checks.rs @@ -304,7 +304,7 @@ fn tool_specs_schema_spot_checks() { ( "apply_patch", vec!["project", "patch"], - vec!["dry_run", "strict_matching", "session_id"], + vec!["dry_run", "matching_mode", "session_id"], ), ( "apply_unified_diff",