Implement Double-Lock safety harness for weaverd with atomic commits#34
Implement Double-Lock safety harness for weaverd with atomic commits#34
Conversation
…ctional edits Introduce a comprehensive safety harness in `weaverd` that wraps all `act` commands with a two-phase verification process before committing file changes. The Double-Lock harness enforces: 1. Syntactic Lock: Parses modified files to ensure valid syntax trees, catching structural errors early. 2. Semantic Lock: Uses a configured language server to verify no new errors or high-severity warnings are introduced. Partial changes are prohibited; only fully verified edits are atomically applied to disk. The harness operates on in-memory file buffers to maintain filesystem integrity. This includes core types (`FileEdit`, `TextEdit`), verification context, transaction management, structured error types, placeholder lock implementations for syntactic and semantic phases, configurable test doubles, and comprehensive behavioral tests with BDD feature specifications. Documentation and design docs are updated to reflect the new safety harness feature, marking progress in the project roadmap. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughExpose a new public module Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant T as EditTransaction
participant FS as FileSystem
participant VC as VerificationContext
participant SY as SyntacticLock
participant SE as SemanticLock
U->>T: add_edit(...) / execute()
T->>FS: read_file(path) for each FileEdit
FS-->>T: original content
T->>VC: build context + apply_edits() (in‑memory)
VC-->>T: modified contents
T->>SY: validate(&context)
alt syntactic fails
SY-->>T: Failed(failures)
T-->>U: TransactionOutcome::SyntacticLockFailed
else syntactic passes
SY-->>T: Passed
T->>SE: validate(&context)
alt semantic fails or unavailable
SE-->>T: Failed(failures) or Err(SemanticBackendUnavailable)
T-->>U: TransactionOutcome::SemanticLockFailed or Err
else semantic passes
SE-->>T: Passed
T->>FS: prepare temp files -> atomic rename for each modified file
FS-->>T: commit success
T-->>U: TransactionOutcome::Committed(files_modified)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideIntroduces a new public safety_harness module in weaverd that models in-memory text/file edits, runs pluggable syntactic and semantic locks in a Double-Lock flow, and atomically commits successful multi-file transactions, with comprehensive error types, tests, and documentation updates. Sequence diagram for Double-Lock EditTransaction executionsequenceDiagram
actor Agent
participant CLI as weaver_CLI
participant Daemon as weaverd
participant Harness as EditTransaction
participant SynLock as SyntacticLock
participant SemLock as SemanticLock
participant FS as FileSystem
Agent->>CLI: issue_act_command
CLI->>Daemon: send_edit_request
Daemon->>Harness: create_EditTransaction
Daemon->>Harness: add_edits(FileEdit)
Daemon->>Harness: execute()
alt no_file_edits
Harness-->>Daemon: TransactionOutcome_NoChanges
Daemon-->>CLI: respond_no_changes
CLI-->>Agent: report_no_changes
else has_file_edits
loop for_each_FileEdit
Harness->>FS: read_file(path)
FS-->>Harness: original_content_or_empty
Harness->>Harness: apply_edits(original, FileEdit)
Harness->>Harness: update_VerificationContext
end
Harness->>SynLock: validate(VerificationContext)
alt syntactic_lock_failed
SynLock-->>Harness: SyntacticLockResult_Failed
Harness-->>Daemon: TransactionOutcome_SyntacticLockFailed
Daemon-->>CLI: report_syntactic_failure
CLI-->>Agent: display_diagnostics
else syntactic_lock_passed
SynLock-->>Harness: SyntacticLockResult_Passed
Harness->>SemLock: validate(VerificationContext)
alt semantic_backend_unavailable
SemLock-->>Harness: SafetyHarnessError_SemanticBackendUnavailable
Harness-->>Daemon: Err(SafetyHarnessError)
Daemon-->>CLI: report_backend_unavailable
CLI-->>Agent: display_backend_error
else semantic_lock_failed
SemLock-->>Harness: Ok(SemanticLockResult_Failed)
Harness-->>Daemon: TransactionOutcome_SemanticLockFailed
Daemon-->>CLI: report_semantic_failure
CLI-->>Agent: display_diagnostics
else both_locks_pass
SemLock-->>Harness: Ok(SemanticLockResult_Passed)
loop for_each_modified_path
Harness->>FS: write_file_atomic(path, content)
FS-->>Harness: ok
end
Harness-->>Daemon: TransactionOutcome_Committed(files_modified)
Daemon-->>CLI: report_success(files_modified)
CLI-->>Agent: confirm_commit
end
end
end
Class diagram for safety_harness core edit and verification typesclassDiagram
class Position {
+u32 line
+u32 column
+new(line u32, column u32) Position
}
class TextRange {
+Position start
+Position end
+new(start Position, end Position) TextRange
+point(position Position) TextRange
}
class ReplacementText {
-String value
+new(text String) ReplacementText
+empty() ReplacementText
+as_str() &str
+into_inner() String
}
class TextEdit {
-TextRange range
-String new_text
+with_replacement(range TextRange, text ReplacementText) TextEdit
+new(range TextRange, new_text String) TextEdit
+from_positions_typed(start Position, end Position, text ReplacementText) TextEdit
+from_positions(start Position, end Position, new_text String) TextEdit
+from_coords(start_line u32, start_column u32, end_line u32, end_column u32, new_text String) TextEdit
+insert_at(position Position, text ReplacementText) TextEdit
+insert(line u32, column u32, text String) TextEdit
+delete_range(start Position, end Position) TextEdit
+delete(start_line u32, start_column u32, end_line u32, end_column u32) TextEdit
+start_line() u32
+start_column() u32
+end_line() u32
+end_column() u32
+new_text() &str
}
class FileEdit {
-PathBuf path
-Vec~TextEdit~ edits
+new(path PathBuf) FileEdit
+add_edit(edit TextEdit) void
+with_edits(path PathBuf, edits Vec~TextEdit~) FileEdit
+path() &PathBuf
+edits() &Vec~TextEdit~
+is_empty() bool
}
Position --> TextRange : used_by
TextRange --> TextEdit : used_by
ReplacementText --> TextEdit : used_by
TextEdit --> FileEdit : part_of
class VerificationContext {
-HashMap~PathBuf, String~ original_content
-HashMap~PathBuf, String~ modified_content
+new() VerificationContext
+add_original(path PathBuf, content String) void
+add_modified(path PathBuf, content String) void
+original(path &PathBuf) Option~&String~
+modified(path &PathBuf) Option~&String~
+modified_paths() Iterator
+modified_files() Iterator
+modified_count() usize
+is_empty() bool
}
class SyntacticLock {
<<interface>>
+validate(context &VerificationContext) SyntacticLockResult
}
class SemanticLock {
<<interface>>
+validate(context &VerificationContext) Result~SemanticLockResult, SafetyHarnessError~
}
class PlaceholderSyntacticLock {
+validate(context &VerificationContext) SyntacticLockResult
}
class PlaceholderSemanticLock {
+validate(context &VerificationContext) Result~SemanticLockResult, SafetyHarnessError~
}
class ConfigurableSyntacticLock {
-Vec~VerificationFailure~ failures
+passing() ConfigurableSyntacticLock
+failing(failures Vec~VerificationFailure~) ConfigurableSyntacticLock
+validate(context &VerificationContext) SyntacticLockResult
}
class ConfigurableSemanticLock {
-Vec~VerificationFailure~ failures
-Option~String~ error
+passing() ConfigurableSemanticLock
+failing(failures Vec~VerificationFailure~) ConfigurableSemanticLock
+unavailable(message String) ConfigurableSemanticLock
+validate(context &VerificationContext) Result~SemanticLockResult, SafetyHarnessError~
}
SyntacticLock <|.. PlaceholderSyntacticLock
SyntacticLock <|.. ConfigurableSyntacticLock
SemanticLock <|.. PlaceholderSemanticLock
SemanticLock <|.. ConfigurableSemanticLock
class SyntacticLockResult {
<<enum>>
+Passed
+Failed(failures Vec~VerificationFailure~)
+passed() bool
+failures() Option~&[VerificationFailure]~
}
class SemanticLockResult {
<<enum>>
+Passed
+Failed(failures Vec~VerificationFailure~)
+passed() bool
+failures() Option~&[VerificationFailure]~
}
class LockPhase {
<<enum>>
+Syntactic
+Semantic
}
class VerificationFailure {
-PathBuf file
-Option~u32~ line
-Option~u32~ column
-String message
+new(file PathBuf, message String) VerificationFailure
+at_location(line u32, column u32) VerificationFailure
+file() &PathBuf
+line() Option~u32~
+column() Option~u32~
+message() &str
}
class SafetyHarnessError {
<<enum>>
+VerificationFailed(phase LockPhase, count usize, failures Vec~VerificationFailure~)
+FileReadError(path PathBuf, message String)
+FileWriteError(path PathBuf, message String)
+EditApplicationError(path PathBuf, message String)
+SemanticBackendUnavailable(message String)
+SyntacticBackendUnavailable(message String)
+syntactic_failed(failures Vec~VerificationFailure~) SafetyHarnessError
+semantic_failed(failures Vec~VerificationFailure~) SafetyHarnessError
+file_read(path PathBuf, error std_io_Error) SafetyHarnessError
+file_write(path PathBuf, error std_io_Error) SafetyHarnessError
+failures() Option~&[VerificationFailure]~
+lock_phase() Option~LockPhase~
}
VerificationFailure --> SyntacticLockResult : used_in
VerificationFailure --> SemanticLockResult : used_in
VerificationFailure --> SafetyHarnessError : used_in
LockPhase --> SafetyHarnessError : used_in
class TransactionOutcome {
<<enum>>
+Committed(files_modified usize)
+SyntacticLockFailed(failures Vec~VerificationFailure~)
+SemanticLockFailed(failures Vec~VerificationFailure~)
+NoChanges
+committed() bool
+files_modified() Option~usize~
}
class EditTransaction {
-Vec~FileEdit~ file_edits
-&SyntacticLock syntactic_lock
-&SemanticLock semantic_lock
+new(syntactic_lock &SyntacticLock, semantic_lock &SemanticLock) EditTransaction
+add_edit(edit FileEdit) void
+add_edits(edits IntoIterator~Item=FileEdit~) void
+execute() Result~TransactionOutcome, SafetyHarnessError~
}
EditTransaction --> FileEdit : aggregates
EditTransaction --> VerificationContext : builds
EditTransaction --> SyntacticLock : uses
EditTransaction --> SemanticLock : uses
EditTransaction --> TransactionOutcome : returns
EditTransaction --> SafetyHarnessError : returns
TransactionOutcome --> VerificationFailure : contains
class safety_harness_mod {
+FileEdit
+Position
+ReplacementText
+TextEdit
+TextRange
+LockPhase
+SafetyHarnessError
+VerificationFailure
+SemanticLockResult
+SyntacticLockResult
+EditTransaction
+TransactionOutcome
+ConfigurableSemanticLock
+ConfigurableSyntacticLock
+PlaceholderSemanticLock
+PlaceholderSyntacticLock
+SemanticLock
+SyntacticLock
+VerificationContext
}
safety_harness_mod ..> EditTransaction
safety_harness_mod ..> FileEdit
safety_harness_mod ..> VerificationContext
safety_harness_mod ..> SafetyHarnessError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add Double-Lock safety harness with atomic commit coordinator to
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Bumpy Road Aheadcrates/weaverd/src/safety_harness/verification.rs: line_column_to_offset What lead to degradation?line_column_to_offset has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function Why does this problem occur?A Bumpy Road is a function that contains multiple chunks of nested conditional logic inside the same function. The deeper the nesting and the more bumps, the lower the code health. How to fix it?Bumpy Road implementations indicate a lack of encapsulation. Check out the detailed description of the Bumpy Road code health issue. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13183 @leynos, I've analyzed the Suggested FixReplace the current /// Converts a line and column pair to a byte offset in the original text.
fn line_column_to_offset(lines: &[&str], line: u32, column: u32) -> Option<usize> {
let line_idx = line as usize;
if line_idx > lines.len() {
return None;
}
let line_start_offset = calculate_line_start_offset(lines, line_idx)?;
add_validated_column_offset(lines, line_idx, column, line_start_offset)
}
/// Calculates the byte offset to the start of the specified line.
fn calculate_line_start_offset(lines: &[&str], target_line_idx: usize) -> Option<usize> {
let mut offset: usize = 0;
for (idx, &line_content) in lines.iter().enumerate() {
if idx == target_line_idx {
break;
}
offset = offset.checked_add(line_content.len())?;
offset = offset.checked_add(1)?; // newline character
}
Some(offset)
}
/// Validates the column offset and adds it to the line start offset.
fn add_validated_column_offset(
lines: &[&str],
line_idx: usize,
column: u32,
line_start_offset: usize,
) -> Option<usize> {
let col_offset = column as usize;
if line_idx < lines.len() {
let line_content = lines.get(line_idx)?;
if col_offset > line_content.len() {
return None;
}
}
line_start_offset.checked_add(col_offset)
}AI Coding Agent PromptThis refactoring improves code health by:
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/tests/safety_harness_behaviour.rs Comment on file //! Behavioural tests for the Double-Lock safety harness.
❌ New issue: String Heavy Function Arguments Use NewTypes to model domain values and eliminate "integer soup". Reach for |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function Argumentscrates/weaverd/src/safety_harness/edit.rs: TextEdit.from_coords What lead to degradation?TextEdit.from_coords has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10684 @leynos, I've analysed the test module. The string-heavy arguments stem from five distinct semantic domains being represented as primitive strings: file names, file content, text patterns for edits, expected content for assertions, and diagnostic messages. Introduce NewTypes to eliminate this "string soup" and make the domain model explicit. Suggested FixCreate a new module //! Domain types for safety harness behavioural tests.
use std::ops::Deref;
use std::path::{Path, PathBuf};
/// A file name used in test scenarios.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct FileName(String);
impl FileName {
pub fn as_str(&self) -> &str {
&self.0
}
pub fn to_path(&self, base: &Path) -> PathBuf {
base.join(&self.0)
}
}
impl From<String> for FileName {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for FileName {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for FileName {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for FileName {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
/// Content of a file in test scenarios.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct FileContent(String);
impl FileContent {
pub fn as_str(&self) -> &str {
&self.0
}
pub fn as_bytes(&self) -> &[u8] {
self.0.as_bytes()
}
}
impl From<String> for FileContent {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for FileContent {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for FileContent {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for FileContent {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
/// A text pattern for search, replace, or assertion operations.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TextPattern(String);
impl TextPattern {
pub fn as_str(&self) -> &str {
&self.0
}
}
impl From<String> for TextPattern {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for TextPattern {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for TextPattern {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for TextPattern {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
/// A diagnostic message for lock configuration.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DiagnosticMessage(String);
impl DiagnosticMessage {
pub fn as_str(&self) -> &str {
&self.0
}
}
impl From<String> for DiagnosticMessage {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for DiagnosticMessage {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for DiagnosticMessage {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for DiagnosticMessage {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}Then update the method signatures in fn create_file(&mut self, name: &FileName, content: &FileContent) { /* ... */ }
fn file_path(&self, name: &FileName) -> PathBuf { /* ... */ }
fn read_file(&self, name: &FileName) -> String { /* ... */ }
fn add_replacement_edit(&mut self, name: &FileName, old: &TextPattern, new: &TextPattern) { /* ... */ }
fn add_creation_edit(&mut self, name: &FileName, content: &FileContent) { /* ... */ }And update all BDD step functions (e.g.): #[given("a source file {name} with content {content}")]
fn given_source_file(world: &RefCell<SafetyHarnessWorld>, name: FileName, content: FileContent) {
world.borrow_mut().create_file(&name, &content);
}
#[given("a syntactic lock that fails with {message}")]
fn given_syntactic_fails(world: &RefCell<SafetyHarnessWorld>, message: DiagnosticMessage) {
let failure = VerificationFailure::new(PathBuf::from("test"), message.as_str());
world.borrow_mut().syntactic_lock = ConfigurableSyntacticLock::failing(vec![failure]);
}
#[when("an edit replaces {old} with {new}")]
fn when_edit_replaces(world: &RefCell<SafetyHarnessWorld>, old: TextPattern, new: TextPattern) {
world.borrow_mut().add_replacement_edit(&"test.txt".into(), &old, &new);
world.borrow_mut().execute_transaction();
}AI Coding Agent PromptThis refactoring achieves:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6361 @leynos, I've analysed the Suggested FixIntroduce a /// Builds a text edit from explicit positions.
///
/// This convenience constructor accepts Position values directly when callers
/// do not want to create an intermediate [`TextRange`] value.
#[must_use]
pub const fn from_positions(start: Position, end: Position, new_text: String) -> Self {
Self::new(TextRange::new(start, end), new_text)
}
/// Builds a text edit from explicit positions.
///
/// This convenience constructor accepts position coordinates directly when
/// callers do not want to create intermediate [`Position`] and [`TextRange`]
/// values. The argument count is intentionally above the clippy threshold to
/// match LSP conventions.
///
/// # Deprecated
/// Use [`Self::from_positions`] instead to reduce argument count.
#[must_use]
#[deprecated(since = "0.2.0", note = "use `from_positions` instead")]
#[allow(clippy::too_many_arguments, reason = "matches LSP coordinate convention")]
pub const fn from_coords(
start_line: u32,
start_column: u32,
end_line: u32,
end_column: u32,
new_text: String,
) -> Self {
Self::from_positions(
Position::new(start_line, start_column),
Position::new(end_line, end_column),
new_text,
)
}Update the /// Creates a deletion spanning the given range.
#[must_use]
pub const fn delete(
start_line: u32,
start_column: u32,
end_line: u32,
end_column: u32,
) -> Self {
Self::from_positions(
Position::new(start_line, start_column),
Position::new(end_line, end_column),
String::new(),
)
}AI Coding Agent PromptThis refactoring improves code health by:
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/verification.rs Comment on lines +278 to +303 fn line_column_to_offset(lines: &[&str], line: u32, column: u32) -> Option<usize> {
let line_idx = line as usize;
if line_idx > lines.len() {
return None;
}
// Calculate offset to the start of the target line
let mut offset: usize = 0;
for (idx, &line_content) in lines.iter().enumerate() {
if idx == line_idx {
break;
}
offset = offset.checked_add(line_content.len())?;
offset = offset.checked_add(1)?; // newline character
}
// Add column offset
let col_offset = column as usize;
if line_idx < lines.len() {
let line_content = lines.get(line_idx)?;
if col_offset > line_content.len() {
return None;
}
}
offset.checked_add(col_offset)
}❌ New issue: Bumpy Road Ahead |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/transaction.rs Comment on lines +230 to +251 fn successful_transaction_commits_changes() {
let dir = TempDir::new().expect("create temp dir");
let path = temp_file(&dir, "test.txt", "hello world");
let edit = FileEdit::with_edits(
path.clone(),
vec![TextEdit::from_coords(0, 0, 0, 5, "greetings".to_string())],
);
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::passing();
let mut transaction = EditTransaction::new(&syntactic, &semantic);
transaction.add_edit(edit);
let outcome = transaction.execute().expect("should succeed");
assert!(outcome.committed());
assert_eq!(outcome.files_modified(), Some(1));
let content = fs::read_to_string(&path).expect("read file");
assert_eq!(content, "greetings world");
}❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. @leynos, I've already provided a comprehensive fix for this exact issue earlier in our conversation. My previous response included:
You can find my detailed response above in this thread, where I addressed the same Bumpy Road issue for the If you need any clarification on the suggested refactoring, have questions about the helper function design, or would like me to explain any part of the solution in more detail, I'm happy to help! Is there a specific aspect of the previous fix you'd like me to elaborate on, or are you encountering any issues applying it? |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/edit.rs Comment on file //! Types representing file edits and modifications.
❌ New issue: Primitive Obsession |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13423 @leynos, I've analysed the test module. The five test functions share identical setup boilerplate: creating a TempDir, creating test files, constructing FileEdit with TextEdit, configuring locks, building and executing a transaction. The variation points are lock configuration and assertions. Extract a test builder to eliminate this duplication. Suggested FixAdd a test builder helper at the beginning of the /// Builder for transaction test scenarios.
struct TransactionTestBuilder {
dir: TempDir,
files: Vec<(PathBuf, String)>,
edits: Vec<FileEdit>,
}
impl TransactionTestBuilder {
fn new() -> Self {
Self {
dir: TempDir::new().expect("create temp dir"),
files: Vec::new(),
edits: Vec::new(),
}
}
fn with_file(mut self, name: &str, content: &str) -> Self {
let path = self.dir.path().join(name);
let mut file = fs::File::create(&path).expect("create temp file");
file.write_all(content.as_bytes()).expect("write temp file");
self.files.push((path, content.to_string()));
self
}
fn with_new_file_path(mut self, name: &str) -> Self {
let path = self.dir.path().join(name);
self.files.push((path, String::new()));
self
}
fn with_replacement_edit(mut self, file_idx: usize, start_col: u32, end_col: u32, text: &str) -> Self {
let path = &self.files[file_idx].0;
let edit = FileEdit::with_edits(
path.clone(),
vec![TextEdit::from_coords(0, 0, 0, end_col, text.to_string())],
);
self.edits.push(edit);
self
}
fn with_insert_edit(mut self, file_idx: usize, text: &str) -> Self {
let path = &self.files[file_idx].0;
let edit = FileEdit::with_edits(
path.clone(),
vec![TextEdit::insert(0, 0, text.to_string())],
);
self.edits.push(edit);
self
}
fn execute_with_locks(
mut self,
syntactic: &dyn SyntacticLock,
semantic: &dyn SemanticLock,
) -> (TransactionOutcome, Vec<PathBuf>) {
let mut transaction = EditTransaction::new(syntactic, semantic);
for edit in self.edits.drain(..) {
transaction.add_edit(edit);
}
let outcome = transaction.execute().expect("execute transaction");
let paths = self.files.into_iter().map(|(p, _)| p).collect();
(outcome, paths)
}
fn file_path(&self, idx: usize) -> &PathBuf {
&self.files[idx].0
}
}Then refactor the tests: #[test]
fn successful_transaction_commits_changes() {
let builder = TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings");
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::passing();
let (outcome, paths) = builder.execute_with_locks(&syntactic, &semantic);
assert!(outcome.committed());
assert_eq!(outcome.files_modified(), Some(1));
let content = fs::read_to_string(&paths[0]).expect("read file");
assert_eq!(content, "greetings world");
}
#[test]
fn syntactic_failure_prevents_commit() {
let builder = TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings");
let path = builder.file_path(0).clone();
let failures = vec![VerificationFailure::new(path.clone(), "syntax error")];
let syntactic = ConfigurableSyntacticLock::failing(failures);
let semantic = ConfigurableSemanticLock::passing();
let (outcome, paths) = builder.execute_with_locks(&syntactic, &semantic);
assert!(matches!(outcome, TransactionOutcome::SyntacticLockFailed { .. }));
let content = fs::read_to_string(&paths[0]).expect("read file");
assert_eq!(content, "hello world");
}
#[test]
fn semantic_failure_prevents_commit() {
let builder = TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings");
let path = builder.file_path(0).clone();
let failures = vec![VerificationFailure::new(path.clone(), "type error")];
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::failing(failures);
let (outcome, paths) = builder.execute_with_locks(&syntactic, &semantic);
assert!(matches!(outcome, TransactionOutcome::SemanticLockFailed { .. }));
let content = fs::read_to_string(&paths[0]).expect("read file");
assert_eq!(content, "hello world");
}
#[test]
fn semantic_backend_error_propagates() {
let builder = TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings");
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::unavailable("LSP crashed");
let mut transaction = EditTransaction::new(&syntactic, &semantic);
for edit in builder.edits {
transaction.add_edit(edit);
}
let result = transaction.execute();
assert!(result.is_err());
assert!(matches!(
result.unwrap_err(),
SafetyHarnessError::SemanticBackendUnavailable { .. }
));
let content = fs::read_to_string(builder.file_path(0)).expect("read file");
assert_eq!(content, "hello world");
}
#[test]
fn handles_new_file_creation() {
let builder = TransactionTestBuilder::new()
.with_new_file_path("new_file.txt")
.with_insert_edit(0, "new content");
let path = builder.file_path(0).clone();
assert!(!path.exists());
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::passing();
let (outcome, paths) = builder.execute_with_locks(&syntactic, &semantic);
assert!(outcome.committed());
let content = fs::read_to_string(&paths[0]).expect("read file");
assert_eq!(content, "new content");
}AI Coding Agent PromptThis refactoring achieves:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6361 @leynos, I've analysed the Suggested FixAdd Position-based method variants and a ReplacementText newtype: /// Replacement text for a text edit operation.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ReplacementText(String);
impl ReplacementText {
/// Creates new replacement text.
#[must_use]
pub fn new(text: String) -> Self {
Self(text)
}
/// Creates empty replacement text (for deletions).
#[must_use]
pub const fn empty() -> Self {
Self(String::new())
}
/// Returns the text as a string slice.
#[must_use]
pub fn as_str(&self) -> &str {
&self.0
}
/// Consumes the wrapper and returns the inner String.
#[must_use]
pub fn into_inner(self) -> String {
self.0
}
}
impl From<String> for ReplacementText {
fn from(s: String) -> Self {
Self(s)
}
}
impl From<&str> for ReplacementText {
fn from(s: &str) -> Self {
Self(s.to_string())
}
}
impl AsRef<str> for ReplacementText {
fn as_ref(&self) -> &str {
&self.0
}
}Update impl TextEdit {
/// Builds a text edit from a range and replacement text.
#[must_use]
pub const fn new(range: TextRange, new_text: String) -> Self {
Self { range, new_text }
}
/// Builds a text edit from a range and replacement text wrapper.
#[must_use]
pub fn with_replacement(range: TextRange, text: ReplacementText) -> Self {
Self {
range,
new_text: text.into_inner(),
}
}
/// Builds a text edit from explicit positions.
///
/// This is the preferred method for creating edits from position objects,
/// reducing primitive obsession compared to coordinate-based constructors.
#[must_use]
pub fn from_positions(start: Position, end: Position, text: ReplacementText) -> Self {
Self::with_replacement(TextRange::new(start, end), text)
}
/// Builds a text edit from explicit positions.
///
/// This convenience constructor accepts position coordinates directly when
/// callers do not want to create intermediate [`Position`] and [`TextRange`]
/// values. The argument count is intentionally above the clippy threshold to
/// match LSP conventions.
///
/// Consider using [`Self::from_positions`] to reduce primitive obsession.
#[must_use]
#[allow(clippy::too_many_arguments, reason = "matches LSP coordinate convention")]
pub const fn from_coords(
start_line: u32,
start_column: u32,
end_line: u32,
end_column: u32,
new_text: String,
) -> Self {
Self::new(
TextRange::new(
Position::new(start_line, start_column),
Position::new(end_line, end_column),
),
new_text,
)
}
/// Creates an insertion at the specified position.
#[must_use]
pub fn insert_at(position: Position, text: ReplacementText) -> Self {
Self::with_replacement(TextRange::point(position), text)
}
/// Creates an insertion at the specified position (coordinate-based).
///
/// Consider using [`Self::insert_at`] to reduce primitive obsession.
#[must_use]
pub const fn insert(line: u32, column: u32, text: String) -> Self {
Self::new(TextRange::point(Position::new(line, column)), text)
}
/// Creates a deletion spanning the given range.
#[must_use]
pub fn delete_range(start: Position, end: Position) -> Self {
Self::from_positions(start, end, ReplacementText::empty())
}
/// Creates a deletion spanning the given range (coordinate-based).
///
/// Consider using [`Self::delete_range`] to reduce primitive obsession.
#[must_use]
pub const fn delete(
start_line: u32,
start_column: u32,
end_line: u32,
end_column: u32,
) -> Self {
Self::from_coords(
start_line,
start_column,
end_line,
end_column,
String::new(),
)
}
// ... rest of the impl unchanged
}AI Coding Agent PromptThis refactoring reduces primitive obsession by:
🐰✨ |
…s tests - Added newtypes (FileName, FileContent, TextPattern, DiagnosticMessage) to replace raw strings in tests - Updated safety harness behaviour tests to use these newtypes improving type safety and clarity - Refactored helper functions and test steps accordingly - Minor improvements and extraction of utility functions in safety_harness verification code for clarity Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/tests/safety_harness_types.rs Comment on file //! Domain-specific NewTypes for safety harness behavioural tests.
❌ New issue: String Heavy Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3958 @leynos, I've analysed the file. The string-heaviness stems from redundant conversion trait implementations. Each NewType has three string-accepting methods: Suggested FixRemove all //! Domain-specific NewTypes for safety harness behavioural tests.
//!
//! These types eliminate string-heavy function arguments and make the test
//! domain model explicit and type-safe.
use std::ops::Deref;
use std::path::{Path, PathBuf};
/// Wraps file name strings.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct FileName(String);
impl From<String> for FileName {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for FileName {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for FileName {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for FileName {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl FileName {
/// Returns the inner string as a string slice.
pub fn as_str(&self) -> &str {
&self.0
}
/// Joins this file name to a base path.
pub fn to_path(&self, base: &Path) -> PathBuf {
base.join(&self.0)
}
}
/// Wraps file content strings.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct FileContent(String);
impl From<String> for FileContent {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for FileContent {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for FileContent {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for FileContent {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl FileContent {
/// Returns the inner string as a string slice.
pub fn as_str(&self) -> &str {
&self.0
}
/// Returns the content as bytes.
pub fn as_bytes(&self) -> &[u8] {
self.0.as_bytes()
}
}
/// Wraps text patterns for search/replace/assertion.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TextPattern(String);
impl From<String> for TextPattern {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for TextPattern {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for TextPattern {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for TextPattern {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl TextPattern {
/// Returns the inner string as a string slice.
pub fn as_str(&self) -> &str {
&self.0
}
}
/// Wraps diagnostic messages for lock configuration.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DiagnosticMessage(String);
impl From<String> for DiagnosticMessage {
fn from(s: String) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl From<&str> for DiagnosticMessage {
fn from(s: &str) -> Self {
Self(s.trim_matches('"').to_string())
}
}
impl AsRef<str> for DiagnosticMessage {
fn as_ref(&self) -> &str {
&self.0
}
}
impl Deref for DiagnosticMessage {
type Target = str;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl DiagnosticMessage {
/// Returns the inner string as a string slice.
pub fn as_str(&self) -> &str {
&self.0
}
}AI Coding Agent PromptThis refactoring achieves:
🐰✨ |
…builder - Introduced ReplacementText newtype to reduce primitive obsession on raw strings in text edits - Added new constructors for TextEdit using ReplacementText for ergonomic and safer API - Exposed ReplacementText from safety_harness module - Added TransactionTestBuilder to simplify and reduce boilerplate in transaction tests - Refactored tests to use TransactionTestBuilder, improving readability and maintainability Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/transaction.rs Comment on lines +343 to +364 fn syntactic_failure_prevents_commit() {
let builder = TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings");
let path = builder.file_path(0).clone();
let failures = vec![VerificationFailure::new(path.clone(), "syntax error")];
let syntactic = ConfigurableSyntacticLock::failing(failures);
let semantic = ConfigurableSemanticLock::passing();
let (result, _, _dir) = builder.execute_with_locks(&syntactic, &semantic);
let outcome = result.expect("should succeed");
assert!(matches!(
outcome,
TransactionOutcome::SyntacticLockFailed { .. }
));
// File should be unchanged
let content = fs::read_to_string(&path).expect("read file");
assert_eq!(content, "hello world");
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/transaction.rs Comment on lines +255 to +275 fn with_replacement_edit(
mut self,
file_idx: usize,
start_col: u32,
end_col: u32,
text: &str,
) -> Self {
let path = self.files[file_idx].0.clone();
let edit = FileEdit::with_edits(
path,
vec![TextEdit::from_coords(
0,
start_col,
0,
end_col,
text.to_string(),
)],
);
self.edits.push(edit);
self
}❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16056 @leynos, I've analysed the three test functions. They share identical setup (builder configuration) and identical assertions (file unchanged check). Extract two helper functions to eliminate this duplication whilst maintaining test clarity. Suggested FixAdd two helper functions in the /// Creates the standard builder setup for failure scenario tests.
fn failure_scenario_builder() -> TransactionTestBuilder {
TransactionTestBuilder::new()
.with_file("test.txt", "hello world")
.with_replacement_edit(0, 0, 5, "greetings")
}
/// Asserts that the file at the given path contains the original unchanged content.
fn assert_file_unchanged(path: &PathBuf) {
let content = fs::read_to_string(path).expect("read file");
assert_eq!(content, "hello world");
}Then refactor the three test functions: #[test]
fn syntactic_failure_prevents_commit() {
let builder = failure_scenario_builder();
let path = builder.file_path(0).clone();
let failures = vec![VerificationFailure::new(path.clone(), "syntax error")];
let syntactic = ConfigurableSyntacticLock::failing(failures);
let semantic = ConfigurableSemanticLock::passing();
let (result, _, _dir) = builder.execute_with_locks(&syntactic, &semantic);
let outcome = result.expect("should succeed");
assert!(matches!(
outcome,
TransactionOutcome::SyntacticLockFailed { .. }
));
assert_file_unchanged(&path);
}
#[test]
fn semantic_failure_prevents_commit() {
let builder = failure_scenario_builder();
let path = builder.file_path(0).clone();
let failures = vec![VerificationFailure::new(path.clone(), "type error")];
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::failing(failures);
let (result, _, _dir) = builder.execute_with_locks(&syntactic, &semantic);
let outcome = result.expect("should succeed");
assert!(matches!(
outcome,
TransactionOutcome::SemanticLockFailed { .. }
));
assert_file_unchanged(&path);
}
#[test]
fn semantic_backend_error_propagates() {
let builder = failure_scenario_builder();
let path = builder.file_path(0).clone();
let syntactic = ConfigurableSyntacticLock::passing();
let semantic = ConfigurableSemanticLock::unavailable("LSP crashed");
let (result, _, _dir) = builder.execute_with_locks(&syntactic, &semantic);
assert!(result.is_err());
assert!(matches!(
result.unwrap_err(),
SafetyHarnessError::SemanticBackendUnavailable { .. }
));
assert_file_unchanged(&path);
}AI Coding Agent PromptThis refactoring achieves:
🐰✨ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16056 @leynos, I've analysed the Suggested FixAdd a parameter object for line replacement operations in the test module: /// Represents a replacement operation on a single line for test scenarios.
#[derive(Debug, Clone)]
struct LineReplacement {
start_col: u32,
end_col: u32,
text: String,
}
impl LineReplacement {
/// Creates a new line replacement.
fn new(start_col: u32, end_col: u32, text: impl Into<String>) -> Self {
Self {
start_col,
end_col,
text: text.into(),
}
}
/// Creates a replacement that spans the entire content (from column 0 to end_col).
fn from_start(end_col: u32, text: impl Into<String>) -> Self {
Self::new(0, end_col, text)
}
}Update the /// Adds a replacement edit for the file at the given index.
fn with_replacement_edit(mut self, file_idx: usize, replacement: LineReplacement) -> Self {
let path = self.files[file_idx].0.clone();
let edit = FileEdit::with_edits(
path,
vec![TextEdit::from_coords(
0,
replacement.start_col,
0,
replacement.end_col,
replacement.text,
)],
);
self.edits.push(edit);
self
}Update test call sites to use the parameter object: // Before:
.with_replacement_edit(0, 0, 5, "greetings")
// After:
.with_replacement_edit(0, LineReplacement::from_start(5, "greetings"))AI Coding Agent PromptThis refactoring improves code health by:
🐰✨ |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function Argumentscrates/weaverd/src/safety_harness/transaction.rs: tests.TransactionTestBuilder.with_replacement_edit What lead to degradation?tests.TransactionTestBuilder.with_replacement_edit has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
|
Tip For best results, initiate chat on the files or code changes. @leynos, I've already provided a comprehensive fix for this exact issue earlier in our conversation thread above! My previous response included:
The solution follows the INTRODUCE PARAMETER OBJECT pattern you requested and reduces the argument count below the threshold whilst maintaining type safety and improving intent clarity. If you're encountering any issues applying the previous fix, have questions about the |
…ion-based constructors - Deprecated `from_coords` method in favor of safer `from_positions` using Position structs - Added `from_positions` accepting String for convenience, promoting parameter object pattern - Renamed original `from_positions` to `from_positions_typed` to clarify usage with ReplacementText newtype - Updated delete constructors to use new position-based methods - Added allow deprecated annotations in tests using legacy coordinate API This improves type safety by reducing primitive coordinate usage and encourages better construction of text edits. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (8)
crates/weaverd/src/tests/safety_harness_behaviour.rs (1)
210-238: Add execute-if-missing guard to these Then steps.These Then steps assume a transaction has executed, but scenarios using
when_edit_replaces_in_fileonly queue edits without executing. Add the same guard pattern used inthen_commitsandthen_no_changes:#[then("the transaction fails with a syntactic lock error")] fn then_syntactic_fails(world: &RefCell<SafetyHarnessWorld>) { + if world.borrow().outcome().is_none() { + world.borrow_mut().execute_transaction(); + } let world = world.borrow(); let outcome = world.outcome().expect("outcome should exist");Apply the same pattern to
then_semantic_failsandthen_backend_error.crates/weaverd/src/safety_harness/transaction.rs (4)
1-546: Extract test module to comply with 400-line limit.At 546 lines, this file exceeds the mandatory 400-line limit. The implementation (lines 1-241) is 241 lines; the test module (lines 243-546) adds 303 lines. Extract the test module to
crates/weaverd/src/safety_harness/transaction/tests.rs.
227-241: Track file existence explicitly in rollback metadata.The rollback logic infers "new file" from
original.is_empty(), which incorrectly deletes existing empty files on rollback. Store file existence asOption<String>or awas_new: boolflag alongside original content.-fn rollback(committed: &[(PathBuf, String)]) { +fn rollback(committed: &[(PathBuf, Option<String>)]) { for (path, original) in committed { - if original.is_empty() { - // File was newly created, remove it - let _ = std::fs::remove_file(path); - } else { - // Restore original content (best effort) - let _ = std::fs::write(path, original); + match original { + None => { + // File was newly created, remove it + let _ = std::fs::remove_file(path); + } + Some(content) => { + // Restore original content (best effort) + let _ = std::fs::write(path, content); + } } } }Update
commit_changesto passOption<String>instead of defaulting to empty string.
211-225: Create parent directories before writing temporary files.
tempfile::NamedTempFile::new_in(parent)fails if the parent directory does not exist. For new files in nested directories (e.g.,src/foo/bar/new.rs), add directory creation:fn prepare_file( path: &std::path::Path, content: &str, ) -> Result<tempfile::NamedTempFile, SafetyHarnessError> { let parent = path.parent().unwrap_or_else(|| std::path::Path::new(".")); + fs::create_dir_all(parent) + .map_err(|err| SafetyHarnessError::file_write(path.to_path_buf(), err))?; + let mut temp_file = tempfile::NamedTempFile::new_in(parent) .map_err(|err| SafetyHarnessError::file_write(path.to_path_buf(), err))?;
480-486: Replace.unwrap_err()with.expect_err().Per coding guidelines, prefer
.expect()over.unwrap()in tests. Provide a descriptive message:assert!(result.is_err()); assert!(matches!( - result.unwrap_err(), + result.expect_err("expected semantic backend unavailable error"), SafetyHarnessError::SemanticBackendUnavailable { .. } ));crates/weaverd/src/safety_harness/verification.rs (3)
1-518: Decompose the file to comply with the 400-line limit.This file contains 518 lines and violates the mandatory 400-line maximum. The past review comment at lines 1-5 provides a clear decomposition strategy:
verification/context.rs—VerificationContextverification/traits.rs—SyntacticLock,SemanticLock, placeholdersverification/apply.rs—apply_editsand offset helpersverification/test_doubles.rs—ConfigurableSyntacticLock,ConfigurableSemanticLockExecute that plan immediately.
As per coding guidelines: "Files must not exceed 400 lines in length."
342-518: Add test coverage for files with trailing newlines.The test suite lacks cases where the original content ends with
\nor\r\n. The past comment at lines 234-284 explicitly requested:
- Multi-line content ending with newline
- Multi-line content without trailing newline
- Edits on the last line in both scenarios
Current tests (lines 478-517) only cover content without trailing newlines. Add tests such as:
In crates/weaverd/src/safety_harness/verification.rs around lines 342 to 518, add unit tests to the tests module that cover: (1) a multi-line string ending with \n where an edit targets the last line, (2) a multi-line string ending with \r\n where an edit targets the last line, and (3) a multi-line string WITHOUT a trailing newline where an edit targets the last line; verify that apply_edits computes offsets correctly in all three cases and that edits are applied at the expected positions; run cargo test -p weaverd to confirm.Based on past review comments.
403-450: Parameterise the duplicatedapply_editstests with rstest.The three tests
apply_edits_inserts_text,apply_edits_deletes_text, andapply_edits_replaces_textshare identical structure. Extract the common pattern into a single parameterised test.Apply this refactoring:
+ use rstest::rstest; + + #[rstest] + #[case("insert", "hello world", TextEdit::insert_at(Position::new(0, 6), "beautiful "), "hello beautiful world")] + #[case("delete", "hello beautiful world", TextEdit::delete_range(Position::new(0, 6), Position::new(0, 16)), "hello world")] + #[case("replace", "fn foo() {}", TextEdit::from_positions(Position::new(0, 3), Position::new(0, 6), "bar".to_string()), "fn bar() {}")] + fn apply_edits_single_operation( + #[case] _name: &str, + #[case] original: &str, + #[case] edit: TextEdit, + #[case] expected: &str, + ) { + let path = PathBuf::from("test.txt"); + let file_edit = FileEdit::with_edits(path, vec![edit]); + let result = apply_edits(original, &file_edit).expect("edit should succeed"); + assert_eq!(result, expected); + } + - #[test] - fn apply_edits_inserts_text() { - use crate::safety_harness::edit::Position; - - let original = "hello world"; - let path = PathBuf::from("test.txt"); - let edit = FileEdit::with_edits( - path, - vec![TextEdit::insert_at(Position::new(0, 6), "beautiful ")], - ); - let result = apply_edits(original, &edit).expect("edit should succeed"); - assert_eq!(result, "hello beautiful world"); - } - - #[test] - fn apply_edits_deletes_text() { - use crate::safety_harness::edit::Position; - - let original = "hello beautiful world"; - let path = PathBuf::from("test.txt"); - let edit = FileEdit::with_edits( - path, - vec![TextEdit::delete_range( - Position::new(0, 6), - Position::new(0, 16), - )], - ); - let result = apply_edits(original, &edit).expect("edit should succeed"); - assert_eq!(result, "hello world"); - } - - #[test] - fn apply_edits_replaces_text() { - use crate::safety_harness::edit::Position; - - let original = "fn foo() {}"; - let path = PathBuf::from("test.rs"); - let edit = FileEdit::with_edits( - path, - vec![TextEdit::from_positions( - Position::new(0, 3), - Position::new(0, 6), - "bar".to_string(), - )], - ); - let result = apply_edits(original, &edit).expect("edit should succeed"); - assert_eq!(result, "fn bar() {}"); - }Add
rstestto dev-dependencies incrates/weaverd/Cargo.tomlif not already present.In crates/weaverd/src/safety_harness/verification.rs around lines 403 to 450, replace the three individual test functions apply_edits_inserts_text, apply_edits_deletes_text, and apply_edits_replaces_text with a single parameterised rstest function named apply_edits_single_operation that accepts case parameters for name, original, edit, and expected; define three #[case] attributes with the test data from the original tests; move the common FileEdit creation and apply_edits call into the parameterised function body; ensure rstest is in dev-dependencies; run cargo test -p weaverd to verify all cases pass.As per coding guidelines: "Replace duplicated tests with #[rstest(...)] parameterised cases in Rust."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
crates/weaverd/src/safety_harness/edit.rs(1 hunks)crates/weaverd/src/safety_harness/error.rs(1 hunks)crates/weaverd/src/safety_harness/mod.rs(1 hunks)crates/weaverd/src/safety_harness/transaction.rs(1 hunks)crates/weaverd/src/safety_harness/verification.rs(1 hunks)crates/weaverd/src/tests/safety_harness_behaviour.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing, which executecargo fmt --workspace -- --check,cargo clippy --workspace --all-targets --all-features -- -D warnings, andcargo test --workspacerespectively.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality; run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated withcargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated tests with#[rstest(...)]parameterised cases in Rust.
Prefermockallfor mocks/stubs in Rust.
Prefer.expect()over.unwrap()in Rust.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line function definitions in Rust where appropriate (e.g., `pub fn new(id: u64) -> Self { ...
Files:
crates/weaverd/src/safety_harness/mod.rscrates/weaverd/src/tests/safety_harness_behaviour.rscrates/weaverd/src/safety_harness/verification.rscrates/weaverd/src/safety_harness/error.rscrates/weaverd/src/safety_harness/transaction.rscrates/weaverd/src/safety_harness/edit.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
crates/weaverd/src/safety_harness/mod.rscrates/weaverd/src/tests/safety_harness_behaviour.rscrates/weaverd/src/safety_harness/verification.rscrates/weaverd/src/safety_harness/error.rscrates/weaverd/src/safety_harness/transaction.rscrates/weaverd/src/safety_harness/edit.rs
🧬 Code graph analysis (1)
crates/weaverd/src/tests/safety_harness_behaviour.rs (5)
crates/weaverd/src/safety_harness/edit.rs (11)
path(218-220)new(23-25)new(40-42)new(65-67)new(123-128)new(198-203)from_positions(135-137)with_edits(212-214)insert_at(143-145)from(89-91)from(95-97)crates/weaverd/src/safety_harness/transaction.rs (3)
new(67-73)new(308-314)new(331-337)crates/weaverd/src/safety_harness/verification.rs (6)
new(29-34)passing(152-154)passing(187-192)failing(158-160)failing(196-201)unavailable(205-210)crates/weaverd/src/safety_harness/error.rs (5)
new(27-34)file(46-48)line(52-54)column(58-60)message(64-66)crates/weaverd/src/tests/safety_harness_types.rs (8)
from(17-19)from(23-25)from(54-56)from(60-62)from(91-93)from(97-99)from(123-125)from(129-131)
🔍 Remote MCP Deepwiki
Summary of additional review-relevant facts (concise)
-
Scope & placement
- New public crate module: crates/weaverd/src/safety_harness with submodules edit, error, locks, transaction, verification; exposed via pub mod safety_harness.
-
Public API surface added
- New public types re-exported: FileEdit, Position, ReplacementText, TextEdit, TextRange, SafetyHarnessError, VerificationFailure, SemanticLockResult, SyntacticLockResult, EditTransaction, TransactionOutcome, VerificationContext, SyntacticLock, SemanticLock, and assorted lock/verification helper types. (Verify consumers for downstream uses).
-
Behavior & guarantees
- EditTransaction implements in‑memory application of edits, two‑phase commit using temp files (prepare → atomic rename) with rollback semantics on failure; TransactionOutcome covers Committed, SyntacticLockFailed, SemanticLockFailed, NoChanges. Tests cover multi-file atomicity and new-file creation.
-
Error & diagnostic model
- Structured error types: SafetyHarnessError enum and VerificationFailure (file + optional line/column + message) with Display impl — used to report lock failures and I/O/edit application errors.
-
Verification model & extensibility
- Pluggable traits SyntacticLock and SemanticLock with Placeholder and Configurable test doubles provided; VerificationContext holds original/modified contents for validators. apply_edits and line/column → offset helpers included and covered by tests.
-
Tests added
- Unit tests across new modules (edit/error/locks/transaction/verification).
- BDD behavioural tests + feature file: crates/weaverd/src/tests/safety_harness_behaviour.rs and crates/weaverd/tests/features/safety_harness.feature exercising success, syntactic/semantic failures, backend-unavailable, empty transactions, atomic multi-file commits, rollback. New test support types in safety_harness_types.rs.
-
Docs & roadmap
- docs/users-guide.md, docs/weaver-design.md, docs/roadmap.md updated to document the Double‑Lock safety harness, two‑phase verification, in‑memory edits, atomic commits, and future LSP doc-sync integration.
-
Build/dev impact
- crates/weaverd/Cargo.toml adds a dev-dependency: derive_more = "1.0" with features as_ref,deref — affects dev/test builds only. Confirm CI/test matrix includes weaverd tests.
Actionable review notes (check during code review)
- API stability: confirm no accidental changes to existing exported symbols beyond the new safety_harness re-exports.
- Correctness of atomic commit/rollback (race conditions, temp-file placement, permission/ownership edge cases).
- apply_edits correctness for mixed/CRLF line endings and offset computations (helpers present; verify edge-case tests pass).
- Error propagation: ensure SafetyHarnessError variants preserve underlying std::io::Error context where needed.
- Test reliability: BDD tests spawn daemons and depend on temporary sockets — confirm fixtures clean up and are stable in CI.
- Docs consistency: verify user guide and design docs match the implemented behavior and public API names.
Sources
- Repo wiki / code documentation pages (contains synthesized content from repository files): Deepwiki_read_wiki_contents (leynos/weaver).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
crates/weaverd/src/safety_harness/mod.rs (1)
39-42: Test doubles exported in public API.
ConfigurableSemanticLock,ConfigurableSyntacticLock,PlaceholderSemanticLock, andPlaceholderSyntacticLockare test doubles. Exporting them publicly is intentional for downstream testing, but document this intent or consider atestingfeature flag to gate these exports if the API surface becomes a concern.crates/weaverd/src/safety_harness/transaction.rs (1)
405-413: LGTM - empty transaction test.Clean test for the no-changes case with proper
.expect()usage.crates/weaverd/src/safety_harness/edit.rs (1)
235-269: LGTM - well-structured unit tests.The tests cover insertion, deletion, and file edit tracking with clear assertions.
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47870 @leynos, Yes! All the issues you listed have been resolved. ✅ Here's the verification summary: Overall Comments1. Multi-file atomicity ✅ RESOLVEDThe
fn commit_changes(...) -> Result<(), SafetyHarnessError> {
// Phase 1: Prepare all files (write to temps)
let mut prepared: Vec<(PathBuf, tempfile::NamedTempFile, String)> = Vec::new();
// ... preparation code ...
// Phase 2: Commit all files (atomic renames)
for (path, temp_file, original) in prepared {
if let Err(err) = temp_file.persist(&path) {
rollback(&committed); // ← Rollback on any failure
return Err(...);
}
committed.push((path, original));
}
}2. CRLF handling ✅ RESOLVEDThe offset computation in
let line_content_end = if line_end > 0 && content.as_bytes().get(line_end - 1) == Some(&b'\n') {
if line_end > 1 && content.as_bytes().get(line_end - 2) == Some(&b'\r') {
line_end - 2 // CRLF
} else {
line_end - 1 // LF
}
} else {
line_end // Last line without trailing newline
};3. Unused error variants ✅ RESOLVEDLooking at Individual CommentsComment 1: ReplacementText::empty const fn ✅ RESOLVEDThe pub fn empty() -> Self {
Self(String::new())
}Comment 2: CRLF offset computation ✅ RESOLVEDSame as overall comment #2 above. Comment 3: API complexity ✅ RESOLVEDThe API has been significantly simplified:
The cleaner API provides a single canonical way to perform each operation. Comment 4: #[allow] in edit.rs ✅ RESOLVEDNo Comment 5: #[allow] in transaction.rs ✅ RESOLVEDThe fn with_replacement_edit(mut self, file_idx: usize, replacement: LineReplacement) -> SelfNo Summary: All issues have been successfully addressed through refactoring. The code now has proper multi-file transaction semantics with rollback, correct CRLF handling, a cleaner API surface, and no lint suppressions. 🎯 🐰✨ |
…dits Add tests to verify that: 1. Edits targeting lines past EOF are rejected with EditApplicationError rather than causing a panic 2. End-of-file positions (last column of last line) work correctly The implementation already handles these cases correctly via the line_starts.get() returning None for invalid line indices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/weaverd/src/safety_harness/verification.rs (2)
1-233: Split this module; file-length exceeds 400-line guidelineKeep this file under the 400-line limit from the coding guidelines. Extract subdomains into submodules so verification.rs only wires them together.
Refactor along these lines:
- Move
VerificationContextintoverification/context.rs.- Move
SyntacticLock/SemanticLocktraits and placeholder locks intoverification/traits.rs.- Move
apply_edits,compute_line_start_offsets, andline_column_to_offsetintoverification/apply.rs.- Move
ConfigurableSyntacticLockandConfigurableSemanticLockinto a dedicated test-support module such asverification/test_doubles.rs.Re-export the public surface from this root module and update use paths in transaction code and tests.
234-340: Keep CRLF-safe offset logic and add explicit trailing-newline coverage; extract newline helperThe new
apply_edits/compute_line_start_offsets/line_column_to_offsetpipeline now:
- Computes line starts from raw bytes and handles both LF and CRLF correctly.
- Applies edits from bottom to top so offsets remain valid.
- Rejects past-EOF lines cleanly via
EditApplicationError.Retain this structure, but strengthen and simplify it:
- Add unit tests that cover multi-line content ending with a trailing newline (both LF and CRLF), including:
- Replacement on the last non-empty line.
- Insertion at the final empty line (EOF) position.
- Extract the CRLF/LF trimming logic into a helper to reduce branching in
line_column_to_offset, along the lines of:fn line_content_length(content: &str, line_start: usize, line_end: usize) -> usize { let adjusted_end = if line_end > 0 && content.as_bytes().get(line_end - 1) == Some(&b'\n') { if line_end > 1 && content.as_bytes().get(line_end - 2) == Some(&b'\r') { line_end - 2 } else { line_end - 1 } } else { line_end }; adjusted_end.saturating_sub(line_start) }and then use
line_content_lengthinsideline_column_to_offsetto computeline_len. This drops nested conditionals and keeps the offset function easier to audit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/weaverd/src/safety_harness/verification.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing, which executecargo fmt --workspace -- --check,cargo clippy --workspace --all-targets --all-features -- -D warnings, andcargo test --workspacerespectively.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality; run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated withcargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line functions in Rust.
Use predicate functions for conditional criteria with more than two branches in Rust.
Lints must not be silenced except as a last resort; lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated tests with#[rstest(...)]parameterised cases in Rust.
Prefermockallfor mocks/stubs in Rust.
Prefer.expect()over.unwrap()in Rust.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line function definitions in Rust where appropriate (e.g., `pub fn new(id: u64) -> Self { ...
Files:
crates/weaverd/src/safety_harness/verification.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
crates/weaverd/src/safety_harness/verification.rs
🔍 Remote MCP Ref
Summary of additional review-relevant facts found on the PR page (concise):
-
PR metadata: Title "Add Double-Lock safety harness for weaverd with atomic commits (PR #34)", author leynos, source branch terragon/implement-double-lock-safety-x8eg4y -> main, created 2025-12-08, URL #34.,
-
Files changed (high-level): new crate module crates/weaverd/src/safety_harness with submodules edit.rs, error.rs, locks.rs, transaction.rs, verification.rs; tests added under crates/weaverd/src/tests and crates/weaverd/tests/features; docs updated (docs/users-guide.md, docs/weaver-design.md, docs/roadmap.md); Cargo dev-dep derive_more added for weaverd. (Matches PR description + file list on PR page.)
-
Tests: extensive unit tests and BDD behavioural tests added; feature file present for rstest_bdd scenarios covering commit, lock failures, backend unavailability, empty transactions, multi-file atomicity, new-file creation. Reviewer should run cargo test -p weaverd (CI matrix not shown on PR page).
-
Review action items visible on PR page: watch for API surface expansion (new public re-exports), correctness of two-phase commit/rollback across multiple files, apply_edits offset/CRLF correctness, error propagation preserving io error context, and test stability in CI. (These align with PR description and visible diffs.)
Sources consulted:
- PR listing/search result and PR page: #34,
…ansactions with verification - Introduce two-phase commit for edit transactions: prepare writes to temp files; commit performs atomic renames. - Track file existence to support rollback that restores original state, including removing newly created files. - Create configurable syntactic and semantic verification locks for test scenarios. - Moves text edit application logic to verification/apply.rs with robust handling of LF and CRLF line endings. - Add extensive tests covering success, syntactic and semantic lock failures, backend errors, new file creation, and multi-file edits. - Update docs to reflect verification and atomic transaction behavior and acceptance criteria. This change enforces safety harness guarantees that multi-file edits are either fully committed or fully rolled back on failure, with verification locks gating commits. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/verification/apply.rs Comment on lines +121 to +130 fn apply_edits_inserts_text() {
let original = "hello world";
let path = PathBuf::from("test.txt");
let edit = FileEdit::with_edits(
path,
vec![TextEdit::insert_at(Position::new(0, 6), "beautiful ")],
);
let result = apply_edits(original, &edit).expect("edit should succeed");
assert_eq!(result, "hello beautiful world");
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/safety_harness/transaction/tests.rs Comment on lines +195 to +211 fn syntactic_failure_prevents_commit() {
test_lock_failure(
|path| {
let failures = vec![VerificationFailure::new(path, "syntax error")];
(
ConfigurableSyntacticLock::failing(failures),
ConfigurableSemanticLock::passing(),
)
},
|outcome| {
assert!(matches!(
outcome,
TransactionOutcome::SyntacticLockFailed { .. }
));
},
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
✅ Actions performedReview triggered.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…th helper function Refactor tests applying text edits to use a new helper `assert_edits_produce` that consolidates common setup and assertions. This reduces boilerplate, improves readability, and maintains test coverage for various edit scenarios including insertions, deletions, replacements, multiple edits, and different line endings. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
…ction execution - Change path argument types from &PathBuf to &Path for more idiomatic usage in verification.rs and error.rs. - Adapt related iterator return types accordingly. - In safety harness behaviour tests, execute transactions on demand before outcome assertions to ensure tests do not fail due to missing execution. These changes improve code clarity and test robustness without altering behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/tests/safety_harness_behaviour.rs Comment on lines +195 to +206 fn then_commits(world: &RefCell<SafetyHarnessWorld>) {
// Execute if not already done
if world.borrow().outcome().is_none() {
world.borrow_mut().execute_transaction();
}
let world = world.borrow();
let outcome = world.outcome().expect("outcome should exist");
assert!(
outcome.as_ref().is_ok_and(|o| o.committed()),
"transaction should commit: {outcome:?}"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…tests - Removed the ReplacementText newtype wrapper in edit.rs and all its usages to reduce unnecessary abstraction. - Updated mod.rs to stop re-exporting ReplacementText. - Consolidated syntactic and semantic lock failure tests into a single parameterized test using rstest for better coverage and maintainability. - Refactored apply.rs to extract line content length logic, improving newline handling accuracy (supporting CR, LF, CRLF). - Converted multiple individual tests on apply_edits to parameterized rstest cases to reduce duplication. - Enhanced safety_harness_behaviour.rs tests by caching original file contents to enable more reliable "file unchanged" assertions. - Added a helper assert_outcome function to consolidate transaction outcome assertions and ensure consistent transaction execution before checks. These changes improve code clarity, test robustness, and maintainability without altering external behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Changes
Safety Harness Core
Transaction Engine
Verification & Validation
Tests & Behavioural Scenarios
Public API Exposure
Documentation
Why
Testing Plan
Notes & Future Work
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/0e88c363-d494-4907-bd35-1913ad42fca8