Skip to content

Commit cff612f

Browse files
committed
fix(format): stop deleting sections and duplicating labels
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.
1 parent c6afbc7 commit cff612f

5 files changed

Lines changed: 391 additions & 92 deletions

File tree

‎crates/lash-cli/src/commands/format.rs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -347,16 +347,17 @@ fn format_single_file(
347347
let task_file = parse_file(file_path, config)
348348
.with_context(|| format!("Failed to parse {}", file_path.display()))?;
349349

350+
// Read original content. The formatter needs it as well as the parsed
351+
// file, to carry through sections the model does not represent.
352+
let original = std::fs::read_to_string(file_path)
353+
.with_context(|| format!("Failed to read {}", file_path.display()))?;
354+
350355
// Format the file
351356
let formatter = Formatter::new(config.clone(), options.clone());
352357
let formatted = formatter
353-
.format_file(&task_file)
358+
.format_file(&original, &task_file)
354359
.with_context(|| format!("Failed to format {}", file_path.display()))?;
355360

356-
// Read original content
357-
let original = std::fs::read_to_string(file_path)
358-
.with_context(|| format!("Failed to read {}", file_path.display()))?;
359-
360361
// Check if content changed
361362
let changed = formatted != original;
362363

‎crates/lash-core/src/formatter/mod.rs‎

Lines changed: 92 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,13 @@
3737
//! description: None,
3838
//! description_agent_notes: Vec::new(),
3939
//! };
40-
//! let formatted = formatter.format_file(&file).unwrap();
40+
//! let formatted = formatter.format_file("", &file).unwrap();
4141
//! assert!(formatted.contains("# Test File"));
4242
//! ```
4343
4444
pub mod options;
4545

46+
use crate::parser::header;
4647
use lash_types::{LashConfig, Result, TaskFile, TaskStatus};
4748
use std::path::Path;
4849

@@ -125,41 +126,79 @@ impl Formatter {
125126
/// # description: None,
126127
/// # description_agent_notes: Vec::new(),
127128
/// # };
128-
/// let formatted = formatter.format_file(&file).unwrap();
129+
/// let formatted = formatter.format_file("", &file).unwrap();
129130
/// assert!(formatted.contains("# Test File"));
130131
/// ```
131132
#[allow(clippy::result_large_err)] // LashError is intentionally rich with context
132-
pub fn format_file(&self, file: &TaskFile) -> Result<String> {
133+
pub fn format_file(&self, source: &str, file: &TaskFile) -> Result<String> {
133134
// Apply auto-fixes if enabled
134135
let file = if self.options.apply_auto_fixes {
135136
self.apply_auto_fixes(file)?
136137
} else {
137138
file.clone()
138139
};
139140

140-
// Build the formatted output
141+
let lines: Vec<&str> = source.lines().collect();
142+
let header = header::header_span(source);
143+
let description = header::section_span(source, "description");
144+
let tasks = header::section_span(source, "tasks");
145+
141146
let mut output = String::new();
142147

143-
// 1. Format header (title, annotations, overview)
148+
// The header is always regenerated, whether or not the source had one.
144149
self.format_header(&file, &mut output);
145150

146-
// 2. Format description section (if present)
147-
if file.description.is_some() {
148-
Self::format_description(&file, &mut output);
149-
}
151+
// Then walk the rest of the source. Two spans are ours to replace with
152+
// generated text; everything else is copied through byte for byte,
153+
// because the model does not represent it and regenerating the file
154+
// from the model alone silently deletes it.
155+
// Each generated block ends with its own blank separator, so the
156+
// source's is skipped rather than copied; emitting both would add a
157+
// blank line on every run and formatting would not be idempotent.
158+
let skip_blanks = |line: &mut usize| {
159+
while lines.get(*line).is_some_and(|l| l.trim().is_empty()) {
160+
*line += 1;
161+
}
162+
};
150163

151-
// 3. Format tasks section
152-
self.format_tasks(&file, &mut output);
164+
let mut line = header.end;
165+
skip_blanks(&mut line);
153166

154-
// 4. Format references section (if present)
155-
// TODO: Implement once references are stored in TaskFile
167+
while line < lines.len() {
168+
if description.as_ref().is_some_and(|span| span.start == line) {
169+
Self::format_description(&file, &mut output);
170+
line = description.as_ref().map_or(line + 1, |span| span.end);
171+
skip_blanks(&mut line);
172+
} else if tasks.as_ref().is_some_and(|span| span.start == line) {
173+
self.format_tasks(&file, &mut output);
174+
output.push('\n');
175+
line = tasks.as_ref().map_or(line + 1, |span| span.end);
176+
skip_blanks(&mut line);
177+
} else {
178+
output.push_str(lines[line]);
179+
output.push('\n');
180+
line += 1;
181+
}
182+
}
156183

157-
// 4. Normalize whitespace
184+
// A source with no `## Tasks` heading still gets one. The parser treats
185+
// such a file as all-tasks and warns, and emitting the section is what
186+
// makes `lash format` able to repair it.
187+
if tasks.is_none() {
188+
if description.is_none() && file.description.is_some() {
189+
Self::format_description(&file, &mut output);
190+
}
191+
self.format_tasks(&file, &mut output);
192+
}
193+
194+
// Normalize whitespace. This only collapses runs of blank lines and
195+
// trims trailing whitespace, so it is safe to run over copied-through
196+
// content as well as generated content.
158197
if self.options.normalize_whitespace {
159198
output = self.normalize_whitespace(&output);
160199
}
161200

162-
// 5. Ensure file ends with single newline
201+
// Ensure file ends with single newline
163202
if !output.ends_with('\n') {
164203
output.push('\n');
165204
} else if output.ends_with("\n\n") {
@@ -206,11 +245,18 @@ impl Formatter {
206245
/// ```
207246
#[allow(clippy::result_large_err)] // LashError is intentionally rich with context
208247
pub fn format_file_in_place(&self, path: &Path) -> Result<()> {
209-
// Parse the file
248+
// Parse the file, keeping the source: the formatter needs it to carry
249+
// through sections the model does not represent.
250+
let source = std::fs::read_to_string(path).map_err(|e| lash_types::LashError::IO {
251+
code: lash_types::error::codes::E_IO_READ_ERROR,
252+
message: format!("Failed to read file for formatting: {e}"),
253+
path: Some(path.to_path_buf()),
254+
io_error: Some(e.to_string()),
255+
})?;
210256
let file = crate::parser::parse_file(path, &self.config)?;
211257

212258
// Format it
213-
let formatted = self.format_file(&file)?;
259+
let formatted = self.format_file(&source, &file)?;
214260

215261
// Write back atomically (tmp file + rename) so a crash mid-write
216262
// can't leave a partial Markdown file on disk. This shares the same
@@ -525,11 +571,21 @@ impl Formatter {
525571
output.push_str("- [");
526572
output.push(task.status.to_checkbox_char());
527573
output.push_str("] ");
528-
output.push_str(&task.title);
574+
575+
// The parser leaves inline labels in the title *and* records them in
576+
// metadata, so writing the title verbatim and then appending the
577+
// labels emits each one twice. Formatting was therefore not
578+
// idempotent: every run grew the line by another copy of every inline
579+
// label, and `format --check` reported the file as needing formatting
580+
// forever. Strip them from the title and let the canonical, sorted
581+
// list below be the only place they appear.
582+
output.push_str(&strip_inline_labels(&task.title));
529583

530584
// Add inline labels if present
531585
if !task.metadata.labels.is_empty() {
532-
for label in &task.metadata.labels {
586+
let mut labels = task.metadata.labels.clone();
587+
labels.sort();
588+
for label in &labels {
533589
output.push_str(" #");
534590
output.push_str(label);
535591
}
@@ -679,6 +735,20 @@ impl Formatter {
679735
}
680736
}
681737

738+
/// Remove inline `#label` words from a task title
739+
///
740+
/// The parser records inline labels in `TaskMetadata` without removing them
741+
/// from the title, so a formatter that writes both would emit each label
742+
/// twice. Only whole words that parsing would turn into labels are removed, so
743+
/// a `#` inside prose survives.
744+
fn strip_inline_labels(title: &str) -> String {
745+
title
746+
.split_whitespace()
747+
.filter(|word| !lash_types::label::is_inline_label(word))
748+
.collect::<Vec<_>>()
749+
.join(" ")
750+
}
751+
682752
#[cfg(test)]
683753
mod tests {
684754
use super::*;
@@ -779,7 +849,7 @@ mod tests {
779849
make_task("task2", "Second task", TaskStatus::Done, None, 0),
780850
]);
781851

782-
let result = formatter.format_file(&file).unwrap();
852+
let result = formatter.format_file("", &file).unwrap();
783853

784854
assert!(result.contains("# Test File"));
785855
assert!(result.contains("@id: test"));
@@ -800,7 +870,7 @@ mod tests {
800870
make_task("child2", "Child 2", TaskStatus::Done, Some("parent"), 1),
801871
]);
802872

803-
let result = formatter.format_file(&file).unwrap();
873+
let result = formatter.format_file("", &file).unwrap();
804874

805875
assert!(result.contains("- [ ] Parent task"));
806876
assert!(result.contains(" - [ ] Child 1"));
@@ -822,7 +892,7 @@ mod tests {
822892
make_task("child2", "Child 2", TaskStatus::Open, Some("parent"), 1),
823893
]);
824894

825-
let result = formatter.format_file(&file).unwrap();
895+
let result = formatter.format_file("", &file).unwrap();
826896

827897
// Children should be auto-waived
828898
assert!(result.contains("- [-] Parent"));

‎crates/lash-core/src/parser/header.rs‎

Lines changed: 105 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -348,68 +348,143 @@ pub fn content_has_tasks_section(content: &str) -> bool {
348348
false
349349
}
350350

351-
/// Line span of the body of the `## Tasks` section
351+
/// Line span of a `## <name>` section, heading line included
352352
///
353-
/// The range is 0-indexed and half-open: it starts at the first line after the
354-
/// `## Tasks` heading and ends at the line holding the next H1 or H2 heading,
355-
/// or at the end of the file. H3 and deeper headings sit inside the section
356-
/// (files often group tasks under `### Subsection` headings) and do not close
357-
/// it. Returns `None` if the file has no `## Tasks` heading.
353+
/// The range is 0-indexed and half-open: it starts at the heading and ends at
354+
/// the line holding the next H1 or H2 heading, or at the end of the file. H3
355+
/// and deeper headings sit inside the section (files often group tasks under
356+
/// `### Subsection` headings) and do not close it. Returns `None` if the file
357+
/// has no such H2. Matching on `name` is case-insensitive.
358358
///
359359
/// A parsed [`lash_types::TaskFile`] records task line numbers but no section
360-
/// boundaries, so anything that needs the extent of the Tasks section (in
361-
/// particular appending to a section with no tasks in it yet) has to go back to
362-
/// the source text. Heading detection runs through pulldown-cmark, so a `##`
363-
/// inside a fenced code block does not close the section.
360+
/// boundaries, so anything that needs the extent of a section has to go back
361+
/// to the source text. Heading detection runs through pulldown-cmark, so a
362+
/// `##` inside a fenced code block neither opens nor closes a section.
364363
///
365364
/// # Example
366365
///
367366
/// ```
368-
/// use lash_core::parser::header::tasks_section_body;
367+
/// use lash_core::parser::header::section_span;
369368
///
370369
/// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n";
371-
/// // Lines 3 through 5: the blank line, the task, and the blank line after it.
372-
/// assert_eq!(tasks_section_body(content), Some(3..6));
373-
///
374-
/// // An empty section still has a body, it is just blank.
375-
/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3));
376-
///
377-
/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None);
370+
/// assert_eq!(section_span(content, "tasks"), Some(2..6));
371+
/// assert_eq!(section_span(content, "notes"), Some(6..9));
372+
/// assert_eq!(section_span(content, "nope"), None);
378373
/// ```
379374
#[must_use]
380-
pub fn tasks_section_body(content: &str) -> Option<std::ops::Range<usize>> {
375+
pub fn section_span(content: &str, name: &str) -> Option<std::ops::Range<usize>> {
381376
let mut heading_level: Option<HeadingLevel> = None;
382377
let mut heading_text = String::new();
383-
let mut body_start_byte: Option<usize> = None;
384-
let mut body_end_byte = content.len();
378+
let mut span: Option<std::ops::Range<usize>> = None;
385379

386380
for (event, range) in Parser::new(content).into_offset_iter() {
387381
match event {
388382
Event::Start(Tag::Heading { level, .. }) => {
389-
if body_start_byte.is_some() && matches!(level, HeadingLevel::H1 | HeadingLevel::H2)
390-
{
391-
body_end_byte = range.start;
392-
break;
383+
if let Some(started) = span.as_mut() {
384+
if matches!(level, HeadingLevel::H1 | HeadingLevel::H2) {
385+
started.end = line_at_byte(content, range.start);
386+
break;
387+
}
393388
}
394389
heading_level = Some(level);
395390
heading_text.clear();
396391
}
397392
Event::Text(text) if heading_level.is_some() => heading_text.push_str(&text),
398393
Event::End(TagEnd::Heading(level)) => {
399-
if body_start_byte.is_none()
394+
if span.is_none()
400395
&& level == HeadingLevel::H2
401-
&& heading_text.trim().eq_ignore_ascii_case("tasks")
396+
&& heading_text.trim().eq_ignore_ascii_case(name)
402397
{
403-
body_start_byte = Some(range.end);
398+
let start = line_at_byte(content, range.start);
399+
span = Some(start..content.lines().count());
404400
}
405401
heading_level = None;
406402
}
407403
_ => {}
408404
}
409405
}
410406

411-
let body_start_byte = body_start_byte?;
412-
Some(line_at_byte(content, body_start_byte)..line_at_byte(content, body_end_byte))
407+
span
408+
}
409+
410+
/// Line span of the body of the `## Tasks` section
411+
///
412+
/// Same as [`section_span`] for "tasks", minus the heading line, so it starts
413+
/// at the first line below `## Tasks`.
414+
///
415+
/// # Example
416+
///
417+
/// ```
418+
/// use lash_core::parser::header::tasks_section_body;
419+
///
420+
/// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n";
421+
/// // Lines 3 through 5: the blank line, the task, and the blank line after it.
422+
/// assert_eq!(tasks_section_body(content), Some(3..6));
423+
///
424+
/// // An empty section still has a body, it is just blank.
425+
/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3));
426+
///
427+
/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None);
428+
/// ```
429+
#[must_use]
430+
pub fn tasks_section_body(content: &str) -> Option<std::ops::Range<usize>> {
431+
section_span(content, "tasks").map(|span| (span.start + 1).min(span.end)..span.end)
432+
}
433+
434+
/// Line span the formatter regenerates as the file header: the H1 and the
435+
/// annotation block below it
436+
///
437+
/// Everything below it belongs to whoever is rewriting the file. Overview
438+
/// prose and any section the model does not represent live there, and the
439+
/// formatter has to copy them through rather than regenerate them.
440+
///
441+
/// The range always starts at 0. It is empty when the file has no H1, which is
442+
/// the case where the parser synthesizes a title from the filename and the
443+
/// formatter writes a header that was not there before.
444+
///
445+
/// # Example
446+
///
447+
/// ```
448+
/// use lash_core::parser::header::header_span;
449+
///
450+
/// // The H1 and the `@id:` line, but not the prose below them.
451+
/// assert_eq!(header_span("# T\n\n@id: t\n\nsome prose\n"), 0..3);
452+
///
453+
/// // No H1 to regenerate around.
454+
/// assert_eq!(header_span("just prose\n"), 0..0);
455+
/// ```
456+
#[must_use]
457+
pub fn header_span(content: &str) -> std::ops::Range<usize> {
458+
let lines: Vec<&str> = content.lines().collect();
459+
460+
let Some(h1) = lines.iter().position(|line| {
461+
let trimmed = line.trim_start();
462+
trimmed.starts_with("# ") && !trimmed.starts_with("##")
463+
}) else {
464+
return 0..0;
465+
};
466+
467+
let mut end = h1 + 1;
468+
let mut saw_annotation = false;
469+
for (offset, line) in lines.iter().enumerate().skip(h1 + 1) {
470+
let trimmed = line.trim();
471+
if trimmed.is_empty() {
472+
// A blank line may separate the H1 from the annotations, or follow
473+
// them. Either way it does not end the block on its own.
474+
continue;
475+
}
476+
if trimmed.starts_with('@') {
477+
saw_annotation = true;
478+
end = offset + 1;
479+
} else if saw_annotation && line.starts_with(' ') {
480+
// Continuation of the annotation above.
481+
end = offset + 1;
482+
} else {
483+
break;
484+
}
485+
}
486+
487+
0..end
413488
}
414489

415490
/// 0-indexed line holding the given byte offset

0 commit comments

Comments
 (0)