From cff612f63da59fac8580ad4ec943bf16f8aba5ab Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Sun, 9 Aug 2026 10:01:43 -0600 Subject: [PATCH] fix(format): stop deleting sections and duplicating labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs, both of which made `lash format` unsafe to run on a file it did not fully model. `format_file` rebuilt the file from the parsed model alone, and the model holds the header, the Description section and the task tree and nothing else. Every other section was silently deleted: a file with `## Notes` and `## References` came back with only the header and the tasks, exit code 0, no warning. This turned out to be broader than filed. A section *above* `## Tasks` went too, because the parser folds it into "overview" text that `TaskFile` never stored and the formatter never emitted. `format_file` now takes the source alongside the parsed file. It regenerates only the spans it owns — the H1 plus annotation block, the Description section, the Tasks section — and copies every other line through unchanged. Section boundaries come from `parser::header::section_span`, which runs through pulldown-cmark, so a `##` inside a fenced code block neither opens nor closes a section. Passing an empty source keeps the old model-only behaviour, which is what the existing unit tests want. The second bug: the parser records inline labels in metadata without removing them from the title, and the formatter wrote both. Every run appended another copy of every label, so `- [ ] one #docs` became `#docs #docs` and then `#docs #docs #docs`, and `format --check` reported the file as needing formatting forever. The formatter strips inline labels from the title and writes the sorted metadata list as the only place they appear. The parsed title keeps them, so `lash list` and search behave as they do today. Each generated block ends with its own blank separator and the source's is skipped rather than copied. Emitting both would add a blank line per run, which is the same non-idempotence in a different coat. Tests: sections above and below `## Tasks` survive and keep their order, a fenced heading does not split a section, labels are emitted once, an already-canonical file comes back byte-identical, and an idempotence property test over six shapes asserting `format(format(x)) == format(x)`. All six fail against the old behaviour. Also ticks off three items in lash.index.md that were fixed in #34 and #35 but never marked done. --- crates/lash-cli/src/commands/format.rs | 11 +- crates/lash-core/src/formatter/mod.rs | 114 ++++++++++--- crates/lash-core/src/parser/header.rs | 135 ++++++++++++---- crates/lash-core/tests/formatter_tests.rs | 185 ++++++++++++++++++---- lash.index.md | 38 ++++- 5 files changed, 391 insertions(+), 92 deletions(-) diff --git a/crates/lash-cli/src/commands/format.rs b/crates/lash-cli/src/commands/format.rs index d9ed43e..f8001dd 100644 --- a/crates/lash-cli/src/commands/format.rs +++ b/crates/lash-cli/src/commands/format.rs @@ -347,16 +347,17 @@ fn format_single_file( let task_file = parse_file(file_path, config) .with_context(|| format!("Failed to parse {}", file_path.display()))?; + // Read original content. The formatter needs it as well as the parsed + // file, to carry through sections the model does not represent. + let original = std::fs::read_to_string(file_path) + .with_context(|| format!("Failed to read {}", file_path.display()))?; + // Format the file let formatter = Formatter::new(config.clone(), options.clone()); let formatted = formatter - .format_file(&task_file) + .format_file(&original, &task_file) .with_context(|| format!("Failed to format {}", file_path.display()))?; - // Read original content - let original = std::fs::read_to_string(file_path) - .with_context(|| format!("Failed to read {}", file_path.display()))?; - // Check if content changed let changed = formatted != original; diff --git a/crates/lash-core/src/formatter/mod.rs b/crates/lash-core/src/formatter/mod.rs index ca6d4c3..56539bf 100644 --- a/crates/lash-core/src/formatter/mod.rs +++ b/crates/lash-core/src/formatter/mod.rs @@ -37,12 +37,13 @@ //! description: None, //! description_agent_notes: Vec::new(), //! }; -//! let formatted = formatter.format_file(&file).unwrap(); +//! let formatted = formatter.format_file("", &file).unwrap(); //! assert!(formatted.contains("# Test File")); //! ``` pub mod options; +use crate::parser::header; use lash_types::{LashConfig, Result, TaskFile, TaskStatus}; use std::path::Path; @@ -125,11 +126,11 @@ impl Formatter { /// # description: None, /// # description_agent_notes: Vec::new(), /// # }; - /// let formatted = formatter.format_file(&file).unwrap(); + /// let formatted = formatter.format_file("", &file).unwrap(); /// assert!(formatted.contains("# Test File")); /// ``` #[allow(clippy::result_large_err)] // LashError is intentionally rich with context - pub fn format_file(&self, file: &TaskFile) -> Result { + pub fn format_file(&self, source: &str, file: &TaskFile) -> Result { // Apply auto-fixes if enabled let file = if self.options.apply_auto_fixes { self.apply_auto_fixes(file)? @@ -137,29 +138,67 @@ impl Formatter { file.clone() }; - // Build the formatted output + let lines: Vec<&str> = source.lines().collect(); + let header = header::header_span(source); + let description = header::section_span(source, "description"); + let tasks = header::section_span(source, "tasks"); + let mut output = String::new(); - // 1. Format header (title, annotations, overview) + // The header is always regenerated, whether or not the source had one. self.format_header(&file, &mut output); - // 2. Format description section (if present) - if file.description.is_some() { - Self::format_description(&file, &mut output); - } + // Then walk the rest of the source. Two spans are ours to replace with + // generated text; everything else is copied through byte for byte, + // because the model does not represent it and regenerating the file + // from the model alone silently deletes it. + // Each generated block ends with its own blank separator, so the + // source's is skipped rather than copied; emitting both would add a + // blank line on every run and formatting would not be idempotent. + let skip_blanks = |line: &mut usize| { + while lines.get(*line).is_some_and(|l| l.trim().is_empty()) { + *line += 1; + } + }; - // 3. Format tasks section - self.format_tasks(&file, &mut output); + let mut line = header.end; + skip_blanks(&mut line); - // 4. Format references section (if present) - // TODO: Implement once references are stored in TaskFile + while line < lines.len() { + if description.as_ref().is_some_and(|span| span.start == line) { + Self::format_description(&file, &mut output); + line = description.as_ref().map_or(line + 1, |span| span.end); + skip_blanks(&mut line); + } else if tasks.as_ref().is_some_and(|span| span.start == line) { + self.format_tasks(&file, &mut output); + output.push('\n'); + line = tasks.as_ref().map_or(line + 1, |span| span.end); + skip_blanks(&mut line); + } else { + output.push_str(lines[line]); + output.push('\n'); + line += 1; + } + } - // 4. Normalize whitespace + // A source with no `## Tasks` heading still gets one. The parser treats + // such a file as all-tasks and warns, and emitting the section is what + // makes `lash format` able to repair it. + if tasks.is_none() { + if description.is_none() && file.description.is_some() { + Self::format_description(&file, &mut output); + } + self.format_tasks(&file, &mut output); + } + + // Normalize whitespace. This only collapses runs of blank lines and + // trims trailing whitespace, so it is safe to run over copied-through + // content as well as generated content. if self.options.normalize_whitespace { output = self.normalize_whitespace(&output); } - // 5. Ensure file ends with single newline + // Ensure file ends with single newline if !output.ends_with('\n') { output.push('\n'); } else if output.ends_with("\n\n") { @@ -206,11 +245,18 @@ impl Formatter { /// ``` #[allow(clippy::result_large_err)] // LashError is intentionally rich with context pub fn format_file_in_place(&self, path: &Path) -> Result<()> { - // Parse the file + // Parse the file, keeping the source: the formatter needs it to carry + // through sections the model does not represent. + let source = std::fs::read_to_string(path).map_err(|e| lash_types::LashError::IO { + code: lash_types::error::codes::E_IO_READ_ERROR, + message: format!("Failed to read file for formatting: {e}"), + path: Some(path.to_path_buf()), + io_error: Some(e.to_string()), + })?; let file = crate::parser::parse_file(path, &self.config)?; // Format it - let formatted = self.format_file(&file)?; + let formatted = self.format_file(&source, &file)?; // Write back atomically (tmp file + rename) so a crash mid-write // can't leave a partial Markdown file on disk. This shares the same @@ -525,11 +571,21 @@ impl Formatter { output.push_str("- ["); output.push(task.status.to_checkbox_char()); output.push_str("] "); - output.push_str(&task.title); + + // The parser leaves inline labels in the title *and* records them in + // metadata, so writing the title verbatim and then appending the + // labels emits each one twice. Formatting was therefore not + // idempotent: every run grew the line by another copy of every inline + // label, and `format --check` reported the file as needing formatting + // forever. Strip them from the title and let the canonical, sorted + // list below be the only place they appear. + output.push_str(&strip_inline_labels(&task.title)); // Add inline labels if present if !task.metadata.labels.is_empty() { - for label in &task.metadata.labels { + let mut labels = task.metadata.labels.clone(); + labels.sort(); + for label in &labels { output.push_str(" #"); output.push_str(label); } @@ -679,6 +735,20 @@ impl Formatter { } } +/// Remove inline `#label` words from a task title +/// +/// The parser records inline labels in `TaskMetadata` without removing them +/// from the title, so a formatter that writes both would emit each label +/// twice. Only whole words that parsing would turn into labels are removed, so +/// a `#` inside prose survives. +fn strip_inline_labels(title: &str) -> String { + title + .split_whitespace() + .filter(|word| !lash_types::label::is_inline_label(word)) + .collect::>() + .join(" ") +} + #[cfg(test)] mod tests { use super::*; @@ -779,7 +849,7 @@ mod tests { make_task("task2", "Second task", TaskStatus::Done, None, 0), ]); - let result = formatter.format_file(&file).unwrap(); + let result = formatter.format_file("", &file).unwrap(); assert!(result.contains("# Test File")); assert!(result.contains("@id: test")); @@ -800,7 +870,7 @@ mod tests { make_task("child2", "Child 2", TaskStatus::Done, Some("parent"), 1), ]); - let result = formatter.format_file(&file).unwrap(); + let result = formatter.format_file("", &file).unwrap(); assert!(result.contains("- [ ] Parent task")); assert!(result.contains(" - [ ] Child 1")); @@ -822,7 +892,7 @@ mod tests { make_task("child2", "Child 2", TaskStatus::Open, Some("parent"), 1), ]); - let result = formatter.format_file(&file).unwrap(); + let result = formatter.format_file("", &file).unwrap(); // Children should be auto-waived assert!(result.contains("- [-] Parent")); diff --git a/crates/lash-core/src/parser/header.rs b/crates/lash-core/src/parser/header.rs index c21c9c1..780ad48 100644 --- a/crates/lash-core/src/parser/header.rs +++ b/crates/lash-core/src/parser/header.rs @@ -348,59 +348,55 @@ pub fn content_has_tasks_section(content: &str) -> bool { false } -/// Line span of the body of the `## Tasks` section +/// Line span of a `## ` section, heading line included /// -/// The range is 0-indexed and half-open: it starts at the first line after the -/// `## Tasks` heading and ends at the line holding the next H1 or H2 heading, -/// or at the end of the file. H3 and deeper headings sit inside the section -/// (files often group tasks under `### Subsection` headings) and do not close -/// it. Returns `None` if the file has no `## Tasks` heading. +/// The range is 0-indexed and half-open: it starts at the heading and ends at +/// the line holding the next H1 or H2 heading, or at the end of the file. H3 +/// and deeper headings sit inside the section (files often group tasks under +/// `### Subsection` headings) and do not close it. Returns `None` if the file +/// has no such H2. Matching on `name` is case-insensitive. /// /// A parsed [`lash_types::TaskFile`] records task line numbers but no section -/// boundaries, so anything that needs the extent of the Tasks section (in -/// particular appending to a section with no tasks in it yet) has to go back to -/// the source text. Heading detection runs through pulldown-cmark, so a `##` -/// inside a fenced code block does not close the section. +/// boundaries, so anything that needs the extent of a section has to go back +/// to the source text. Heading detection runs through pulldown-cmark, so a +/// `##` inside a fenced code block neither opens nor closes a section. /// /// # Example /// /// ``` -/// use lash_core::parser::header::tasks_section_body; +/// use lash_core::parser::header::section_span; /// /// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n"; -/// // Lines 3 through 5: the blank line, the task, and the blank line after it. -/// assert_eq!(tasks_section_body(content), Some(3..6)); -/// -/// // An empty section still has a body, it is just blank. -/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3)); -/// -/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None); +/// assert_eq!(section_span(content, "tasks"), Some(2..6)); +/// assert_eq!(section_span(content, "notes"), Some(6..9)); +/// assert_eq!(section_span(content, "nope"), None); /// ``` #[must_use] -pub fn tasks_section_body(content: &str) -> Option> { +pub fn section_span(content: &str, name: &str) -> Option> { let mut heading_level: Option = None; let mut heading_text = String::new(); - let mut body_start_byte: Option = None; - let mut body_end_byte = content.len(); + let mut span: Option> = None; for (event, range) in Parser::new(content).into_offset_iter() { match event { Event::Start(Tag::Heading { level, .. }) => { - if body_start_byte.is_some() && matches!(level, HeadingLevel::H1 | HeadingLevel::H2) - { - body_end_byte = range.start; - break; + if let Some(started) = span.as_mut() { + if matches!(level, HeadingLevel::H1 | HeadingLevel::H2) { + started.end = line_at_byte(content, range.start); + break; + } } heading_level = Some(level); heading_text.clear(); } Event::Text(text) if heading_level.is_some() => heading_text.push_str(&text), Event::End(TagEnd::Heading(level)) => { - if body_start_byte.is_none() + if span.is_none() && level == HeadingLevel::H2 - && heading_text.trim().eq_ignore_ascii_case("tasks") + && heading_text.trim().eq_ignore_ascii_case(name) { - body_start_byte = Some(range.end); + let start = line_at_byte(content, range.start); + span = Some(start..content.lines().count()); } heading_level = None; } @@ -408,8 +404,87 @@ pub fn tasks_section_body(content: &str) -> Option> { } } - let body_start_byte = body_start_byte?; - Some(line_at_byte(content, body_start_byte)..line_at_byte(content, body_end_byte)) + span +} + +/// Line span of the body of the `## Tasks` section +/// +/// Same as [`section_span`] for "tasks", minus the heading line, so it starts +/// at the first line below `## Tasks`. +/// +/// # Example +/// +/// ``` +/// use lash_core::parser::header::tasks_section_body; +/// +/// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n"; +/// // Lines 3 through 5: the blank line, the task, and the blank line after it. +/// assert_eq!(tasks_section_body(content), Some(3..6)); +/// +/// // An empty section still has a body, it is just blank. +/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3)); +/// +/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None); +/// ``` +#[must_use] +pub fn tasks_section_body(content: &str) -> Option> { + section_span(content, "tasks").map(|span| (span.start + 1).min(span.end)..span.end) +} + +/// Line span the formatter regenerates as the file header: the H1 and the +/// annotation block below it +/// +/// Everything below it belongs to whoever is rewriting the file. Overview +/// prose and any section the model does not represent live there, and the +/// formatter has to copy them through rather than regenerate them. +/// +/// The range always starts at 0. It is empty when the file has no H1, which is +/// the case where the parser synthesizes a title from the filename and the +/// formatter writes a header that was not there before. +/// +/// # Example +/// +/// ``` +/// use lash_core::parser::header::header_span; +/// +/// // The H1 and the `@id:` line, but not the prose below them. +/// assert_eq!(header_span("# T\n\n@id: t\n\nsome prose\n"), 0..3); +/// +/// // No H1 to regenerate around. +/// assert_eq!(header_span("just prose\n"), 0..0); +/// ``` +#[must_use] +pub fn header_span(content: &str) -> std::ops::Range { + let lines: Vec<&str> = content.lines().collect(); + + let Some(h1) = lines.iter().position(|line| { + let trimmed = line.trim_start(); + trimmed.starts_with("# ") && !trimmed.starts_with("##") + }) else { + return 0..0; + }; + + let mut end = h1 + 1; + let mut saw_annotation = false; + for (offset, line) in lines.iter().enumerate().skip(h1 + 1) { + let trimmed = line.trim(); + if trimmed.is_empty() { + // A blank line may separate the H1 from the annotations, or follow + // them. Either way it does not end the block on its own. + continue; + } + if trimmed.starts_with('@') { + saw_annotation = true; + end = offset + 1; + } else if saw_annotation && line.starts_with(' ') { + // Continuation of the annotation above. + end = offset + 1; + } else { + break; + } + } + + 0..end } /// 0-indexed line holding the given byte offset diff --git a/crates/lash-core/tests/formatter_tests.rs b/crates/lash-core/tests/formatter_tests.rs index 8cf0de9..43f7786 100644 --- a/crates/lash-core/tests/formatter_tests.rs +++ b/crates/lash-core/tests/formatter_tests.rs @@ -41,7 +41,7 @@ fn test_format_basic_file() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); assert!(formatted.contains("# Test File")); assert!(formatted.contains("@id: test-file")); @@ -70,7 +70,7 @@ fn test_format_preserves_hierarchy() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Verify hierarchy is preserved with exactly 2 spaces assert!(formatted.contains("- [ ] Parent\n")); @@ -98,7 +98,7 @@ fn test_format_normalizes_indentation() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // All children should have exactly 2 spaces assert!(formatted.contains(" - [ ] Child with 2 spaces\n")); @@ -127,7 +127,7 @@ fn test_annotation_sorting() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Find positions of annotations let id_pos = formatted.find("@id:").unwrap(); @@ -162,7 +162,7 @@ fn test_annotation_sorting_disabled() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Annotations should maintain original order (after @id) let id_pos = formatted.find("@id:").unwrap(); @@ -191,7 +191,7 @@ fn test_whitespace_normalization() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // No trailing spaces assert!(!formatted.lines().any(|line| line.ends_with(' '))); @@ -225,7 +225,7 @@ fn test_auto_waive_children() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Children should be auto-waived assert!(formatted.contains("- [-] Waived parent\n")); @@ -254,7 +254,7 @@ fn test_auto_fix_status_consistency() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Parent should be unmarked (changed to open) assert!(formatted.contains("- [ ] Parent marked done\n")); @@ -282,7 +282,7 @@ fn test_auto_fix_disabled() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Without auto-fix, parent-child inconsistency remains assert!(formatted.contains("- [x] Parent done\n")); @@ -306,11 +306,11 @@ fn test_round_trip_idempotence() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted1 = formatter.format_file(&file).unwrap(); + let formatted1 = formatter.format_file("", &file).unwrap(); // Parse the formatted output let file2 = parse_file_from_string(&formatted1, &config).unwrap(); - let formatted2 = formatter.format_file(&file2).unwrap(); + let formatted2 = formatter.format_file("", &file2).unwrap(); // Second format should be identical to first assert_eq!(formatted1, formatted2); @@ -337,11 +337,11 @@ fn test_round_trip_with_metadata() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted1 = formatter.format_file(&file).unwrap(); + let formatted1 = formatter.format_file("", &file).unwrap(); // Parse and format again let file2 = parse_file_from_string(&formatted1, &config).unwrap(); - let formatted2 = formatter.format_file(&file2).unwrap(); + let formatted2 = formatter.format_file("", &file2).unwrap(); // Second format should be identical to first (idempotent) assert_eq!( @@ -371,7 +371,7 @@ fn test_format_empty_file() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); assert!(formatted.contains("# Empty")); assert!(formatted.contains("@id: empty")); @@ -395,7 +395,7 @@ fn test_format_deeply_nested() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Verify exact indentation assert!(formatted.contains("- [ ] Level 0\n")); @@ -419,7 +419,7 @@ fn test_format_with_inline_labels() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Inline labels should be preserved assert!(formatted.contains("#label1")); @@ -445,7 +445,7 @@ fn test_format_preserves_task_order() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); let first_pos = formatted.find("First").unwrap(); let second_pos = formatted.find("Second").unwrap(); @@ -477,7 +477,7 @@ fn test_format_multiple_root_tasks() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // All root tasks should be at depth 0 let lines: Vec<&str> = formatted.lines().collect(); @@ -513,7 +513,7 @@ fn test_format_mixed_statuses() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); assert!(formatted.contains("- [ ] Open")); assert!(formatted.contains("- [x] Done")); @@ -542,7 +542,7 @@ fn test_format_nested_auto_waive() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Both child and grandchild should be waived assert!(formatted.contains(" - [-] Child\n")); @@ -568,7 +568,7 @@ fn test_minimal_formatter() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Minimal formatter doesn't apply auto-fixes, so parent-child inconsistency remains // (done parent stays done even with open child when auto-fix is disabled) @@ -600,7 +600,7 @@ fn test_strict_formatter() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Strict mode: max 1 blank line assert!(!formatted.contains("\n\n\n")); @@ -617,7 +617,7 @@ fn test_format_file_ends_with_newline() { - [ ] Task"#; // No trailing newline let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); assert!(formatted.ends_with('\n')); assert!(!formatted.ends_with("\n\n")); @@ -641,7 +641,7 @@ fn test_format_with_custom_annotations() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Custom annotations should be preserved assert!(formatted.contains("@priority: high")); @@ -681,7 +681,7 @@ fn test_format_preserves_task_annotations_issue_6() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // Task-level @id annotations must be preserved assert!( @@ -754,11 +754,11 @@ fn test_round_trip_preserves_task_annotations() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted1 = formatter.format_file(&file).unwrap(); + let formatted1 = formatter.format_file("", &file).unwrap(); // Parse and format again - should be idempotent let file2 = parse_file_from_string(&formatted1, &config).unwrap(); - let formatted2 = formatter.format_file(&file2).unwrap(); + let formatted2 = formatter.format_file("", &file2).unwrap(); assert_eq!( formatted1, formatted2, @@ -791,7 +791,7 @@ fn test_format_preserves_doc_annotation_singular() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // @doc must remain singular (not become @docs) assert!( @@ -825,7 +825,7 @@ fn test_format_preserves_doc_after_contextual_notes() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // @doc must be preserved even when it appears after contextual notes assert!( @@ -867,7 +867,7 @@ fn test_format_preserves_file_level_doc_annotation() { "#; let file = parse_file_from_string(content, &config).unwrap(); - let formatted = formatter.format_file(&file).unwrap(); + let formatted = formatter.format_file("", &file).unwrap(); // File-level @doc must be preserved assert!( @@ -885,3 +885,128 @@ fn test_format_preserves_file_level_doc_annotation() { "File-level @depends-on should be preserved" ); } + +// --------------------------------------------------------------------- +// Sections the model does not represent, and idempotence +// +// `format_file` used to rebuild the file from the parsed model alone. +// The model holds the header, the Description section and the task tree +// and nothing else, so every other section was silently deleted: a file +// with `## Notes` and `## References` came back with only the header and +// the tasks, exit code 0, no warning. +// +// Separately, the parser leaves inline labels in the title as well as +// recording them in metadata, and the formatter wrote both. Each run +// appended another copy of every label, so `format --check` reported a +// file as needing formatting forever. +// --------------------------------------------------------------------- + +/// Format `content` once, the way `lash format` does. +fn format(content: &str) -> String { + let config = make_config(); + let formatter = Formatter::new(config.clone(), FormatOptions::default()); + let file = parse_file_from_string(content, &config).unwrap(); + formatter.format_file(content, &file).unwrap() +} + +#[test] +fn test_sections_after_tasks_survive() { + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n\n## Notes\n\nnotes prose\n\n## References\n\n- [link](target.md)\n"; + + let formatted = format(content); + + for expected in [ + "## Notes", + "notes prose", + "## References", + "- [link](target.md)", + ] { + assert!( + formatted.contains(expected), + "'{expected}' was deleted by formatting:\n{formatted}" + ); + } +} + +#[test] +fn test_sections_before_tasks_survive() { + let content = + "# F\n\n@id: f\n\n## Background\n\nsome background prose\n\n## Tasks\n\n- [ ] one\n"; + + let formatted = format(content); + + assert!( + formatted.contains("## Background") && formatted.contains("some background prose"), + "a section above ## Tasks was deleted:\n{formatted}" + ); + // And it must still be above the tasks, not relocated. + assert!( + formatted.find("## Background").unwrap() < formatted.find("## Tasks").unwrap(), + "section order was not preserved:\n{formatted}" + ); +} + +#[test] +fn test_a_fenced_heading_does_not_split_a_section() { + let content = + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n\n## Notes\n\n```markdown\n## Tasks\n```\n"; + + let formatted = format(content); + + assert!( + formatted.contains("```markdown\n## Tasks\n```"), + "the fenced block was treated as a real heading:\n{formatted}" + ); +} + +#[test] +fn test_inline_labels_are_not_duplicated() { + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one #docs #infra\n"; + + let formatted = format(content); + + assert_eq!( + formatted.matches("#docs").count(), + 1, + "label emitted more than once:\n{formatted}" + ); + assert_eq!( + formatted.matches("#infra").count(), + 1, + "label emitted more than once:\n{formatted}" + ); +} + +#[test] +fn test_formatting_is_idempotent() { + // The property that catches both bugs at once: whatever formatting does, + // doing it again must change nothing. + let cases = [ + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n", + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one #docs #infra\n", + "# F\n\n@id: f\n\n## Description\n\ndesc\n\n## Tasks\n\n- [ ] one\n", + "# F\n\n@id: f\n\n## Background\n\nprose\n\n## Tasks\n\n- [ ] one\n\n## Notes\n\nmore prose\n", + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] parent\n - [ ] child #api\n @agent-note: a note\n", + "# F\n\n@id: f\n@labels: a, b\n\n## Tasks\n\n- [ ] one\n", + ]; + + for content in cases { + let once = format(content); + let twice = format(&once); + assert_eq!( + once, twice, + "formatting is not idempotent for:\n{content}\nfirst pass:\n{once}\nsecond pass:\n{twice}" + ); + } +} + +#[test] +fn test_an_already_canonical_file_is_left_byte_identical() { + let content = "# F\n\n@id: f\n\n## Background\n\nsome background prose\n\n## Tasks\n\n- [ ] one #docs\n\n## Notes\n\nnotes prose\n"; + + assert_eq!( + format(content), + content, + "a canonical file must not be rewritten" + ); +} diff --git a/lash.index.md b/lash.index.md index 80910e2..24ebb25 100644 --- a/lash.index.md +++ b/lash.index.md @@ -109,7 +109,7 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. — assert byte-exact preservation of every pre-existing task and note, and that the new task lands last. Plus a round-trip property test: parse → add → parse preserves all prior notes verbatim -- [ ] Remove the `[llm]` section from `flawd.toml` (removed in Flawd v0.7.0) #flawd #config +- [x] Remove the `[llm]` section from `flawd.toml` (removed in Flawd v0.7.0) #flawd #config - Our committed `flawd.toml` still carries an `[llm]` table (`mode = "off"`, `max_edit_chars = 120`). Flawd removed LLM semantic operators in v0.7.0 and treats a lingering `[llm]` table as a hard config error, so `flawd run` on a @@ -120,7 +120,9 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. - Consider a CI job running `flawd config show` against the committed config so version drift in our own tool is caught by the build, not by a human months later -- [ ] Make `flawd.toml`'s coverage command work in container mode #flawd #docker #ci + - Done in #35. The CI job is not; it needs flawd installed on the runner, + which is a bigger change than the fix +- [x] Make `flawd.toml`'s coverage command work in container mode #flawd #docker #ci - `flawd.toml` sets coverage to `cargo llvm-cov ...`, but the Dockerfile never installs `cargo-llvm-cov`. Flawd defaults to docker isolation, so the documented `flawd run` builds the image, runs coverage inside it, and fails @@ -132,6 +134,12 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. point `[coverage] command` at a tool the image has; make the paths portable rather than machine-specific. Verify by running `flawd run` with default isolation and confirming per-test collection succeeds instead of falling back + - Done in #35. The Dockerfile installs `cargo-llvm-cov` from its prebuilt + release (arch picked by `uname -m`) plus `llvm-tools-preview`, which + supplies llvm-cov and llvm-profdata, so the command needs no path overrides + at all. The command also creates `coverage/` first, since it is gitignored + and so never exists in a fresh container. Verified with `flawd run` under + default isolation: per-test targeting 6/6, no full-suite fallback - [x] `@depends-on` comma form over-counts lines, appending outside the tasks section #cli #bug - Follow-up to the multi-line note fix, same function (`PlacementResolver::count_annotation_lines`). `count += depends_on.len()` @@ -225,7 +233,7 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. reported line now comes from where the task was actually written, so the `:0` in the success message is gone too, as is the missing trailing newline noted under the ID ticket below -- [ ] `lash format` deletes every section after `## Tasks` #cli #bug #data-loss #launch-blocker +- [x] `lash format` deletes every section after `## Tasks` #cli #bug #data-loss #launch-blocker - Worse than the `lash add` bugs it was found next to, because `format` is documented as a normalizer and the README tells people to run it. Found 2026-08-09 while checking that the empty-Tasks-section fix left files @@ -248,7 +256,14 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. and assert byte-exact preservation of both; a file with a trailing section containing a fenced code block; and a round-trip property test asserting `format(format(x)) == format(x)` -- [ ] `lash format` duplicates inline labels on every run #cli #bug + - Turned out to be broader than filed: sections *before* `## Tasks` were + destroyed too (a `## Background` between the header and the tasks), because + the parser folds them into "overview" text that `TaskFile` never stored. + Fixed by taking the source alongside the parsed file: `format_file` now + regenerates only the spans it owns (the H1 plus annotation block, the + Description section, the Tasks section) and copies every other line + through unchanged +- [x] `lash format` duplicates inline labels on every run #cli #bug - Not idempotent, and the file grows without bound. `- [ ] task one #docs #infra` becomes `#docs #infra #docs #infra` after one run and `#docs #infra #docs #infra #docs #infra` after two @@ -263,6 +278,10 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. the title verbatim and skip the label pass. Whichever, add the `format(format(x)) == format(x)` property test above; it catches this and the section-deletion bug at once + - Fixed on the emit side: the formatter strips inline labels from the title + and writes the sorted metadata list as the single source. Leaving them in + the parsed title keeps `lash list` and search working the way they do + today. The idempotence property test covers both bugs, as predicted - [x] `lash add` reports a task ID that does not match the indexed one #cli #bug #ux - The ID printed on creation cannot be used with `lash show` / `lash complete` / `@depends-on`, so any workflow that copies it fails. Observed repeatedly @@ -300,7 +319,7 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. synthesized id needs updating; explicit `@id:` values are untouched - The trailing-newline half was fixed with the empty-Tasks-section ticket above, since it lives in the same function -- [ ] `test_user_config_save_and_load` writes to the real `~/.lash/config.toml` and can leave the CLI broken #testing #bug #isolation +- [x] `test_user_config_save_and_load` writes to the real `~/.lash/config.toml` and can leave the CLI broken #testing #bug #isolation - A unit test in `crates/lash-types/src/config.rs` (`test_user_config_save_and_land` at ~L727) constructs a `UserConfig` with `color_scheme = "Test Theme"` and calls `config.save()`, which writes the developer's REAL home-directory @@ -327,3 +346,12 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. still skips it, and it still cannot restore settings it never captured - Audit the rest of the suite for the same pattern: `crates/lash-cli/tests/` has several files referencing `user_config_path` + - Done in #34. `UserConfig::load_from(path)` / `save_to(path)` take the path + explicitly and the tests point at a `TempDir`; `load()` and `save()` are + thin wrappers, so production behaviour is unchanged. Verified by running + the full suite and confirming the real `~/.lash/config.toml` came out + byte-identical + - Audit found nothing else. lash-cli's `Config::user_config_path` resolves a + different location (`dirs::config_dir`) and its test only inspects the + path; lash-tui's `apply_selected_theme` calls `save()` for real, which is + correct, and no test exercises it