From e4b21700c4a2e1fc74839f2ebd8bd124a4a9f8c5 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Sat, 8 Aug 2026 14:57:03 -0600 Subject: [PATCH 1/2] fix(add): count multi-line annotations when computing the append point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lash add` spliced a new task into the middle of a preceding task's multi-line `@agent-note`, and the orphaned continuation lines were then destroyed on the next reindex — silent data loss with a success message and exit code 0. `PlacementResolver::count_annotation_lines` counted every annotation as exactly one source line. The parser folds indented continuation lines into the annotation value, joining them with `\n` (see `parser::annotations::parse_annotation_block`), so a three-line note is a single `Option` holding two newlines and occupies three lines on disk. Counting it as one put the insertion point after the note's first line. Count actual source lines via `value.lines().count().max(1)`, applied to every single-value annotation the parser can fold: `@agent-note`, `@owner`, `@estimate`, and custom annotations. The `.max(1)` floor matters — an empty value has `lines().count() == 0` but still occupies its own `@key:` line. This is the same class as the `@id` undercount fixed previously (the issue #24 regression test); the assumption of one line per annotation was simply never revisited for values that can wrap. Tests: four unit tests in placement.rs (multi-line note, single-line note, empty note, multi-line owner/estimate) and two end-to-end tests in add_command_test.rs (note stays contiguous in the file; all lines survive reindex). Verified the multi-line tests fail without the fix. Two related defects found while verifying are filed as follow-ups in lash.index.md rather than fixed here: `@depends-on` in comma form over-counts and can append outside the tasks section, and `lash add --agent-note` with an embedded newline emits an unindented continuation that the parser silently drops. --- crates/lash-cli/tests/add_command_test.rs | 100 ++++++++++++++++ crates/lash-core/src/creation/placement.rs | 128 +++++++++++++++++++-- lash.index.md | 98 ++++++++++++++++ 3 files changed, 316 insertions(+), 10 deletions(-) diff --git a/crates/lash-cli/tests/add_command_test.rs b/crates/lash-cli/tests/add_command_test.rs index f95e43f..cf9626d 100644 --- a/crates/lash-cli/tests/add_command_test.rs +++ b/crates/lash-cli/tests/add_command_test.rs @@ -266,3 +266,103 @@ fn test_add_depends_on_resolves_task_added_earlier_via_explicit_id() { let content = fs::read_to_string(project.file_path("tasks.md")).unwrap(); assert!(content.contains("@depends-on: base-step")); } + +// --------------------------------------------------------------------- +// Multi-line @agent-note preservation +// +// `lash add` counted a multi-line `@agent-note` as a single line when +// computing the append point, so the new task was spliced between the +// note's first line and its continuations. The orphaned continuation +// lines were then destroyed on reindex — silent data loss, exit code 0. +// --------------------------------------------------------------------- + +/// A project whose single task carries a three-line `@agent-note`. +fn project_with_multiline_note() -> TestProject { + TestProject::builder() + .with_index("test-project", "Test Project") + .with_file( + "tasks.md", + r#"# Tasks + +@id: tasks + +## Tasks + +- [ ] Existing task + @id: existing + @agent-note: line one of the note + line two of the note + line three of the note +"#, + ) + .build() +} + +#[test] +fn test_add_preserves_preceding_multiline_agent_note_in_file() { + let project = project_with_multiline_note(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Second task") + .arg("--file") + .arg("tasks.md") + .assert() + .success(); + + let content = fs::read_to_string(project.file_path("tasks.md")).unwrap(); + + // The note must remain contiguous: all three lines still adjacent, with + // no task line spliced between them. + assert!( + content.contains( + " @agent-note: line one of the note\n line two of the note\n line three of the note" + ), + "multi-line agent note was split apart:\n{content}" + ); + + // And the new task must land after the whole note, not inside it. + let note_end = content.find("line three of the note").unwrap(); + let new_task = content + .find("- [ ] Second task") + .expect("new task missing from file"); + assert!( + new_task > note_end, + "new task was inserted inside the preceding note:\n{content}" + ); +} + +#[test] +fn test_add_preserves_multiline_agent_note_through_reindex() { + // The destructive step: even when the on-disk splice looks survivable, + // reindexing used to drop the orphaned continuation lines entirely. + let project = project_with_multiline_note(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Second task") + .arg("--file") + .arg("tasks.md") + .assert() + .success(); + + index(&project); + + let content = fs::read_to_string(project.file_path("tasks.md")).unwrap(); + for line in [ + "line one of the note", + "line two of the note", + "line three of the note", + ] { + assert!( + content.contains(line), + "note content '{line}' lost after reindex:\n{content}" + ); + } +} diff --git a/crates/lash-core/src/creation/placement.rs b/crates/lash-core/src/creation/placement.rs index 81d5a43..fdd2fed 100644 --- a/crates/lash-core/src/creation/placement.rs +++ b/crates/lash-core/src/creation/placement.rs @@ -285,6 +285,13 @@ impl PlacementResolver { /// annotation block, silently detaching a trailing annotation (e.g. an /// existing task's `@id:`) from its owner. /// + /// An annotation value may span several source lines: the parser folds + /// indented continuation lines into the value, joining them with `\n` + /// (see `parser::annotations::parse_annotation_block`). Such a value + /// therefore occupies `value.lines().count()` source lines, not one, and + /// counting it as one splices the new task into the middle of the note — + /// destroying the continuation lines on the next reindex. + /// /// `@labels` is deliberately not counted: labels may be written inline /// on the checkbox line (`#backend`) or as a separate `@labels:` block, /// and [`lash_types::task::TaskMetadata`] doesn't record which form was @@ -293,6 +300,14 @@ impl PlacementResolver { /// `MarkdownEmitter::format_task_line`), so this only affects appending /// after a hand-edited task that used the block form. fn count_annotation_lines(task: &Task) -> usize { + /// Source lines occupied by a single annotation value. + /// + /// Always at least 1 (the `@key:` line itself), plus one per folded + /// continuation line. + fn value_lines(value: &str) -> usize { + value.lines().count().max(1) + } + let mut count = 0; // @id: one line, only when it's an explicit annotation (not a @@ -301,23 +316,34 @@ impl PlacementResolver { count += 1; } - // @owner / @estimate / @agent-note: one line each when present. - if task.metadata.owner.is_some() { - count += 1; + // @owner / @estimate / @agent-note: one line each, plus any folded + // continuation lines. + if let Some(owner) = &task.metadata.owner { + count += value_lines(owner); } - if task.metadata.estimate.is_some() { - count += 1; + if let Some(estimate) = &task.metadata.estimate { + count += value_lines(estimate); } - if task.metadata.agent_note.is_some() { - count += 1; + if let Some(note) = &task.metadata.agent_note { + count += value_lines(note); } - // @depends-on / @doc: one line per reference. + // @depends-on / @doc: one line per reference, as emitted by + // `MarkdownEmitter::format_task_annotations`. A hand-written + // comma-separated `@depends-on: a, b, c` parses to several references + // from a single line and is over-counted here; that is the same + // inline-vs-block ambiguity described above for `@labels`, and + // over-counting appends after the block rather than inside it. count += task.metadata.depends_on.len(); count += task.metadata.docs.len(); - // Custom annotations: one line per key. - count += task.metadata.custom.len(); + // Custom annotations: one line per key, plus folded continuations. + count += task + .metadata + .custom + .values() + .map(|v| value_lines(v)) + .sum::(); count } @@ -986,4 +1012,86 @@ mod tests { assert_eq!(placement.line_number, 8); assert_eq!(placement.order_index, 1); } + + /// Build a single-task file whose one task carries `note` as its + /// `@agent-note`, with the checkbox on line 6. + fn file_with_agent_note(note: &str) -> lash_types::TaskFile { + let mut task = TaskBuilder::new("Existing task") + .id("existing") + .order_index(0) + .line_number(6) + .build() + .unwrap(); + task.metadata.agent_note = Some(note.to_string()); + + let mut tasks = TaskTree::new(); + tasks.add_task(task).unwrap(); + create_test_file(tasks) + } + + fn append_line_for(file: &lash_types::TaskFile) -> usize { + let config = ConfigBuilder::new().build().unwrap(); + let validator = TaskValidator::new(config); + let request = TaskCreationRequestBuilder::new("New task").build(); + let ctx = validator.validate(&request, Some(file)).unwrap(); + PlacementResolver::resolve(&ctx, &request) + .unwrap() + .line_number + } + + #[test] + fn test_append_after_task_with_multiline_agent_note_clears_whole_note() { + // Regression test: a multi-line `@agent-note` was counted as a single + // line, so appending landed *between* the note's first line and its + // continuation lines. The orphaned continuations were then destroyed + // on reindex — silent data loss, exit code 0. + // + // The parser folds indented continuation lines into the value joined + // by '\n' (parser::annotations::parse_annotation_block), so a value of + // "a\nb\nc" occupies three source lines. + // + // Layout: checkbox 6, @id: 7, @agent-note: 8, continuations 9 and 10 + // -> append at 11. + let file = file_with_agent_note("line one\nline two\nline three"); + assert_eq!(append_line_for(&file), 11); + } + + #[test] + fn test_append_after_task_with_single_line_agent_note_is_unchanged() { + // Guards the fix against over-correcting: a one-line note must still + // count as exactly one line. Checkbox 6, @id: 7, @agent-note: 8 + // -> append at 9. + let file = file_with_agent_note("just one line"); + assert_eq!(append_line_for(&file), 9); + } + + #[test] + fn test_append_after_task_with_empty_agent_note_counts_one_line() { + // An empty value still occupies its own `@agent-note:` line, but + // "".lines().count() is 0 — hence the .max(1) floor. Without it this + // would append at 8, landing on top of the annotation. + let file = file_with_agent_note(""); + assert_eq!(append_line_for(&file), 9); + } + + #[test] + fn test_append_after_task_with_multiline_owner_and_estimate() { + // The same folding applies to every single-value annotation, not just + // `@agent-note`. Checkbox 6, @id: 7, @owner: 8-9, @estimate: 10-11 + // -> append at 12. + let mut task = TaskBuilder::new("Existing task") + .id("existing") + .order_index(0) + .line_number(6) + .build() + .unwrap(); + task.metadata.owner = Some("alice\ncontinued".to_string()); + task.metadata.estimate = Some("3d\ncontinued".to_string()); + + let mut tasks = TaskTree::new(); + tasks.add_task(task).unwrap(); + let file = create_test_file(tasks); + + assert_eq!(append_line_for(&file), 12); + } } diff --git a/lash.index.md b/lash.index.md index e196767..e17331e 100644 --- a/lash.index.md +++ b/lash.index.md @@ -68,3 +68,101 @@ For background docs, see: finders are thin wrappers handling their own error/fallback semantics (anyhow vs LashError vs DbError vs return-self-on-miss). Side benefit: lash-db's variant used to ignore `index.lash.md` — now it doesn't. + +### Launch polish (Flawd dogfooding, 2026-08-08) + +Found while running Flawd v0.7.0 against this repo. The `lash add` item is a +silent data-loss bug in our own write path and blocks the OSS launch; the other +two are rot in this repo's Flawd config, which means a contributor following +the README cannot run Flawd on Lash at all. Counterpart Flawd-side findings are +filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. + +- [x] `lash add` splices into a preceding multi-line note and silently destroys content #cli #bug #launch-blocker + - LAUNCH BLOCKER — silent data loss in the primary write path. When the last + task in the target file carries a multi-line `@agent-note`, `lash add` + inserts the new task after the *first line* of that note instead of after + the whole task block. On reindex the orphaned continuation lines are + destroyed, not merely misattributed. Exit code is 0 with a normal success + message, so nothing signals the loss + - Minimal repro: a file with one task whose note is a `@agent-note:` line plus + two continuation lines; run `lash add "second task" -f tasks/t.md -l x`. The + new task lands between note line one and line two. After `lash index`, + `lash show` on the first task returns only line one, and the second task has + no note at all — lines two and three are gone from the index entirely + - Hit twice unprompted while filing real tickets in the flawd repo, so this is + not a contrived edge case: any repo using multi-line notes loses content on + every subsequent add + - Fix: insertion point must be the end of the full task block (checkbox line + plus all indented continuation lines including `@`-directives). Regression + tests: add after a task with (a) a multi-line note, (b) several + `@`-directives, (c) a note containing a blank continuation line, (d) no note + — 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 + - 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 + fresh clone dead-ends immediately and produces no output. The config had not + been exercised since the last run in March, so this rotted unnoticed + - Deleting those three lines is the whole fix — with `[llm]` gone the rest + validates clean against v0.7.0 (`flawd config show`, no other stale keys) + - 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 + - `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 + with cargo exit 101 having written nothing + - The committed command also hardcodes host paths (`LLVM_COV=/opt/homebrew/...`, + `LLVM_PROFDATA=/opt/homebrew/...`) — macOS Homebrew paths that cannot resolve + inside a Linux container + - Fix: install `cargo-llvm-cov` plus `llvm-tools-preview` in the Dockerfile, or + 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 +- [ ] `@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()` + assumes one source line per reference, which holds for what + `MarkdownEmitter::format_task_annotations` writes. But the parser splits a + hand-written comma-separated `@depends-on: a, b, c` into three references + from a *single* line, so the count runs two lines long + - Over-counting is not content-destroying like the note bug, but it is not + harmless either: the insertion point escapes the task block. Repro — a file + whose last task carries `@depends-on: a, b, c` on line 9, followed by a + blank line, `## Notes` on line 11, then prose. `lash add "second task"` + computes line 12 and writes the new task *after the `## Notes` heading*, + landing it outside `## Tasks` entirely and splitting Notes from its prose. + Correct placement is line 10 + - When the task is last in the file the emitter's `.min(lines.len())` clamp + hides it — placement reported line 12 for a 10-line file and appended at the + end, correct by accident. So this only bites when content follows + - Fix needs the same information `@labels` lacks: whether the source used the + inline or one-per-line form. Options: have the parser record the source line + span for each annotation (fixes this, `@labels`, and any future variant at + once), or count `min(depends_on.len(), actual_lines)` by re-reading the + source. The first is the real fix; `TaskFile` currently retains no raw + content, so it needs plumbing + - Regression test: append after a task with comma-separated `@depends-on` + followed by a section heading; assert the new task lands inside `## Tasks` +- [ ] `lash add --agent-note` with an embedded newline writes a malformed file and loses content #cli #bug #data-loss + - Latent counterpart to the multi-line note fix, on the write side. + `MarkdownEmitter::format_task_annotations` builds the note with + `format!("{annotation_indent}@agent-note: {note}")` and pushes it as one + "line". If the value contains a newline, the continuation is emitted with no + indentation, so the parser stops at it and the content is dropped + - I previously assessed this as unreachable from the CLI because notes are + single-line. That is wrong — it is trivially reachable: + `lash add "noted task" -f tasks/t.md --agent-note "$(printf 'first\nsecond')"` + writes ` @agent-note: first` followed by a bare unindented `second`, and + `lash show` afterwards returns only "first". The second line is silently + gone, exit code 0 + - Fix: re-indent continuation lines to the annotation indent when emitting, so + a multi-line value round-trips through parse -> emit -> parse. Reject or + escape values that cannot round-trip rather than writing a file the parser + will silently truncate + - Regression test: emit a task with a two-line note, reparse the written file, + assert both lines survive; plus a round-trip property test over notes with + varying line counts and indentation From 7ba1d9d55d11263a201d22fe41c6c2a0a3b82ee2 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Sun, 9 Aug 2026 07:25:38 -0600 Subject: [PATCH 2/2] docs: file three more bugs found while verifying the add path Two are in `lash add` itself: it prepends above the H1 when the target file has a `## Tasks` section with nothing in it (the task lands on disk, `lash index` never sees it, and `lash lint` passes), and the task ID it prints on success is not the one the indexer stores, so copying it into `lash show` or `@depends-on` fails. The third is a test-isolation bug. `test_user_config_save_and_load` in lash-types writes the developer's real `~/.lash/config.toml` and only restores it if the test runs to completion. An interrupted `cargo test` leaves `color_scheme = "Test Theme"` behind, which no theme resolves, and every later `lash` invocation on that machine fails. --- lash.index.md | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/lash.index.md b/lash.index.md index e17331e..29c1dc1 100644 --- a/lash.index.md +++ b/lash.index.md @@ -166,3 +166,75 @@ filed in the flawd repo under `tasks/tasks.fail-fast-degradation.md`. - Regression test: emit a task with a two-line note, reparse the written file, assert both lines survive; plus a round-trip property test over notes with varying line counts and indentation +- [ ] `lash add` prepends above the H1 when the target file has no tasks yet, and the task is never indexed #cli #bug #data-loss + - Severe: the task is written to disk, invisible to Lash, and `lash lint` + passes clean. Found 2026-08-08 while verifying the multi-line note fix + - Repro: a well-formed file with a header and an empty `## Tasks` section + (`# T` / `@id: t` / `## Tasks`). `lash add "Ship it" -f tasks/t.md -l docs` + writes the checkbox at **line 1, above the `# T` heading**, reports + `at tasks/t.md:0`, and a following `lash index` reports `0 task(s)` + - Contrast confirming the trigger is the empty section: the identical command + against the same file with one seed task appends correctly at line 9 + - Cause: `PlacementResolver::resolve_append` returns `line_number: 0` when + `ctx.resolved_file.tasks.is_empty()`, commented "Signal for new file". The + emitter maps `line_number == 0` to `insert_idx = 0` + (`MarkdownEmitter::insert_into_existing`) and prepends. The sentinel + conflates "brand-new file" with "existing file whose Tasks section is + empty" — only the first is safe to write at offset 0 + - Fix: distinguish the two cases. For an existing file with no tasks, append + at the end of the `## Tasks` section (`find_end_of_tasks_section` already + exists and is used for the no-siblings-no-parent branch). Consider replacing + the magic 0 with an explicit enum so the ambiguity cannot recur + - Regression tests: add to (a) an existing file with an empty `## Tasks` + section, (b) an existing file with a `## Tasks` section followed by other + sections, (c) a genuinely new file — assert the header survives at line 1 + in (a) and (b), and that `lash index` finds the task in all three +- [ ] `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 + while filing tickets on 2026-08-08 + - Repro: `lash add "Ship v0.7.0 release notes" -f tasks/t.md -l docs` prints + `Created task ship-v0-7-0-release-notes`, but the indexed id is + `t#ship-v070-release-notes-docs`. Three separate discrepancies in one line: + 1. Version numbers slug differently — `v0-7-0` when reported, `v070` when + indexed. Same input, two normalizations + 2. The label tag is folded into the identity — `#docs` becomes a `-docs` + suffix. A task's id should not change because someone added a label + 3. Long titles are truncated to 40 chars on index but reported in full + (e.g. reported `coverage-fail-fast-when-the-coverage-command-fails-and-its-output-predates-the-run`, + indexed `coverage-fail-fast-when-the-coverage-com`) + - Consequence beyond ergonomics: `@depends-on` written against the reported id + dangles, and #27's dangling-reference check will reject it + - Fix: derive the id once, in one place, and have `lash add` report exactly + what the indexer will store. Label folding looks like the slug being + computed from the raw checkbox line rather than the parsed title — labels + should be excluded from identity + - Minor, same code path: the appended final line has no trailing newline, so + the file ends mid-line and diffs show "\ No newline at end of file" +- [ ] `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 + config. It restores at the end via `UserConfig::default().save()` + - Two problems with that. Even on success it does not restore what the user + had — it overwrites with defaults, silently destroying real settings. And if + the test panics, is filtered out mid-run, or the process is killed, the + restore never executes and `~/.lash/config.toml` is left containing + `color_scheme = "Test Theme"`, which no theme resolves. Every subsequent + `lash` invocation then fails with + `error: Color scheme 'Test Theme' not found` + - Observed 2026-08-09: after a Flawd mutation run executed the suite hundreds + of times (with mutants killed mid-execution), `~/.lash/config.toml` was left + with the test value, mtime inside the run window. `lash index` broke + machine-wide until the line was removed by hand. Mutation testing did not + cause the bug, it amplified it — any interrupted `cargo test` does the same + - The neighbouring `test_user_config_load_nonexistent` comments "This test + assumes ~/.lash/config.toml doesn't exist or is valid", which acknowledges + the hazard without containing it + - Fix: never touch the real home directory from tests. Parameterise the config + path (e.g. `UserConfig::save_to(path)` / `load_from(path)`) and point the + test at a `tempfile::TempDir`, or gate resolution behind an env var the test + sets. Restoring in a `Drop` guard is not sufficient on its own — a SIGKILL + 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`