From 807e4aefaacb481ee5b588cc55a3a72b59b23fe7 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Mon, 10 Aug 2026 09:56:37 -0600 Subject: [PATCH] fix(add): stop inserting a task between the previous task's title and its body The parser records a task's checkbox and annotation lines but not its free-text body, so the emitter now walks the source past the whole block before inserting. Closes #48. --- crates/lash-cli/tests/add_command_test.rs | 137 ++++++++++++++++ crates/lash-core/src/creation/emitter.rs | 174 ++++++++++++++++++++- crates/lash-core/src/creation/placement.rs | 26 ++- devlog.md | 48 ++++++ lash.index.md | 52 ++++++ 5 files changed, 429 insertions(+), 8 deletions(-) diff --git a/crates/lash-cli/tests/add_command_test.rs b/crates/lash-cli/tests/add_command_test.rs index 3210e0f..113b9b9 100644 --- a/crates/lash-cli/tests/add_command_test.rs +++ b/crates/lash-cli/tests/add_command_test.rs @@ -795,6 +795,143 @@ Prose that must stay under its heading. ); } +// --------------------------------------------------------------------- +// A task's body must stay with its task (GitHub issue #48) +// +// The parser records a task's checkbox line and its annotation lines and +// nothing else, so free-text body under a task — prose, numbered steps, +// acceptance criteria — was invisible when the insertion point was +// computed. `lash add` landed the new task between the previous task's +// title and its own body, silently reassigning that body to the new task. +// `lash lint` passes on the result and `lash show` does not print bodies, +// so nothing surfaced the damage. +// --------------------------------------------------------------------- + +#[test] +fn test_add_does_not_steal_the_previous_tasks_body() { + let project = TestProject::builder() + .with_index("test-project", "Test Project") + .with_file( + "polish.md", + r#"# Repro + +@id: polish + +## Tasks + +- [x] First task @id:rep-first + A one-line body belonging to the first task. + +- [x] Second task @id:rep-second + This body belongs to the SECOND task and must stay attached to it. + 1. First numbered point. + 2. Second numbered point. + Acceptance: this paragraph still reads as part of the second task. +"#, + ) + .build(); + index(&project); + + let stdout = add_task(&project, "New task", "polish.md"); + + let content = fs::read_to_string(project.file_path("polish.md")).unwrap(); + + let task = task_line_number(&content, "- [ ] New task"); + assert!( + task > task_line_number(&content, "Acceptance: this paragraph"), + "the new task split the second task from its body:\n{content}" + ); + assert!( + content.contains( + "- [x] Second task @id:rep-second\n This body belongs to the SECOND task and must stay attached to it." + ), + "the second task lost its body:\n{content}" + ); + + // The reported line must be where the task actually is, not where the + // resolver first guessed. + let reported: usize = stdout + .lines() + .find_map(|line| line.rsplit_once(':')) + .and_then(|(_, line_num)| line_num.trim().parse().ok()) + .unwrap_or_else(|| panic!("no line number in:\n{stdout}")); + assert_eq!(reported, task, "reported line disagrees with the file"); +} + +#[test] +fn test_add_does_not_steal_a_body_split_into_paragraphs() { + // A blank line inside a body does not end the block; indented content + // resumes after it. + let project = TestProject::builder() + .with_index("test-project", "Test Project") + .with_file( + "tasks.md", + r#"# Tasks + +@id: tasks + +## Tasks + +- [ ] Only task + First paragraph of the body. + + Second paragraph of the body. + +## Notes + +Prose that must stay under its heading. +"#, + ) + .build(); + index(&project); + + add_task(&project, "Second task", "tasks.md"); + + let content = fs::read_to_string(project.file_path("tasks.md")).unwrap(); + + let task = task_line_number(&content, "- [ ] Second task"); + assert!( + task > task_line_number(&content, "Second paragraph of the body."), + "the new task split the body's paragraphs:\n{content}" + ); + assert!( + task < task_line_number(&content, "## Notes"), + "task escaped the Tasks section:\n{content}" + ); +} + +#[test] +fn test_add_does_not_steal_a_tasks_contextual_notes() { + // Indented plain bullets are contextual notes on the task above them. + let project = TestProject::builder() + .with_index("test-project", "Test Project") + .with_file( + "tasks.md", + r#"# Tasks + +@id: tasks + +## Tasks + +- [ ] Only task + - a contextual note + - another contextual note +"#, + ) + .build(); + index(&project); + + add_task(&project, "Second task", "tasks.md"); + + let content = fs::read_to_string(project.file_path("tasks.md")).unwrap(); + + assert!( + task_line_number(&content, "- [ ] Second task") + > task_line_number(&content, "- another contextual note"), + "the new task was spliced into the note bullets:\n{content}" + ); +} + // --------------------------------------------------------------------- // The reported task ID must be the indexed one // diff --git a/crates/lash-core/src/creation/emitter.rs b/crates/lash-core/src/creation/emitter.rs index c951389..80b3bd5 100644 --- a/crates/lash-core/src/creation/emitter.rs +++ b/crates/lash-core/src/creation/emitter.rs @@ -12,7 +12,7 @@ use std::path::{Path, PathBuf}; use super::placement::{InsertAnchor, PlacementInfo}; use super::validation::ValidationContext; -use crate::parser::header; +use crate::parser::{checkbox, header}; /// Markdown emitter for task creation /// @@ -161,6 +161,17 @@ impl MarkdownEmitter { // Copy lines before insertion point new_lines.extend_from_slice(&lines[..insert_idx]); + // Separate the new task from a preceding task's body. Landing directly + // under the last line of someone else's prose reads as a continuation + // of it, which is the confusion this insertion point exists to avoid. + // Tasks with no body keep butting up against each other as before. + let follows_body = insert_idx + .checked_sub(1) + .is_some_and(|prev| Self::continues_task_block(lines[prev])); + if follows_body { + new_lines.push(""); + } + // Insert task line new_lines.push(task_line); @@ -181,7 +192,11 @@ impl MarkdownEmitter { updated.push('\n'); Self::write_file_atomic(file_path, &updated)?; - Ok(insert_idx + 1) + Ok(if follows_body { + insert_idx + 2 + } else { + insert_idx + 1 + }) } /// Translate a [`InsertAnchor`] into a 0-indexed position in `lines` @@ -195,6 +210,10 @@ impl MarkdownEmitter { fn resolve_insert_index(content: &str, lines: &[&str], anchor: InsertAnchor) -> usize { match anchor { InsertAnchor::Line(line_number) => line_number.saturating_sub(1).min(lines.len()), + InsertAnchor::AfterTaskBlock(line_number) => { + let start = line_number.saturating_sub(1).min(lines.len()); + Self::skip_task_body(lines, start) + } InsertAnchor::EndOfTasksSection => { // No `## Tasks` heading at all: append at the end of the file // rather than guessing where the section would have been. @@ -217,6 +236,65 @@ impl MarkdownEmitter { } } + /// Advance past the body lines that belong to the preceding task + /// + /// A parsed task records its checkbox line and its annotation lines and + /// nothing else, so the free-text body a task may carry underneath — + /// prose, numbered steps, acceptance criteria, indented contextual note + /// bullets — is invisible to the placement resolver. Anchoring on parsed + /// data alone therefore lands "after this task" between the task's title + /// and its own body, which silently reassigns that body to the newly + /// inserted task (GitHub issue #48). Only the source text says where the + /// block actually ends. + /// + /// A line continues the block when it is indented and is not itself a + /// checkbox; a checkbox at any depth starts a new task, and an + /// unindented line has left the block. Blank lines are consumed only when + /// indented content resumes after them, so a body split into paragraphs + /// stays whole while a blank line that genuinely ends the block still + /// stops the scan. + /// + /// `start` is a 0-indexed position in `lines`; the return value is the + /// 0-indexed position to insert at. + fn skip_task_body(lines: &[&str], start: usize) -> usize { + let mut idx = start; + + while idx < lines.len() { + if lines[idx].trim().is_empty() { + // A blank line only belongs to the block if the block resumes + // after it. Otherwise the block ended at this blank line. + let resumes = lines[idx..] + .iter() + .position(|line| !line.trim().is_empty()) + .map(|offset| idx + offset) + .filter(|&next| Self::continues_task_block(lines[next])); + + match resumes { + Some(next) => idx = next, + None => break, + } + } else if Self::continues_task_block(lines[idx]) { + idx += 1; + } else { + break; + } + } + + idx + } + + /// Does this line continue the block that a task's checkbox line opened? + /// + /// Indented non-checkbox content belongs to the task above it: body prose, + /// folded annotation values, contextual note bullets. A checkbox at any + /// depth starts a new task, and an unindented line has left the block + /// entirely. + fn continues_task_block(line: &str) -> bool { + !line.trim().is_empty() + && line.starts_with(char::is_whitespace) + && checkbox::CheckboxLine::parse(line, 1).is_none() + } + /// Create a new task file with proper structure /// /// Generates a complete task file with header, metadata, and the first task. @@ -986,6 +1064,98 @@ mod tests { assert_eq!(insert_index(content, InsertAnchor::Line(99)), 3); } + #[test] + fn test_after_task_block_steps_over_the_previous_tasks_body() { + // GitHub issue #48: the parser records the checkbox line and nothing + // about the body under it, so the resolver anchors on line 6 — between + // the task's title and its own body. The body must stay with its task. + let content = "# T\n\n## Tasks\n\n- [x] Second task\n Body prose.\n 1. A step.\n Acceptance: still the second task's.\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 8); + } + + #[test] + fn test_after_task_block_keeps_a_body_split_into_paragraphs_whole() { + // A blank line inside a body is not the end of the block: indented + // content resumes after it. + let content = + "# T\n\n## Tasks\n\n- [x] One\n First paragraph.\n\n Second paragraph.\n\n## Notes\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 8); + } + + #[test] + fn test_after_task_block_stops_at_a_blank_line_that_ends_the_block() { + // Nothing indented follows the blank, so the block really did end. + let content = "# T\n\n## Tasks\n\n- [x] One\n Body.\n\nUnindented prose.\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 6); + } + + #[test] + fn test_after_task_block_stops_at_the_next_checkbox() { + // An indented checkbox is a child task, not body text. + let content = "# T\n\n## Tasks\n\n- [x] One\n - [ ] Child\n- [ ] Two\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 5); + } + + #[test] + fn test_after_task_block_leaves_bodyless_tasks_where_they_were() { + // The common case must resolve exactly like `Line` did before. + let content = "# T\n\n## Tasks\n\n- [ ] One\n- [ ] Two\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 5); + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(7)), 6); + } + + #[test] + fn test_after_task_block_absorbs_indented_note_bullets() { + // Contextual note bullets belong to the task above them. + let content = "# T\n\n## Tasks\n\n- [x] One\n - a note\n - another note\n"; + assert_eq!(insert_index(content, InsertAnchor::AfterTaskBlock(6)), 7); + } + + /// Insert `task_line` into `content` at `anchor`, returning the new file + /// text and the 1-indexed line the task landed on. + fn insert_at(content: &str, anchor: InsertAnchor, task_line: &str) -> (String, usize) { + let temp_dir = + std::env::temp_dir().join(format!("lash-test-insert-{:p}", content.as_ptr())); + let _ = fs::remove_dir_all(&temp_dir); + fs::create_dir_all(&temp_dir).unwrap(); + let file_path = temp_dir.join("tasks.md"); + fs::write(&file_path, content).unwrap(); + + let placement = PlacementInfo { + anchor, + order_index: 0, + indent_level: 0, + }; + let line = MarkdownEmitter::insert_into_existing(&file_path, task_line, &[], &placement) + .expect("insert must succeed"); + let updated = fs::read_to_string(&file_path).unwrap(); + let _ = fs::remove_dir_all(&temp_dir); + (updated, line) + } + + #[test] + fn test_insert_after_a_body_separates_the_new_task_from_it() { + // GitHub issue #48's expected output: the new task sits below the whole + // block, with a blank line so it does not read as more of that prose. + let content = "# T\n\n## Tasks\n\n- [x] One\n Body prose.\n Acceptance: mine.\n"; + let (updated, line) = insert_at(content, InsertAnchor::AfterTaskBlock(6), "- [ ] New task"); + + assert_eq!( + updated, + "# T\n\n## Tasks\n\n- [x] One\n Body prose.\n Acceptance: mine.\n\n- [ ] New task\n" + ); + assert_eq!(line, 9, "reported line must be where the task really is"); + } + + #[test] + fn test_insert_after_a_bodyless_task_adds_no_separator() { + let content = "# T\n\n## Tasks\n\n- [ ] One\n"; + let (updated, line) = insert_at(content, InsertAnchor::AfterTaskBlock(6), "- [ ] New task"); + + assert_eq!(updated, "# T\n\n## Tasks\n\n- [ ] One\n- [ ] New task\n"); + assert_eq!(line, 6); + } + /// Emit `note` as annotation lines, then read it back the way the parser /// would when it encounters those lines under a task. fn round_trip_agent_note(indent: usize, note: &str) -> Option { diff --git a/crates/lash-core/src/creation/placement.rs b/crates/lash-core/src/creation/placement.rs index fd82f9e..708547c 100644 --- a/crates/lash-core/src/creation/placement.rs +++ b/crates/lash-core/src/creation/placement.rs @@ -23,6 +23,16 @@ pub enum InsertAnchor { /// A concrete 1-indexed source line; the task is inserted before it. Line(usize), + /// Just past a task's block, starting the search at this 1-indexed line. + /// + /// Used when appending after an existing task. The line is the first one + /// the new task could take according to parsed data, but the parser + /// records only a task's checkbox line and its annotation lines — a + /// free-text body underneath (prose, numbered steps, acceptance criteria) + /// is invisible here. The emitter advances past that body before + /// inserting; see [`super::emitter::MarkdownEmitter`]. + AfterTaskBlock(usize), + /// After the last content line of the `## Tasks` section. /// /// Used when the section has no tasks to append after. If the file has no @@ -33,12 +43,14 @@ pub enum InsertAnchor { impl InsertAnchor { /// The 1-indexed line this anchor names, if it names one /// - /// Returns `None` for [`Self::EndOfTasksSection`], which only the emitter - /// can turn into a line. + /// For [`Self::AfterTaskBlock`] this is the earliest line the task could + /// land on; the emitter may push it further down. Returns `None` for + /// [`Self::EndOfTasksSection`], which only the emitter can turn into a + /// line. #[must_use] pub fn line(self) -> Option { match self { - Self::Line(line_number) => Some(line_number), + Self::Line(line_number) | Self::AfterTaskBlock(line_number) => Some(line_number), Self::EndOfTasksSection => None, } } @@ -142,12 +154,14 @@ impl PlacementResolver { let anchor = if let Some(last_sibling) = siblings.last() { // After the last sibling's subtree - InsertAnchor::Line(Self::find_end_of_task_subtree(&ctx.resolved_file, last_sibling) + 1) + InsertAnchor::AfterTaskBlock( + Self::find_end_of_task_subtree(&ctx.resolved_file, last_sibling) + 1, + ) } else if let Some(parent) = &ctx.parent_task { // No siblings, insert right after parent (accounting for annotations) let parent_line = Self::get_task_line(&ctx.resolved_file, parent); let annotation_lines = Self::count_annotation_lines(parent); - InsertAnchor::Line(parent_line + annotation_lines + 1) + InsertAnchor::AfterTaskBlock(parent_line + annotation_lines + 1) } else { // Nothing to append after: the file has no tasks at this level and // no parent to hang off. Only the source text says where the @@ -248,7 +262,7 @@ impl PlacementResolver { let line_number = Self::find_end_of_task_subtree(&ctx.resolved_file, task) + 1; Ok(PlacementInfo { - anchor: InsertAnchor::Line(line_number), + anchor: InsertAnchor::AfterTaskBlock(line_number), order_index, indent_level: ctx.computed_depth as usize, }) diff --git a/devlog.md b/devlog.md index d36d86e..f37caf0 100644 --- a/devlog.md +++ b/devlog.md @@ -2518,3 +2518,51 @@ copying every other line through. The test that catches both at once is changes from shortly before the stream opened. - The two tests `tasks/tasks.status-bar-activity.md` deferred for want of a TUI harness are written now that one exists (#45). + +## `lash add` stole the previous task's body (#48, 2026-08-10) + +The placement bug the launch-blocker sweep did not reach. That sweep fixed the +*annotation* line count and stopped there, on the assumption that a task's +checkbox line plus its annotation block is the whole task. It is not. Tasks +also carry free-text bodies — prose, numbered steps, acceptance criteria, +indented note bullets — and the parser records none of it. + +So appending to a file whose last task had a body anchored between that task's +title and its own body: + +```markdown +- [x] Second task @id:rep-second +- [ ] New task #report + This body belongs to the SECOND task and must stay attached to it. +``` + +The body was reassigned to a task it has nothing to do with. Nothing surfaced +it: `lash lint` passes on the result, and `lash show` prints a task's ID, +title, status, file and labels but never its body. Reported from real use in +flawd's `tasks/tasks.polish.md`, where the last entry was a long task with +numbered steps; present since 0.1.0. + +Fixed the way `EndOfTasksSection` was: the source text is the only thing that +knows where a task's block ends, so the answer is computed in the emitter, +which is already holding the file. `InsertAnchor::AfterTaskBlock(line)` carries +"start looking here" through from the resolver, and the emitter walks forward +past every line that continues the block — indented, not a checkbox — before +inserting. Blank lines count as part of the block only when indented content +resumes after them, so a body split into paragraphs stays whole while a blank +line that genuinely ends the block still stops the walk. + +Deriving this from the parsed model instead would have meant a new field on +`Task`, a fourth thing to keep in sync, and the same class of bug the sweep +already fixed twice. Bodyless tasks resolve to exactly the line they did +before, so the common case is untouched. + +One formatting change: a new task inserted directly below another task's body +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. diff --git a/lash.index.md b/lash.index.md index 24ebb25..65cfe42 100644 --- a/lash.index.md +++ b/lash.index.md @@ -355,3 +355,55 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. 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 + +### Post-0.3.0 (Flawd dogfooding, 2026-08-10) + +- [x] `lash add -f` inserts the new task between the previous task's title and its body #cli #bug #data-loss + - The placement bug the launch-blocker sweep did not reach. That sweep fixed + the *annotation* line count on the assumption that a task is its checkbox + line plus its annotation block. Tasks also carry free-text bodies — prose, + numbered steps, acceptance criteria, indented note bullets — and the parser + records none of it, so the insertion point lands one line below the title + - The previous task keeps its title and loses its body; the new task inherits + lines it has nothing to do with. Markdown is the source of truth, so the + file itself is now wrong for whoever reads it next. Reported from real use + in flawd's `tasks/tasks.polish.md`, where the last entry was a long task + with numbered steps and acceptance criteria; present since 0.1.0 + - Easy to miss: `lash lint` passes on the damaged file, and `lash show` + prints ID, title, status, file and labels but never the body, so querying + either task looks normal + - Fixed the way `EndOfTasksSection` was — the source text is the only thing + that knows where a block ends, so the emitter computes it while it is + already holding the file. `InsertAnchor::AfterTaskBlock(line)` carries + "start looking here" through from the resolver, and the emitter walks past + every line that continues the block (indented, not a checkbox). Blank lines + belong to the block only when indented content resumes after them, so a + body split into paragraphs stays whole. Deriving this from the parsed model + instead would need a new `Task` field, a fourth thing to keep in sync, and + the same class of bug the sweep already fixed twice + - Bodyless tasks resolve to exactly the line they did before. One formatting + change: a task inserted directly below another task's body now gets a blank + line above it, since butting it against the tail of someone else's prose + reads as more of that prose + - Regression tests: the reported repro end to end (`lash add` after a task + with a multi-line body, asserting the body stays attached and the reported + 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 + - 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 + with prose or acceptance criteria under it comes back as a bare checkbox + line, exit code 0 + - Repro: a file whose task carries an indented body, `lash format` it, the + 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