diff --git a/crates/lash-cli/tests/format_output_tests.rs b/crates/lash-cli/tests/format_output_tests.rs index dc3e6c4..f5664eb 100644 --- a/crates/lash-cli/tests/format_output_tests.rs +++ b/crates/lash-cli/tests/format_output_tests.rs @@ -1202,3 +1202,87 @@ fn test_format_json_mode_summary_has_correct_structure() { ); } } + +// --------------------------------------------------------------------------- +// Content inside `## Tasks` that the model does not represent +// +// `format` rebuilt the `## Tasks` span from the parsed model, which holds +// checkbox lines, their annotations and the first line of each contextual +// note. A task's free-text body, a `---` separator and a `### Subsection` +// heading had nothing to be rebuilt from and were deleted on disk, exit +// code 0 and a "Formatted 1 file(s) successfully" message. +// --------------------------------------------------------------------------- + +/// A file whose `## Tasks` section carries content the model cannot hold. +const TASKS_WITH_BODIES: &str = "# Task List\n\n@id: example\n@created: 2024-01-15\n\n## Tasks\n\n### Group one\n\n- [ ] First task\n Body prose belonging to the first task.\n 1. First numbered point.\n 2. Second numbered point.\n Acceptance: this paragraph stays with the first task.\n\n---\n\n- [x] Done task\n - a note that wraps\n onto a second line\n"; + +#[test] +fn test_format_does_not_delete_task_bodies_on_disk() { + let td = TempDir::new().unwrap(); + let path = write_md(&td, "lash.index.md", TASKS_WITH_BODIES); + + let output = lash() + .arg("format") + .arg(&path) + .output() + .expect("lash must run"); + + assert_eq!( + output.status.code().unwrap_or(-1), + 0, + "format must succeed; stderr={}", + String::from_utf8_lossy(&output.stderr) + ); + + let after = fs::read_to_string(&path).expect("file must still be readable"); + + for expected in [ + "### Group one", + " Body prose belonging to the first task.", + " 1. First numbered point.", + " 2. Second numbered point.", + " Acceptance: this paragraph stays with the first task.", + "---", + " - a note that wraps", + " onto a second line", + ] { + assert!( + after.contains(expected), + "`lash format` deleted '{expected}' from the file:\n{after}" + ); + } +} + +#[test] +fn test_format_leaves_a_file_with_bodies_byte_identical() { + // Nothing in this file needs normalizing, so `format` must not touch it — + // and `--check` must agree that it is already formatted. + let td = TempDir::new().unwrap(); + let path = write_md(&td, "lash.index.md", TASKS_WITH_BODIES); + + lash() + .arg("format") + .arg(&path) + .output() + .expect("lash must run"); + + assert_eq!( + fs::read_to_string(&path).unwrap(), + TASKS_WITH_BODIES, + "a canonical file with bodies was rewritten" + ); + + let check = lash() + .arg("format") + .arg("--check") + .arg(&path) + .output() + .expect("lash must run"); + + assert_eq!( + check.status.code().unwrap_or(-1), + 0, + "--check must report a file with bodies as already formatted; stderr={}", + String::from_utf8_lossy(&check.stderr) + ); +} diff --git a/crates/lash-core/src/formatter/mod.rs b/crates/lash-core/src/formatter/mod.rs index 56539bf..6fdb79f 100644 --- a/crates/lash-core/src/formatter/mod.rs +++ b/crates/lash-core/src/formatter/mod.rs @@ -49,6 +49,21 @@ use std::path::Path; pub use options::FormatOptions; +/// Something the formatter regenerates, found at a source line +/// +/// Everything inside `## Tasks` that is not one of these is copied through +/// from the source, because the model does not represent it. +enum Anchor<'a> { + /// A task's checkbox line, which also carries its annotation block. + Task(&'a lash_types::Task), + + /// A single contextual-note bullet, with the depth of the task it belongs + /// to. Notes are anchored individually: only a note's first line lives in + /// the model, so its wrapped continuation lines are copied through and the + /// first line has to stay in front of them. + Note(usize, &'a lash_types::task::ContextualNote), +} + /// Formatter for Lash task files /// /// The formatter takes a parsed `TaskFile` and produces formatted Markdown @@ -170,7 +185,14 @@ impl Formatter { 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); + // The body is walked rather than regenerated: the model holds + // the checkbox lines, their annotations and their contextual + // notes, and nothing else in the section. Bodies, separators + // and anything else present are copied through. + let body = tasks.as_ref().map_or(line + 1..line + 1, |span| { + (span.start + 1).min(span.end)..span.end + }); + self.format_tasks(&file, Some((&lines[body.clone()], body.start)), &mut output); output.push('\n'); line = tasks.as_ref().map_or(line + 1, |span| span.end); skip_blanks(&mut line); @@ -188,7 +210,7 @@ impl Formatter { if description.is_none() && file.description.is_some() { Self::format_description(&file, &mut output); } - self.format_tasks(&file, &mut output); + self.format_tasks(&file, None, &mut output); } // Normalize whitespace. This only collapses runs of blank lines and @@ -540,17 +562,121 @@ impl Formatter { } /// Format the tasks section - fn format_tasks(&self, file: &TaskFile, output: &mut String) { + /// + /// `body` is the source of the section below the heading, as the lines + /// themselves plus the 0-indexed position of the first of them. Given it, + /// the section is walked rather than rebuilt. + /// + /// Rebuilding was the bug. A parsed task carries its checkbox line, its + /// annotations and its contextual notes, and the model holds nothing else + /// that lives inside `## Tasks`. Everything else the section contained — + /// a task's free-text body of prose, numbered steps or acceptance + /// criteria, a `---` separator, a comment — had no representation to be + /// rebuilt from and was deleted, with exit code 0. #44 stopped `format` + /// from dropping whole sections the same way; this is the same fix + /// applied one level down, to the one section the formatter does own. + /// + /// So the formatter regenerates only the lines it can account for and + /// copies every other line through. Without a source (a file whose + /// `## Tasks` heading is missing entirely, which `format` adds) there is + /// nothing to walk and the whole section is generated from the model. + fn format_tasks(&self, file: &TaskFile, body: Option<(&[&str], usize)>, output: &mut String) { output.push_str("## Tasks\n"); output.push('\n'); - // Get all root tasks (tasks with no parent) let all_tasks = file.tasks.tasks(); - let root_tasks: Vec<_> = all_tasks.iter().filter(|t| t.parent_id.is_none()).collect(); - // Format each root task and its descendants - for task in root_tasks { - self.format_task(task, all_tasks, output); + let Some((body_lines, body_start)) = body else { + for task in all_tasks.iter().filter(|t| t.parent_id.is_none()) { + self.format_task(task, all_tasks, output); + } + return; + }; + + // 1-indexed source lines the walk covers. + let walked = body_start + 1..=body_start + body_lines.len(); + + // What the formatter regenerates, keyed by the source line it starts + // on. A task's checkbox line and each of its contextual notes are + // separate anchors: a note is regenerated where it sits, not alongside + // the task, because only a note's *first* line is in the model. Its + // wrapped continuation lines are copied through, so hoisting the first + // line up to the task would strand the rest of the note behind it. + let mut starts_at: std::collections::HashMap = + std::collections::HashMap::new(); + + // Annotation lines, which the task's own anchor regenerates. + let mut owned: std::collections::HashSet = std::collections::HashSet::new(); + + for task in all_tasks { + if !walked.contains(&task.line_number) { + continue; + } + starts_at.insert(task.line_number, Anchor::Task(task)); + for offset in 1..=task.annotation_line_count { + owned.insert(task.line_number + offset); + } + for note in &task.contextual_notes { + if walked.contains(¬e.line_number()) { + starts_at.insert(note.line_number(), Anchor::Note(task.depth as usize, note)); + } + } + } + + let mut emitted: std::collections::HashSet<&str> = std::collections::HashSet::new(); + + // Blank lines bounding the section are the separators around it, and + // both ends are written by the caller — the heading above carries its + // own, and the section is followed by one. Copying them through as + // well would double them on every run. + let leading_blanks = body_lines + .iter() + .position(|line| !line.trim().is_empty()) + .unwrap_or(body_lines.len()); + let body_end = body_lines + .iter() + .rposition(|line| !line.trim().is_empty()) + .map_or(leading_blanks, |last| last + 1); + + for (offset, line) in body_lines[..body_end] + .iter() + .enumerate() + .skip(leading_blanks) + { + let line_number = body_start + offset + 1; + + match starts_at.get(&line_number) { + Some(Anchor::Task(task)) => { + self.format_task_head(task, output); + // A note with no line the walk will reach cannot be placed, + // so it goes with its task rather than being dropped. + for note in &task.contextual_notes { + if !walked.contains(¬e.line_number()) { + self.format_contextual_note(task.depth as usize, note, output); + } + } + emitted.insert(task.id.as_str()); + } + Some(Anchor::Note(depth, note)) => { + self.format_contextual_note(*depth, note, output); + } + None if !owned.contains(&line_number) => { + output.push_str(line); + output.push('\n'); + } + None => {} + } + } + + // A task the walk never reached has no source line to have been found + // at — it was built in memory, or `format_file` was handed a source + // that is not the one the file was parsed from. Dropping it would be + // the very data loss this walk exists to prevent, so it goes at the + // end of the section. + for task in all_tasks { + if !emitted.contains(task.id.as_str()) { + self.format_task_entry(task, output); + } } } @@ -561,6 +687,45 @@ impl Formatter { all_tasks: &[lash_types::Task], output: &mut String, ) { + self.format_task_entry(task, output); + + let children: Vec<_> = all_tasks + .iter() + .filter(|child| child.parent_id.as_deref() == Some(&task.id)) + .collect(); + + for child in children { + self.format_task(child, all_tasks, output); + } + } + + /// Format one task's own lines: checkbox, annotations, contextual notes + /// + /// Children are not included. Walking the source reaches each task at its + /// own line, so recursing here would emit a subtree twice. + fn format_task_entry(&self, task: &lash_types::Task, output: &mut String) { + self.format_task_head(task, output); + for note in &task.contextual_notes { + self.format_contextual_note(task.depth as usize, note, output); + } + } + + /// Format one contextual note as a plain bullet + fn format_contextual_note( + &self, + depth: usize, + note: &lash_types::task::ContextualNote, + output: &mut String, + ) { + let indent_spaces = self.options.indent_spaces as usize; + output.push_str(&" ".repeat((depth + 1) * indent_spaces)); + output.push_str("- "); + output.push_str(note.text()); + output.push('\n'); + } + + /// Format a task's checkbox line and its annotation block + fn format_task_head(&self, task: &lash_types::Task, output: &mut String) { // Calculate indentation let indent_spaces = self.options.indent_spaces as usize; let indent = " ".repeat(task.depth as usize * indent_spaces); @@ -595,24 +760,6 @@ impl Formatter { // Format task-level annotations (indented one level deeper) self.format_task_annotations(task, &annotation_indent, output); - - // Format contextual notes as plain bullet points - for note in &task.contextual_notes { - output.push_str(&annotation_indent); - output.push_str("- "); - output.push_str(note.text()); - output.push('\n'); - } - - // Format children - let children: Vec<_> = all_tasks - .iter() - .filter(|child| child.parent_id.as_deref() == Some(&task.id)) - .collect(); - - for child in children { - self.format_task(child, all_tasks, output); - } } /// Format task-level annotations diff --git a/crates/lash-core/tests/formatter_tests.rs b/crates/lash-core/tests/formatter_tests.rs index 43f7786..d37af0e 100644 --- a/crates/lash-core/tests/formatter_tests.rs +++ b/crates/lash-core/tests/formatter_tests.rs @@ -1010,3 +1010,148 @@ fn test_an_already_canonical_file_is_left_byte_identical() { "a canonical file must not be rewritten" ); } + +// --------------------------------------------------------------------- +// Content inside `## Tasks` that the model does not represent +// +// The same bug as above, one level down. #44 stopped `format` from +// deleting whole sections, but the `## Tasks` span was still rebuilt from +// the model — which holds checkbox lines, their annotations and the first +// line of each contextual note, and nothing else that lives in the +// section. A task's free-text body, a `---` separator, a comment, a +// `### Subsection` heading and the wrapped continuation lines of every +// note were all deleted, exit code 0. +// --------------------------------------------------------------------- + +#[test] +fn test_a_tasks_free_text_body_survives() { + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n Body prose for one.\n 1. First step.\n 2. Second step.\n Acceptance: this stays.\n"; + + let formatted = format(content); + + for expected in [ + " Body prose for one.", + " 1. First step.", + " 2. Second step.", + " Acceptance: this stays.", + ] { + assert!( + formatted.contains(expected), + "'{expected}' was deleted by formatting:\n{formatted}" + ); + } +} + +#[test] +fn test_a_body_stays_with_the_task_it_belongs_to() { + let content = + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n Body of one.\n- [ ] two\n Body of two.\n"; + + let formatted = format(content); + + assert_eq!( + formatted, content, + "bodies were reordered or dropped:\n{formatted}" + ); +} + +#[test] +fn test_separators_comments_and_subheadings_inside_tasks_survive() { + let content = "# F\n\n@id: f\n\n## Tasks\n\n### Group one\n\n- [ ] one\n\n---\n\n### Group two\n\n\n\n- [ ] two\n"; + + let formatted = format(content); + + for expected in [ + "### Group one", + "---", + "### Group two", + "", + ] { + assert!( + formatted.contains(expected), + "'{expected}' was deleted by formatting:\n{formatted}" + ); + } +} + +#[test] +fn test_a_wrapped_contextual_note_is_not_split_from_its_own_text() { + // Only a note's first line is in the model. Regenerating notes alongside + // their task hoisted that line above the wrapped remainder, scrambling + // every multi-line note in the file. + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n - first note, which wraps onto\n a second line and then\n a third\n - second note, also wrapping\n onto another line\n"; + + let formatted = format(content); + + assert_eq!( + formatted, content, + "a wrapped note was reordered or dropped:\n{formatted}" + ); +} + +#[test] +fn test_the_formatter_still_normalizes_what_it_owns_around_a_body() { + // Copying unowned lines through must not stop the formatter fixing the + // lines it does own: statuses, label order, trailing whitespace. + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [-] parent\n Body under the parent.\n - [ ] child #b #a \n Body under the child.\n"; + + let formatted = format(content); + + assert!( + formatted.contains("- [-] child #a #b\n"), + "the child was not auto-waived, sorted and trimmed:\n{formatted}" + ); + assert!( + formatted.contains(" Body under the parent.") + && formatted.contains(" Body under the child."), + "a body was lost while normalizing:\n{formatted}" + ); +} + +#[test] +fn test_blank_lines_between_tasks_are_left_alone() { + // They are the author's, not the formatter's. `lash add` writes one when + // it appends below a task that has a body (issue #48). + let content = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n Body of one.\n\n- [ ] two\n"; + + assert_eq!(format(content), content, "a blank separator was removed"); +} + +#[test] +fn test_formatting_is_idempotent_over_unrepresented_content() { + let cases = [ + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n Body prose.\n 1. A step.\n", + "# F\n\n@id: f\n\n## Tasks\n\n### Group\n\n---\n\n- [ ] one\n\n\n", + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n - a note that wraps\n onto a second line\n", + "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n @owner: alice\n - a note\n\n- [ ] two\n Body.\n\n## Notes\n\nprose\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_no_task_is_dropped_when_the_source_does_not_match_the_model() { + // `format_file` takes the source and the parsed model separately, so a + // caller can hand it a source the model did not come from. The walk finds + // no task at any line; they must still all be written out. + let config = make_config(); + let formatter = Formatter::new(config.clone(), FormatOptions::default()); + + let parsed = "# F\n\n@id: f\n\n## Tasks\n\n- [ ] one\n- [ ] two\n"; + let file = parse_file_from_string(parsed, &config).unwrap(); + + let other_source = "# F\n\n@id: f\n\n## Tasks\n\nprose where the tasks were\n"; + let formatted = formatter.format_file(other_source, &file).unwrap(); + + assert!( + formatted.contains("- [ ] one") && formatted.contains("- [ ] two"), + "a task in the model was dropped:\n{formatted}" + ); +} diff --git a/devlog.md b/devlog.md index f37caf0..9bffe20 100644 --- a/devlog.md +++ b/devlog.md @@ -2561,8 +2561,56 @@ now gets a blank line above it, because butting it against the tail of someone else's prose reads as more of that prose — the confusion the insertion point exists to avoid. Tasks with no body still sit flush against each other. -Still open, same root cause: `lash format` deletes task bodies outright. The -`## Tasks` span is regenerated from the model, and the model has no body to -regenerate. #44 stopped `format` from deleting whole sections but the span it -owns still loses anything the parser did not record. Fixing that needs the -model to carry bodies, which is a larger change than this one. +Same root cause, fixed next: `lash format` deleted task bodies outright. + +## `lash format` deleted everything inside `## Tasks` it could not rebuild (2026-08-10) + +Found while fixing the `add` placement bug above, and worse than it: `add` +misattributes a body, `format` destroys it, and the README tells people to run +`format`. + +#44 stopped `format` from deleting whole sections by having it regenerate only +the spans the model owns and copy the rest through. But the `## Tasks` span was +still rebuilt wholesale from the task tree, and the tree holds checkbox lines, +their annotations, and the first line of each contextual note. Nothing else +that lives in the section had anything to be rebuilt from. + +The repro was a task body. The actual blast radius was larger: + +- free-text bodies — prose, numbered steps, acceptance criteria +- `---` separators and comments +- `### Subsection` headings, which `section_span` explicitly supports and files + routinely use to group tasks +- the wrapped continuation lines of every contextual note, since + `ContextualNote` records only the note's first line + +Formatting `lash.index.md` used to produce a 238-line diff, most of it +deletion. It is 52 lines now, all of it label sorting. + +The fix is #44's rule applied one level down: walk the section's source, +regenerate the lines the model can account for, copy every other line through. +No new model state — extending `TaskFile` to carry bodies would have been a +fourth thing to keep in sync with the source, which is the shape of bug this +codebase keeps paying for. + +The note handling is the part worth remembering. Emitting a task's notes +alongside the task looks right and is wrong: only a note's first line is in the +model, so the continuation lines get copied through where they sit while the +first line is hoisted up to the task, stranding each note's text behind an +unrelated bullet. Notes are anchored individually at their own source line +instead. This was invisible in the small repro and obvious the moment the +formatter was pointed at a real file. + +Two guards on the walk. A task the walk never finds is written at the end of +the section rather than dropped, which covers a caller handing `format_file` a +source the model did not come from. And blank lines bounding the section are +not copied, because the caller writes those separators — copying them too added +a blank line per run and formatting stopped being idempotent. + +One deliberate behavior change: blank lines *between* tasks now survive. +They belong to the author, and `lash add` writes one when it appends below a +task with a body. + +The lint rule suggested on #48 is still not worth building. A misattributed +body is syntactically indistinguishable from a correct one, so there is nothing +for the linter to check against. diff --git a/lash.index.md b/lash.index.md index 65cfe42..fcf98ed 100644 --- a/lash.index.md +++ b/lash.index.md @@ -390,7 +390,7 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. line matches the file), plus bodies split into paragraphs, contextual note bullets, and unit coverage of the anchor walk including the cases that must still stop it -- [ ] `lash format` deletes task bodies inside `## Tasks` #cli #bug #data-loss +- [x] `lash format` deletes task bodies inside `## Tasks` #cli #bug #data-loss - Same root cause as the ticket above, found while fixing it. #44 stopped `format` from deleting whole sections, but the `## Tasks` span is still regenerated from the model, and the model has no body to regenerate. A task @@ -400,10 +400,24 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. body is gone. The `---` separator inside the section goes too - Worse than the `add` bug it was found next to: `add` misattributes a body, `format` destroys it, and the README tells people to run `format` - - Fix needs `TaskFile` to carry the body lines the parser currently drops, so - the formatter can copy them through the same way it now copies unowned - sections. Larger than a placement fix — filing rather than folding in - - A lint rule was suggested on #48 for hand-edited files. Worth revisiting - once the model carries bodies; until then the linter has nothing to check - against, since a misattributed body is syntactically indistinguishable from - a correct one + - Scope was bigger than the repro showed. `### Subsection` headings inside + `## Tasks` went the same way, and so did the wrapped continuation lines of + every contextual note — `ContextualNote` holds only a note's first line. + Formatting this very file used to produce a 238-line diff, most of it + deletion; it is 52 lines now, all of it label sorting + - Fixed without extending the model. The formatter now walks the section's + source and regenerates only the lines it can account for — a task's + checkbox line, its annotation block, the first line of each note — copying + every other line through. That is exactly what #44 did one level up, so + `format` is now the same rule applied at both levels: rebuild what the + model owns, carry everything else + - Notes are anchored individually, at their own source line, rather than + emitted alongside their task. Emitting them with the task hoists each + note's first line above the wrapped remainder and scrambles it, which is + how the note handling was found in the first place + - A task the walk never finds — built in memory, or a `format_file` caller + passing a source the model did not come from — is still written out at the + end of the section rather than dropped + - A lint rule was suggested on #48 for hand-edited files. Still not worth + it: a misattributed body is syntactically indistinguishable from a correct + one, so there is nothing for the linter to check against