Skip to content

Commit dc5d83c

Browse files
committed
fix(add): count annotation lines from the source, not from parsed metadata
Appending after a task that carried a hand-written `@depends-on: a, b, c` put the new task two lines too far down, outside the `## Tasks` section entirely. In the repro the Notes heading ended up separated from its own prose. `count_annotation_lines` added one line per parsed dependency. That holds for what `MarkdownEmitter::format_task_annotations` writes, one reference per line, but the parser splits a comma-separated value into several references off a single line, so the count ran two lines long. `@labels` had the same problem from the other direction and was simply skipped, with a comment saying it could not be counted, because labels can be inline on the checkbox line or in an `@labels:` block and the parsed metadata does not say which. Both are the same defect: the number of source lines cannot be recovered from parsed metadata, because the same metadata can be written in more than one shape. So record it at parse time instead. The parser already collects the annotation lines for a task and therefore already knows the count; it now keeps it on `Task::annotation_line_count`, and the resolver uses that. This settles the comma form, `@labels`, and any future annotation with more than one written shape, without the resolver having to know about any of them. Tasks built in memory rather than parsed (the TUI's in-memory tasks, test fixtures) have no source lines to have counted, so `annotation_line_count` is 0 and the resolver falls back to deriving a count from the metadata, assuming the emitter's shape. That keeps existing behaviour for callers that never touch a file. Tests: three integration tests appending after a task with comma-form `@depends-on`, with one `@depends-on` per line, and with block-form `@labels`, each asserting the new task lands inside `## Tasks` and the following section keeps its content. Two unit tests covering the recorded count winning over the derived estimate, and the fallback for an unparsed task. The comma-form and block-labels tests fail without the fix. Also files two `lash format` bugs found while verifying: it deletes every section after `## Tasks`, and it duplicates inline labels on every run so the output never reaches a fixed point.
1 parent 698d642 commit dc5d83c

33 files changed

Lines changed: 345 additions & 25 deletions

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,7 @@ mod tests {
628628
parent_id: None,
629629
order_index: 0,
630630
line_number: 0,
631+
annotation_line_count: 0,
631632
metadata: TaskMetadata {
632633
depends_on: deps
633634
.iter()

‎crates/lash-cli/src/utils/task_target.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ mod tests {
8686
parent_id: None,
8787
order_index: order,
8888
line_number: 0,
89+
annotation_line_count: 0,
8990
metadata: TaskMetadata::default(),
9091
body: None,
9192
contextual_notes: Vec::new(),

‎crates/lash-cli/tests/add_command_test.rs‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,3 +654,143 @@ fn test_add_rejects_an_agent_note_line_starting_with_an_annotation() {
654654
let content = fs::read_to_string(project.file_path("tasks.md")).unwrap();
655655
assert!(!content.contains("At note task"));
656656
}
657+
658+
// ---------------------------------------------------------------------
659+
// `@depends-on` written in comma form
660+
//
661+
// `count_annotation_lines` added one line per parsed dependency, which
662+
// holds for what the emitter writes but not for a hand-written
663+
// `@depends-on: a, b, c`. The parser splits that single line into three
664+
// references, so the count ran two lines long and the insertion point
665+
// escaped the task block, landing the new task outside `## Tasks`.
666+
// ---------------------------------------------------------------------
667+
668+
#[test]
669+
fn test_add_after_comma_form_depends_on_stays_inside_the_tasks_section() {
670+
let project = TestProject::builder()
671+
.with_index("test-project", "Test Project")
672+
.with_file(
673+
"tasks.md",
674+
r#"# Tasks
675+
676+
@id: tasks
677+
678+
## Tasks
679+
680+
- [ ] Alpha
681+
- [ ] Beta
682+
- [ ] Gamma
683+
- [ ] Last task
684+
@depends-on: alpha, beta, gamma
685+
686+
## Notes
687+
688+
Prose that must stay under its heading.
689+
"#,
690+
)
691+
.build();
692+
index(&project);
693+
694+
add_task(&project, "Second task", "tasks.md");
695+
696+
let content = fs::read_to_string(project.file_path("tasks.md")).unwrap();
697+
698+
let task = task_line_number(&content, "- [ ] Second task");
699+
assert!(
700+
task > task_line_number(&content, "@depends-on: alpha, beta, gamma"),
701+
"task must land after the dependency line:\n{content}"
702+
);
703+
assert!(
704+
task < task_line_number(&content, "## Notes"),
705+
"task escaped the Tasks section:\n{content}"
706+
);
707+
assert!(
708+
content.contains("## Notes\n\nProse that must stay under its heading."),
709+
"the Notes heading was split from its prose:\n{content}"
710+
);
711+
}
712+
713+
#[test]
714+
fn test_add_after_one_dependency_per_line_still_clears_the_block() {
715+
// The other half of the ambiguity: the same three references written the
716+
// way the emitter writes them occupy three lines, not one.
717+
let project = TestProject::builder()
718+
.with_index("test-project", "Test Project")
719+
.with_file(
720+
"tasks.md",
721+
r#"# Tasks
722+
723+
@id: tasks
724+
725+
## Tasks
726+
727+
- [ ] Alpha
728+
- [ ] Beta
729+
- [ ] Gamma
730+
- [ ] Last task
731+
@depends-on: alpha
732+
@depends-on: beta
733+
@depends-on: gamma
734+
735+
## Notes
736+
737+
Prose that must stay under its heading.
738+
"#,
739+
)
740+
.build();
741+
index(&project);
742+
743+
add_task(&project, "Second task", "tasks.md");
744+
745+
let content = fs::read_to_string(project.file_path("tasks.md")).unwrap();
746+
747+
let task = task_line_number(&content, "- [ ] Second task");
748+
assert!(
749+
task > task_line_number(&content, "@depends-on: gamma"),
750+
"task was spliced into the dependency block:\n{content}"
751+
);
752+
assert!(
753+
task < task_line_number(&content, "## Notes"),
754+
"task escaped the Tasks section:\n{content}"
755+
);
756+
}
757+
758+
#[test]
759+
fn test_add_after_a_task_whose_labels_use_the_block_form() {
760+
// `@labels:` used to be uncountable for the same reason, since labels can
761+
// also live inline on the checkbox line.
762+
let project = TestProject::builder()
763+
.with_index("test-project", "Test Project")
764+
.with_file(
765+
"tasks.md",
766+
r#"# Tasks
767+
768+
@id: tasks
769+
770+
## Tasks
771+
772+
- [ ] Last task
773+
@labels: backend, infra
774+
775+
## Notes
776+
777+
Prose that must stay under its heading.
778+
"#,
779+
)
780+
.build();
781+
index(&project);
782+
783+
add_task(&project, "Second task", "tasks.md");
784+
785+
let content = fs::read_to_string(project.file_path("tasks.md")).unwrap();
786+
787+
let task = task_line_number(&content, "- [ ] Second task");
788+
assert!(
789+
task > task_line_number(&content, "@labels: backend, infra"),
790+
"task was spliced above the labels line:\n{content}"
791+
);
792+
assert!(
793+
task < task_line_number(&content, "## Notes"),
794+
"task escaped the Tasks section:\n{content}"
795+
);
796+
}

‎crates/lash-core/src/creation/placement.rs‎

Lines changed: 93 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -279,27 +279,31 @@ impl PlacementResolver {
279279

280280
/// Count the number of annotation lines that follow a task
281281
///
282-
/// Every annotation type is written on its own line after the task
283-
/// checkbox line, so appending correctly requires knowing exactly how
284-
/// many trailing lines belong to the task being appended after —
285-
/// undercounting here inserts the new task in the middle of that
286-
/// annotation block, silently detaching a trailing annotation (e.g. an
287-
/// existing task's `@id:`) from its owner.
282+
/// Appending after a task means stepping past its whole block: the
283+
/// checkbox line plus every annotation line below it. Getting this wrong
284+
/// in either direction is a bug. Undercounting splices the new task into
285+
/// the middle of the annotation block, detaching a trailing annotation
286+
/// from its owner or destroying the continuation lines of a multi-line
287+
/// note. Overcounting pushes the insertion point past the end of the
288+
/// block, which can land the new task outside the `## Tasks` section
289+
/// entirely.
288290
///
289-
/// An annotation value may span several source lines: the parser folds
290-
/// indented continuation lines into the value, joining them with `\n`
291-
/// (see `parser::annotations::parse_annotation_block`). Such a value
292-
/// therefore occupies `value.lines().count()` source lines, not one, and
293-
/// counting it as one splices the new task into the middle of the note —
294-
/// destroying the continuation lines on the next reindex.
291+
/// For a task that came from a file, the count is whatever the parser
292+
/// counted, recorded on [`Task::annotation_line_count`]. It is the only
293+
/// place with the answer, because the same metadata can be written in more
294+
/// than one shape and the parsed [`lash_types::task::TaskMetadata`] does
295+
/// not record which was used:
295296
///
296-
/// `@labels` is deliberately not counted: labels may be written inline
297-
/// on the checkbox line (`#backend`) or as a separate `@labels:` block,
298-
/// and [`lash_types::task::TaskMetadata`] doesn't record which form was
299-
/// used, so it can't be counted without re-reading the source line.
300-
/// `lash add` itself always emits labels inline (see
301-
/// `MarkdownEmitter::format_task_line`), so this only affects appending
302-
/// after a hand-edited task that used the block form.
297+
/// - `@depends-on: a, b, c` on one line, or one `@depends-on:` line each
298+
/// - `#backend` inline on the checkbox line, or an `@labels:` block
299+
/// - a value folded across several indented continuation lines
300+
///
301+
/// For a task built programmatically rather than parsed (the TUI's
302+
/// in-memory tasks, test fixtures), there are no source lines to have
303+
/// counted, so the count is derived from the metadata on the assumption
304+
/// that it will be written the way [`super::emitter::MarkdownEmitter`]
305+
/// writes it: one line per annotation, one line per `@depends-on` and
306+
/// `@doc` reference, labels inline.
303307
fn count_annotation_lines(task: &Task) -> usize {
304308
/// Source lines occupied by a single annotation value.
305309
///
@@ -309,6 +313,10 @@ impl PlacementResolver {
309313
value.lines().count().max(1)
310314
}
311315

316+
if task.annotation_line_count > 0 {
317+
return task.annotation_line_count;
318+
}
319+
312320
let mut count = 0;
313321

314322
// @id: one line, only when it's an explicit annotation (not a
@@ -330,11 +338,7 @@ impl PlacementResolver {
330338
}
331339

332340
// @depends-on / @doc: one line per reference, as emitted by
333-
// `MarkdownEmitter::format_task_annotations`. A hand-written
334-
// comma-separated `@depends-on: a, b, c` parses to several references
335-
// from a single line and is over-counted here; that is the same
336-
// inline-vs-block ambiguity described above for `@labels`, and
337-
// over-counting appends after the block rather than inside it.
341+
// `MarkdownEmitter::format_task_annotations`.
338342
count += task.metadata.depends_on.len();
339343
count += task.metadata.docs.len();
340344

@@ -982,6 +986,71 @@ mod tests {
982986
assert_eq!(placement.order_index, 3);
983987
}
984988

989+
/// An explicit-id dependency reference, the form `lash add --depends-on`
990+
/// produces.
991+
fn dep_ref(target: &str) -> lash_types::DependencyRef {
992+
lash_types::DependencyRef::new(target.to_string(), lash_types::DependencyKind::ExplicitId)
993+
}
994+
995+
#[test]
996+
fn test_parsed_annotation_line_count_wins_over_the_derived_estimate() {
997+
// A hand-written `@depends-on: a, b, c` parses to three references off
998+
// a single line. Deriving the count from the metadata says three
999+
// lines, which pushes the insertion point two lines past the end of
1000+
// the task block; the parser's own count says one.
1001+
let config = ConfigBuilder::new().build().unwrap();
1002+
let validator = TaskValidator::new(config);
1003+
1004+
let mut task = TaskBuilder::new("Existing task")
1005+
.id("existing")
1006+
.order_index(0)
1007+
.line_number(6)
1008+
.build()
1009+
.unwrap();
1010+
task.metadata.depends_on = ["a", "b", "c"].map(dep_ref).to_vec();
1011+
// `@id:` on line 7, `@depends-on: a, b, c` on line 8.
1012+
task.annotation_line_count = 2;
1013+
1014+
let mut tasks = TaskTree::new();
1015+
tasks.add_task(task).unwrap();
1016+
let file = create_test_file(tasks);
1017+
1018+
let request = TaskCreationRequestBuilder::new("New task").build();
1019+
let ctx = validator.validate(&request, Some(&file)).unwrap();
1020+
let placement = PlacementResolver::resolve(&ctx, &request).unwrap();
1021+
1022+
assert_eq!(placement.anchor.line(), Some(9));
1023+
}
1024+
1025+
#[test]
1026+
fn test_unparsed_task_falls_back_to_deriving_the_count() {
1027+
// Tasks built in memory rather than parsed have no source lines to
1028+
// have counted, so the derived estimate is all there is. It assumes
1029+
// the emitter's shape: one line per reference.
1030+
let config = ConfigBuilder::new().build().unwrap();
1031+
let validator = TaskValidator::new(config);
1032+
1033+
let mut task = TaskBuilder::new("Existing task")
1034+
.id("existing")
1035+
.order_index(0)
1036+
.line_number(6)
1037+
.build()
1038+
.unwrap();
1039+
task.metadata.depends_on = ["a", "b"].map(dep_ref).to_vec();
1040+
assert_eq!(task.annotation_line_count, 0);
1041+
1042+
let mut tasks = TaskTree::new();
1043+
tasks.add_task(task).unwrap();
1044+
let file = create_test_file(tasks);
1045+
1046+
let request = TaskCreationRequestBuilder::new("New task").build();
1047+
let ctx = validator.validate(&request, Some(&file)).unwrap();
1048+
let placement = PlacementResolver::resolve(&ctx, &request).unwrap();
1049+
1050+
// Checkbox on 6, `@id:` on 7, two `@depends-on:` lines on 8 and 9.
1051+
assert_eq!(placement.anchor.line(), Some(10));
1052+
}
1053+
9851054
#[test]
9861055
fn test_append_after_task_with_explicit_id_skips_its_annotation_line() {
9871056
// Regression test: appending after a task that carries an explicit

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,7 @@ mod tests {
742742
parent_id: parent.map(std::string::ToString::to_string),
743743
order_index: 0,
744744
line_number: 0,
745+
annotation_line_count: 0,
745746
metadata: TaskMetadata::default(),
746747
body: None,
747748
contextual_notes: Vec::new(),

‎crates/lash-core/src/linter/linter.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,7 @@ mod tests {
311311
parent_id: None,
312312
order_index: i,
313313
line_number: 0,
314+
annotation_line_count: 0,
314315
metadata: TaskMetadata::default(),
315316
body: None,
316317
contextual_notes: Vec::new(),

‎crates/lash-core/src/linter/rules/crossfile/circular_deps.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,7 @@ mod tests {
270270
parent_id: None,
271271
order_index: 0,
272272
line_number: 0,
273+
annotation_line_count: 0,
273274
metadata,
274275
body: None,
275276
contextual_notes: Vec::new(),

0 commit comments

Comments
 (0)