From f94207b30159e765f16f7e48518321350d9995e0 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 11 Aug 2026 15:12:53 -0600 Subject: [PATCH] fix(add): accept qualified position IDs and make --dry-run resolve placement --before/--after now take the file#slug form that lash show prints, and --dry-run runs the same load/validate/resolve path as a real add instead of echoing the request back. Fixes #53 --- CHANGELOG.md | 16 + crates/lash-cli/src/commands/add.rs | 51 ++- crates/lash-cli/tests/add_command_test.rs | 279 +++++++++++++++++ crates/lash-core/src/creation/placement.rs | 327 +++++++++++++++++++- crates/lash-core/src/creation/service.rs | 95 +++++- crates/lash-types/src/creation_errors.rs | 7 +- crates/lash-types/src/error_explanations.rs | 8 +- devlog.md | 38 +++ docs/error-codes.md | 8 +- docs/task-creation.md | 31 +- docs/user-guide.md | 4 +- 11 files changed, 821 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1534bbc..b1b51ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,22 @@ While the major version is 0, minor version bumps may contain breaking changes. ## [Unreleased] +### Fixed + +- `lash add --before/--after` now accept the file-qualified task ID that `lash + show` and `lash list` print (`index#beta-task`), not just the bare slug. The + target file is already fixed by `--file`, so the qualifier was redundant, but + passing it back failed with "task not found" — which read as the task being + missing rather than the argument being spelled the way lash spells it. A + qualifier naming a *different* file is still rejected, since that means the + task was expected somewhere it is not. The not-found error now lists the IDs + that do exist at that level. +- `lash add --dry-run` now resolves the request instead of echoing it back. It + never opened the target file, so it reported success for a `--before` naming + a task that did not exist and the real add then failed on the same argument. + Dry run and the real add now share one code path, and dry run reports the + insert line it resolved. + ## [0.3.1] - 2026-08-11 Two fixes to the root cause the 0.3.0 sweep left standing. The parsed model diff --git a/crates/lash-cli/src/commands/add.rs b/crates/lash-cli/src/commands/add.rs index 9a93838..cb06eb5 100644 --- a/crates/lash-cli/src/commands/add.rs +++ b/crates/lash-cli/src/commands/add.rs @@ -5,6 +5,7 @@ use anyhow::{Context as AnyhowContext, Result}; use clap::Args; use lash::theme::CliTheme; +use lash_core::creation::placement::InsertAnchor; use lash_core::creation::service::TaskCreationService; use lash_types::creation::{ FileTarget, InsertPosition, ParentRef, TaskCreationRequest, TaskCreationRequestBuilder, @@ -166,16 +167,17 @@ pub fn execute(args: &AddArgs) -> Result { depends_on_warnings = validation.warnings; } - // 4. Handle dry-run mode - if args.dry_run { - return handle_dry_run(&request, args); - } - - // 5. Create service and execute + // 4. Create the service. Dry run needs it too: it reports what would + // happen by actually resolving it, not by echoing the request back. let config = lash_types::config::LashConfig::from_root(&project_root) .unwrap_or_else(|_| lash_types::config::LashConfig::default()); let service = TaskCreationService::new(config.clone()); + // 5. Handle dry-run mode + if args.dry_run { + return handle_dry_run(&service, &request, args, theme.as_ref()); + } + match service.create_task(&request) { Ok(result) => { // 6. Re-index to update the database with the new task @@ -388,9 +390,27 @@ fn output_errors( } /// Handle dry-run mode -#[allow(clippy::unnecessary_wraps)] -fn handle_dry_run(request: &TaskCreationRequest, _args: &AddArgs) -> Result { - // In dry-run mode, we just validate and show what would be created +/// +/// Resolves the request exactly as a real add would — parsing the target +/// file, validating against it, and locating the insert position — and +/// reports the outcome without writing. Reporting the request back +/// unexamined, as this used to, made `--dry-run` a false green: it passed for +/// a `--before` naming a task that did not exist, and the real add then failed +/// on the same argument (GitHub issue #53). +fn handle_dry_run( + service: &TaskCreationService, + request: &TaskCreationRequest, + args: &AddArgs, + theme: Option<&CliTheme>, +) -> Result { + let plan = match service.plan_task(request) { + Ok(plan) => plan, + Err(errors) => { + output_errors(&errors, &args.format, theme)?; + return Ok(1); + } + }; + println!("Validation passed. Task would be created:"); println!(" Title: {}", request.title); @@ -415,13 +435,24 @@ fn handle_dry_run(request: &TaskCreationRequest, _args: &AddArgs) -> Result ParentRef::AppendAtDepth(depth) => println!(" Parent: at depth {depth}"), } - // Position + // Position, as asked for and as resolved match &request.position { InsertPosition::Append => println!(" Position: append"), InsertPosition::AtIndex(idx) => println!(" Position: at index {idx}"), InsertPosition::Before(id) => println!(" Position: before {id}"), InsertPosition::After(id) => println!(" Position: after {id}"), } + match plan.placement.anchor { + InsertAnchor::Line(line) => println!(" Insert at: line {line}"), + // The parser records a task's checkbox and annotation lines but not + // the free-text body underneath it, so the emitter may push the + // insertion further down than this. Saying so beats printing a line + // number that turns out to be wrong. + InsertAnchor::AfterTaskBlock(line) => { + println!(" Insert at: line {line} or below, past the preceding task's body"); + } + InsertAnchor::EndOfTasksSection => println!(" Insert at: end of the ## Tasks section"), + } // Status if let Some(ref status) = request.status { diff --git a/crates/lash-cli/tests/add_command_test.rs b/crates/lash-cli/tests/add_command_test.rs index 113b9b9..442fd20 100644 --- a/crates/lash-cli/tests/add_command_test.rs +++ b/crates/lash-cli/tests/add_command_test.rs @@ -1072,3 +1072,282 @@ fn test_add_reported_id_resolves_as_a_dependency_target() { .success() .stderr(predicate::str::contains("Warning").not()); } + +// --------------------------------------------------------------------- +// Issue #53: `--before`/`--after` reject the qualified ID lash prints, +// and `--dry-run` does not resolve the position at all +// --------------------------------------------------------------------- + +/// A two-task file, so there is something to position against. +fn project_with_two_tasks() -> TestProject { + TestProject::builder() + .with_index("test-project", "Test Project") + .with_file( + "tasks.md", + r#"# Tasks + +@id: tasks + +## Tasks + +- [ ] Alpha task +- [ ] Beta task +"#, + ) + .build() +} + +/// The order top-level task titles appear in `tasks.md`. +fn task_titles(project: &TestProject) -> Vec { + fs::read_to_string(project.file_path("tasks.md")) + .unwrap() + .lines() + .filter_map(|line| line.trim_start().strip_prefix("- [ ] ").map(str::to_string)) + .collect() +} + +#[test] +fn test_add_before_accepts_the_qualified_id_that_show_prints() { + // `lash show` reports `tasks#beta-task`; pasting that back into --before + // used to fail with "task not found" even though the task existed. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("tasks#beta-task") + .assert() + .success(); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Gamma", "Beta task"], + "Gamma should sit between Alpha and Beta" + ); +} + +#[test] +fn test_add_after_accepts_the_qualified_id_that_show_prints() { + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--after") + .arg("tasks#alpha-task") + .assert() + .success(); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Gamma", "Beta task"] + ); +} + +#[test] +fn test_add_before_still_accepts_the_bare_slug() { + // The form that already worked must keep working. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("beta-task") + .assert() + .success(); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Gamma", "Beta task"] + ); +} + +#[test] +fn test_add_before_accepts_the_file_path_as_qualifier() { + // `tasks.md#beta-task` is the other spelling a caller can have in hand, + // since `@depends-on` references are written against paths. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("tasks.md#task:beta-task") + .assert() + .success(); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Gamma", "Beta task"] + ); +} + +#[test] +fn test_add_before_rejects_a_qualifier_naming_a_different_file() { + // Accepting the qualifier must not mean ignoring it: a qualifier naming + // another file means the caller expected the task somewhere it is not, + // and inserting next to a same-named task here would be wrong. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("index#beta-task") + .assert() + .failure() + .stderr(predicate::str::contains("names file 'index'")); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Beta task"], + "nothing should have been written" + ); +} + +#[test] +fn test_add_dry_run_fails_on_a_position_that_does_not_exist() { + // Dry run used to echo the requested position back and exit 0, so it + // passed for arguments the real add rejected. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Epsilon") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("no-such-task-at-all") + .arg("--dry-run") + .assert() + .failure() + .stderr(predicate::str::contains("not found")) + .stdout(predicate::str::contains("Validation passed").not()); +} + +#[test] +fn test_add_dry_run_error_names_the_ids_that_do_exist() { + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Epsilon") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("no-such-task-at-all") + .arg("--dry-run") + .assert() + .failure() + .stderr(predicate::str::contains("alpha-task")) + .stderr(predicate::str::contains("beta-task")); +} + +#[test] +fn test_add_dry_run_reports_the_resolved_insert_line() { + // The point of dry run is to check placement, so it has to report the + // placement it resolved rather than the argument it was handed. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--before") + .arg("tasks#beta-task") + .arg("--dry-run") + .assert() + .success() + .stdout(predicate::str::contains("Validation passed")) + // `- [ ] Beta task` is on line 8 of the fixture. + .stdout(predicate::str::contains("Insert at: line 8")); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Beta task"], + "dry run must not write" + ); +} + +#[test] +fn test_add_dry_run_still_fails_on_other_validation_errors() { + // Position resolution is the new check, but dry run must keep catching + // everything it caught before it reached the file at all. + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--estimate") + .arg("not-a-duration") + .arg("--dry-run") + .assert() + .failure() + .stderr(predicate::str::contains("E_CREATE_INVALID_ESTIMATE")); +} + +#[test] +fn test_add_dry_run_passes_for_a_plain_append() { + let project = project_with_two_tasks(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("add") + .arg("Gamma") + .arg("--file") + .arg("tasks.md") + .arg("--dry-run") + .assert() + .success() + .stdout(predicate::str::contains("Validation passed")); + + assert_eq!( + task_titles(&project), + vec!["Alpha task", "Beta task"], + "dry run must not write" + ); +} diff --git a/crates/lash-core/src/creation/placement.rs b/crates/lash-core/src/creation/placement.rs index 708547c..4886314 100644 --- a/crates/lash-core/src/creation/placement.rs +++ b/crates/lash-core/src/creation/placement.rs @@ -211,11 +211,13 @@ impl PlacementResolver { ctx: &ValidationContext, task_id: &str, ) -> Result { - let task = ctx.resolved_file.tasks.get_task(task_id).ok_or_else(|| { - TaskCreationError::InvalidPosition { - reason: format!("task '{task_id}' not found"), - } - })?; + let task_id = Self::local_position_id(&ctx.resolved_file, task_id)?; + + let task = ctx + .resolved_file + .tasks + .get_task(task_id) + .ok_or_else(|| Self::position_task_not_found(ctx, task_id))?; // Verify task is at the right level (sibling of new task) let siblings = Self::get_siblings(&ctx.resolved_file, ctx.parent_task.as_ref()); @@ -240,11 +242,13 @@ impl PlacementResolver { ctx: &ValidationContext, task_id: &str, ) -> Result { - let task = ctx.resolved_file.tasks.get_task(task_id).ok_or_else(|| { - TaskCreationError::InvalidPosition { - reason: format!("task '{task_id}' not found"), - } - })?; + let task_id = Self::local_position_id(&ctx.resolved_file, task_id)?; + + let task = ctx + .resolved_file + .tasks + .get_task(task_id) + .ok_or_else(|| Self::position_task_not_found(ctx, task_id))?; // Verify task is at the right level (sibling of new task) let siblings = Self::get_siblings(&ctx.resolved_file, ctx.parent_task.as_ref()); @@ -268,6 +272,132 @@ impl PlacementResolver { }) } + /// The local task ID named by a `--before`/`--after` argument + /// + /// Read commands print IDs qualified with the file they live in + /// (`index#beta-task`), and that is the string people paste straight back + /// into `--before`. The target file is already fixed by `-f`, so the + /// qualifier carries no information — but it is still worth checking, + /// because a qualifier naming a *different* file means the caller expected + /// the task to be somewhere it is not, and silently ignoring it would + /// insert next to whatever unrelated task happens to share the slug. + /// + /// Accepts the `#task:` form of the reference syntax too, since + /// `@depends-on` is written as `path/to/file.md#task:id` and the two + /// forms turn up in the same invocation. + fn local_position_id<'a>( + file: &TaskFile, + task_id: &'a str, + ) -> Result<&'a str, TaskCreationError> { + let Some((qualifier, local_id)) = task_id.split_once('#') else { + return Ok(task_id); + }; + + if !Self::qualifier_names_file(file, qualifier) { + return Err(TaskCreationError::InvalidPosition { + reason: format!( + "task '{task_id}' names file '{qualifier}', but the task is being added to '{}'", + Self::file_label(file) + ), + }); + } + + Ok(local_id.strip_prefix("task:").unwrap_or(local_id)) + } + + /// Whether the `file#` part of a qualified ID names this file + /// + /// Matches any of the spellings a caller could reasonably have in hand: + /// the file's own `@id` (what `lash show` and `lash list` print), its + /// file name with or without the `.md` extension, and any trailing + /// portion of its path (so `tasks/backend.md` matches a file indexed at + /// that relative path). + fn qualifier_names_file(file: &TaskFile, qualifier: &str) -> bool { + let qualifier = qualifier.trim(); + if qualifier.is_empty() { + // `#beta-task` — an empty qualifier is just the bare slug written + // the long way, and there is nothing to disagree with. + return true; + } + + if qualifier.eq_ignore_ascii_case(&file.id) { + return true; + } + + if file.id.is_empty() && file.path.as_os_str().is_empty() { + // The placeholder file a new-file request validates against has no + // identity to compare a qualifier with. Accept it and let the + // "not found" path report the real problem: the file holds no + // tasks to position against at all. + return true; + } + + let path = file.path.to_string_lossy().replace('\\', "/"); + let normalized = qualifier.replace('\\', "/"); + let candidates = [ + normalized.clone(), + format!("{normalized}.md"), + // `synthesize_file_id` spells separators as dots, so the dotted + // form of a nested path resolves too. + format!("{}.md", normalized.replace('.', "/")), + ]; + + candidates.iter().any(|candidate| { + path.eq_ignore_ascii_case(candidate) + || path + .strip_suffix(candidate) + .is_some_and(|prefix| prefix.is_empty() || prefix.ends_with('/')) + }) + } + + /// How to name the target file in an error message + fn file_label(file: &TaskFile) -> String { + if file.id.is_empty() { + file.path.display().to_string() + } else { + file.id.clone() + } + } + + /// A "not found" error that names the IDs the caller could have meant + /// + /// The bare "not found" this replaced was actively misleading for the + /// commonest cause — a qualified ID copied out of `lash show` — because + /// the task really did exist. Listing the file's actual IDs makes the + /// mismatch visible without a second command. + fn position_task_not_found(ctx: &ValidationContext, task_id: &str) -> TaskCreationError { + /// Beyond a handful, the list stops helping and starts burying the + /// error, so it is truncated with a count of what was left out. + const MAX_LISTED: usize = 8; + + let siblings = Self::get_siblings(&ctx.resolved_file, ctx.parent_task.as_ref()); + let listed: Vec<&str> = siblings + .iter() + .take(MAX_LISTED) + .map(|t| t.id.as_str()) + .collect(); + + let reason = format!( + "task '{task_id}' not found in '{}'", + Self::file_label(&ctx.resolved_file) + ); + + if listed.is_empty() { + return TaskCreationError::InvalidPosition { reason }; + } + + let available = listed.join(", "); + let elided = if siblings.len() > MAX_LISTED { + format!(", … ({} more)", siblings.len() - MAX_LISTED) + } else { + String::new() + }; + + TaskCreationError::InvalidPosition { + reason: format!("{reason}; available at this level: {available}{elided}"), + } + } + /// Find the end of a task's subtree (last descendant line number) /// /// Returns the line number where the task and all its descendants end. @@ -1162,6 +1292,183 @@ mod tests { assert_eq!(append_line_for(&file), 9); } + // ------------------------------------------------------------------ + // Qualified position IDs (GitHub issue #53) + // + // `lash show` and `lash list` print `file#slug`, so that is the string + // people paste into `--before`/`--after`. It used to be rejected as + // "not found" even though the task existed. + // ------------------------------------------------------------------ + + /// A two-task file at `test.md` with id `test-file`. + fn file_with_two_tasks() -> TaskFile { + let mut tasks = TaskTree::new(); + tasks + .add_task( + TaskBuilder::new("Alpha task") + .id("alpha-task") + .order_index(0) + .line_number(6) + .build() + .unwrap(), + ) + .unwrap(); + tasks + .add_task( + TaskBuilder::new("Beta task") + .id("beta-task") + .order_index(1) + .line_number(8) + .build() + .unwrap(), + ) + .unwrap(); + create_test_file(tasks) + } + + /// Resolve `--before {position}` against [`file_with_two_tasks`]. + fn resolve_before(position: &str) -> Result { + let config = ConfigBuilder::new().build().unwrap(); + let validator = TaskValidator::new(config); + let file = file_with_two_tasks(); + + let request = TaskCreationRequestBuilder::new("New task") + .before(position) + .build(); + let ctx = validator.validate(&request, Some(&file)).unwrap(); + PlacementResolver::resolve(&ctx, &request) + } + + #[test] + fn test_before_accepts_an_id_qualified_with_the_file_id() { + let placement = resolve_before("test-file#beta-task").unwrap(); + assert_eq!(placement.order_index, 1); + assert_eq!(placement.anchor, InsertAnchor::Line(8)); + } + + #[test] + fn test_before_accepts_an_id_qualified_with_the_file_name() { + assert_eq!(resolve_before("test.md#beta-task").unwrap().order_index, 1); + assert_eq!(resolve_before("test#beta-task").unwrap().order_index, 1); + } + + #[test] + fn test_before_accepts_the_task_prefixed_reference_form() { + // `@depends-on` is written as `path/to/file.md#task:id`, and both + // forms turn up in the same invocation. + assert_eq!( + resolve_before("test.md#task:beta-task") + .unwrap() + .order_index, + 1 + ); + } + + #[test] + fn test_before_accepts_a_bare_slug_unchanged() { + assert_eq!(resolve_before("beta-task").unwrap().order_index, 1); + } + + #[test] + fn test_before_rejects_a_qualifier_naming_another_file() { + // Accepting the qualifier must not mean discarding it. A qualifier + // pointing elsewhere means the caller expected the task in a + // different file, and positioning against a same-named task here + // would silently do the wrong thing. + let err = resolve_before("other-file#beta-task").unwrap_err(); + let TaskCreationError::InvalidPosition { reason } = err else { + panic!("Expected InvalidPosition error"); + }; + assert!(reason.contains("names file 'other-file'"), "got: {reason}"); + assert!(reason.contains("test-file"), "got: {reason}"); + } + + #[test] + fn test_before_treats_an_empty_qualifier_as_a_bare_slug() { + // `#beta-task` is the bare slug written the long way; there is no + // file claim to disagree with. + assert_eq!(resolve_before("#beta-task").unwrap().order_index, 1); + } + + #[test] + fn test_position_not_found_error_names_the_available_ids() { + // The bare "not found" was actively misleading when the ID had been + // copied out of `lash show`, because the task really did exist. + let err = resolve_before("no-such-task").unwrap_err(); + let TaskCreationError::InvalidPosition { reason } = err else { + panic!("Expected InvalidPosition error"); + }; + assert!(reason.contains("not found in 'test-file'"), "got: {reason}"); + assert!(reason.contains("alpha-task"), "got: {reason}"); + assert!(reason.contains("beta-task"), "got: {reason}"); + } + + #[test] + fn test_position_not_found_error_truncates_a_long_id_list() { + // Past a handful the list buries the error instead of explaining it. + let config = ConfigBuilder::new().build().unwrap(); + let validator = TaskValidator::new(config); + + let mut tasks = TaskTree::new(); + for i in 0..12 { + tasks + .add_task( + TaskBuilder::new(format!("Task {i}")) + .id(format!("task-{i}")) + .order_index(i) + .line_number(6 + i) + .build() + .unwrap(), + ) + .unwrap(); + } + let file = create_test_file(tasks); + + let request = TaskCreationRequestBuilder::new("New task") + .before("no-such-task") + .build(); + let ctx = validator.validate(&request, Some(&file)).unwrap(); + let err = PlacementResolver::resolve(&ctx, &request).unwrap_err(); + + let TaskCreationError::InvalidPosition { reason } = err else { + panic!("Expected InvalidPosition error"); + }; + assert!(reason.contains("(4 more)"), "got: {reason}"); + } + + #[test] + fn test_after_accepts_an_id_qualified_with_the_file_id() { + let config = ConfigBuilder::new().build().unwrap(); + let validator = TaskValidator::new(config); + let file = file_with_two_tasks(); + + let request = TaskCreationRequestBuilder::new("New task") + .after("test-file#alpha-task") + .build(); + let ctx = validator.validate(&request, Some(&file)).unwrap(); + let placement = PlacementResolver::resolve(&ctx, &request).unwrap(); + + assert_eq!(placement.order_index, 1); + } + + #[test] + fn test_after_rejects_a_qualifier_naming_another_file() { + let config = ConfigBuilder::new().build().unwrap(); + let validator = TaskValidator::new(config); + let file = file_with_two_tasks(); + + let request = TaskCreationRequestBuilder::new("New task") + .after("other-file#alpha-task") + .build(); + let ctx = validator.validate(&request, Some(&file)).unwrap(); + let err = PlacementResolver::resolve(&ctx, &request).unwrap_err(); + + let TaskCreationError::InvalidPosition { reason } = err else { + panic!("Expected InvalidPosition error"); + }; + assert!(reason.contains("names file 'other-file'"), "got: {reason}"); + } + #[test] fn test_append_after_task_with_multiline_owner_and_estimate() { // The same folding applies to every single-value annotation, not just diff --git a/crates/lash-core/src/creation/service.rs b/crates/lash-core/src/creation/service.rs index 9d7c3a9..5cab066 100644 --- a/crates/lash-core/src/creation/service.rs +++ b/crates/lash-core/src/creation/service.rs @@ -12,8 +12,23 @@ use lash_types::file::TaskFile; use std::path::{Path, PathBuf}; use super::emitter::MarkdownEmitter; -use super::placement::PlacementResolver; -use super::validation::TaskValidator; +use super::placement::{PlacementInfo, PlacementResolver}; +use super::validation::{TaskValidator, ValidationContext}; + +/// Everything decided about a task before anything is written +/// +/// Produced by [`TaskCreationService::plan_task`]. `create_task` turns a plan +/// into a file write; `lash add --dry-run` reports it and stops. Both go +/// through the same code so a dry run cannot pass on a request the real add +/// would reject. +#[derive(Debug, Clone)] +pub struct TaskCreationPlan { + /// Validation context: resolved file, parent, depth, existing IDs + pub context: ValidationContext, + + /// Where the task would be inserted + pub placement: PlacementInfo, +} /// Service that orchestrates task creation /// @@ -109,18 +124,13 @@ impl TaskCreationService { &self, request: &TaskCreationRequest, ) -> Result> { - // Step 1: Load target file (if existing file) - let file_content = self.load_target_file(&request.file_target)?; - - // Step 2: Validate request - let validator = TaskValidator::new(self.config.clone()); - let ctx = validator.validate(request, file_content.as_ref())?; - - // Step 3: Resolve placement - let placement = PlacementResolver::resolve(&ctx, request).map_err(|e| vec![e])?; + // Steps 1-3: load the target file, validate, resolve placement + let plan = self.plan_task(request)?; + let TaskCreationPlan { context, placement } = plan; // Step 4: Emit to markdown - let mut result = MarkdownEmitter::emit(request, &ctx, &placement).map_err(|e| vec![e])?; + let mut result = + MarkdownEmitter::emit(request, &context, &placement).map_err(|e| vec![e])?; // Step 5: Report the ID the parser gives the task, not the one the // emitter guessed. The two agree on the slug now that both go through @@ -136,6 +146,67 @@ impl TaskCreationService { Ok(result) } + /// Work out what would be created, without writing anything + /// + /// Runs every check `create_task` runs — loading and parsing the target + /// file, validating the request against it, resolving the insert position + /// — and stops short of the write. This is what backs + /// `lash add --dry-run`, which previously printed the request back + /// unexamined and so reported success for positions that did not exist + /// (GitHub issue #53). + /// + /// # Arguments + /// + /// * `request` - The task creation request to plan + /// + /// # Returns + /// + /// * `Ok(TaskCreationPlan)` - The validated context and resolved placement + /// * `Err(Vec)` - Every error the real add would raise + /// + /// # Errors + /// + /// Returns the same errors as [`Self::create_task`], minus the ones that + /// can only arise from the write itself (I/O failures). + /// + /// # Examples + /// + /// ```no_run + /// use lash_core::creation::service::TaskCreationService; + /// use lash_types::config::ConfigBuilder; + /// use lash_types::creation::TaskCreationRequestBuilder; + /// use std::path::PathBuf; + /// + /// let config = ConfigBuilder::new().build().unwrap(); + /// let service = TaskCreationService::new(config); + /// + /// let request = TaskCreationRequestBuilder::new("New task") + /// .file_path(PathBuf::from("tasks.md")) + /// .before("existing-task") + /// .build(); + /// + /// match service.plan_task(&request) { + /// Ok(plan) => println!("would insert at index {}", plan.placement.order_index), + /// Err(errors) => eprintln!("{} problem(s)", errors.len()), + /// } + /// ``` + pub fn plan_task( + &self, + request: &TaskCreationRequest, + ) -> Result> { + // Step 1: Load target file (if existing file) + let file_content = self.load_target_file(&request.file_target)?; + + // Step 2: Validate request + let validator = TaskValidator::new(self.config.clone()); + let context = validator.validate(request, file_content.as_ref())?; + + // Step 3: Resolve placement + let placement = PlacementResolver::resolve(&context, request).map_err(|e| vec![e])?; + + Ok(TaskCreationPlan { context, placement }) + } + /// The ID the parser assigns to the task written at `line_number` /// /// Returns `None` if the file cannot be re-read or holds no task on that diff --git a/crates/lash-types/src/creation_errors.rs b/crates/lash-types/src/creation_errors.rs index af1b5eb..3b20463 100644 --- a/crates/lash-types/src/creation_errors.rs +++ b/crates/lash-types/src/creation_errors.rs @@ -226,7 +226,7 @@ impl TaskCreationError { "an agent note may span several lines, but each line must have non-whitespace content and must not begin with '@'".to_string() } Self::InvalidPosition { .. } => { - "use a valid position: Append, AtIndex, Before, or After with an existing task ID".to_string() + "--before/--after take a task ID from the target file, either bare ('beta-task') or qualified with the file ('index#beta-task'); --at-index takes a 0-based position among siblings".to_string() } Self::IoError { .. } => { "check file permissions and disk space".to_string() @@ -407,7 +407,10 @@ mod tests { }; assert_eq!(err.error_code(), "E_CREATE_INVALID_POSITION"); assert!(err.message().contains("referenced task not found")); - assert!(err.help().contains("valid position")); + // The help names both accepted spellings of a position ID, because + // the qualified one is what `lash show` prints (GitHub issue #53). + assert!(err.help().contains("--before/--after")); + assert!(err.help().contains("index#beta-task")); } #[test] diff --git a/crates/lash-types/src/error_explanations.rs b/crates/lash-types/src/error_explanations.rs index c7b8e49..910450b 100644 --- a/crates/lash-types/src/error_explanations.rs +++ b/crates/lash-types/src/error_explanations.rs @@ -522,11 +522,11 @@ pub fn explain_error(code: &str) -> Option { codes::E_CREATE_INVALID_POSITION => Some(ErrorExplanation { code: codes::E_CREATE_INVALID_POSITION, summary: "Insert position is invalid", - description: "The position specified with --before or --after references a task that doesn't exist or is invalid.", + description: "The position specified with --before or --after references a task that doesn't exist in the target file, lives at a different nesting level, or belongs to another file.", why_it_matters: "Task ordering requires valid position references to maintain the correct sequence.", - how_to_fix: "Ensure the task ID used with --before or --after exists in the target file, or omit these options to append at the end.", - example_bad: Some("lash add \"Task\" --after nonexistent-task"), - example_good: Some("lash add \"Task\" --after existing-task-id\nlash add \"Task\" # Appends at end"), + how_to_fix: "Use a task ID from the target file. Both the bare ID and the qualified 'file#id' form that `lash show` prints are accepted, but the file part must name the file you are adding to. Omit these options to append at the end.", + example_bad: Some("lash add \"Task\" --after nonexistent-task\nlash add \"Task\" -f a.md --after other-file#some-task"), + example_good: Some("lash add \"Task\" --after existing-task-id\nlash add \"Task\" -f lash.index.md --after index#existing-task-id\nlash add \"Task\" # Appends at end"), }), codes::E_CREATE_IO_ERROR => Some(ErrorExplanation { diff --git a/devlog.md b/devlog.md index 9bffe20..7bd0357 100644 --- a/devlog.md +++ b/devlog.md @@ -2614,3 +2614,41 @@ 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. + +## `lash add --before` rejected the ID lash prints, and `--dry-run` never looked (#53, 2026-08-11) + +Two halves of one report, and the second is what made the first expensive. + +`lash show` and `lash list` qualify task IDs with their file — `index#beta-task` +— so that is the string people have in hand. `--before`/`--after` only accepted +the bare slug and reported the qualified form as "task not found", which reads +as the task being missing rather than the argument being spelled the way the +tool spells it. `--depends-on` on the same command line accepts the qualified +form, so a single invocation could need both. + +Position IDs now go through `PlacementResolver::local_position_id`, which +strips a `file#` qualifier when it names the target file and errors when it +names a different one. Accepting the qualifier is not the same as ignoring it: +a qualifier pointing at another file means the caller expected the task +somewhere it is not, and positioning against whatever local task happens to +share the slug would be silently wrong. The qualifier matches against the +file's `@id`, its name with or without `.md`, and any trailing portion of its +path, and a `#task:` prefix on the local part is tolerated because that is how +`@depends-on` references are written. + +The not-found error now names the IDs that do exist at that level. The bare +"not found" was actively misleading for the commonest cause, since the task +really did exist. + +`--dry-run` was the worse half. It printed the request back field by field and +exited 0 — it never opened the target file, so it reported success for a +`--before` naming a task that did not exist. Using it to check placement, which +is the one thing it is for, confirmed an argument the real add then rejected. + +`create_task` now splits into `plan_task` (load, validate, resolve placement) +and the emit that follows it, and dry run calls `plan_task`. There is no +separate dry-run code path left to drift out of agreement with the real one. +Dry run also reports the line it resolved rather than the argument it was +handed — exact when a following task fixes the position, and stated as a lower +bound when the emitter still has to step past a preceding task's free-text +body, which the parsed model does not record. diff --git a/docs/error-codes.md b/docs/error-codes.md index 5ce8d95..c07d856 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -561,9 +561,15 @@ Task A depends on B, B depends on A **Example:** ```bash lash add "Task" --before nonexistent-task + +# Or: a qualifier naming a file other than the one being added to +lash add "Task" -f tasks.md --before other-file#some-task ``` -**How to fix:** Use a valid task ID for `--before` or `--after` position +**How to fix:** Use a task ID from the target file. Both the bare ID +(`beta-task`) and the qualified form `lash show` prints (`tasks#beta-task`) +are accepted; the file part must name the file you are adding to. The error +lists the IDs available at that level. --- diff --git a/docs/task-creation.md b/docs/task-creation.md index 098c781..5cd9fe6 100644 --- a/docs/task-creation.md +++ b/docs/task-creation.md @@ -63,6 +63,13 @@ lash add [OPTIONS] | `--after <ID>` | | Insert after this task ID | | `--before <ID>` | | Insert before this task ID | +**Position IDs**: `--before` and `--after` accept either the bare slug +(`beta-task`) or the file-qualified form that `lash show` and `lash list` +print (`tasks#beta-task`). The file is already fixed by `--file`, so the +qualifier is redundant — but a qualifier naming a *different* file is +rejected rather than ignored, since it means the task was expected somewhere +it is not. + #### Metadata Options | Option | Short | Description | @@ -149,11 +156,31 @@ lash add "Fix regression bug" \ ```bash # Check if task would be valid without creating it lash add "Test task" --file tasks.md --dry-run +# Validation passed. Task would be created: +# Title: Test task +# File: /path/to/tasks.md +# Parent: <none> +# Position: append +# Insert at: line 15 +``` + +A dry run resolves the request the same way the real add does: it parses the +target file, validates against it, and locates the insert position. A +`--before`/`--after` naming a task that does not exist fails the dry run +rather than passing and then failing on the write: -# On success: "Validation passed. Task would be created at line 15" -# On failure: Shows validation errors +```bash +lash add "Test task" --file tasks.md --before no-such-task --dry-run +# Error [E_CREATE_INVALID_POSITION]: invalid insert position: task +# 'no-such-task' not found in 'tasks'; available at this level: alpha-task, +# beta-task ``` +The reported `Insert at` line is exact when the position is fixed by a +following task. When the task is appended after another one, the emitter +steps past that task's free-text body first, so the dry run reports the line +as a lower bound (`line N or below`). + #### Creating Tasks Out of Dependency Order ```bash diff --git a/docs/user-guide.md b/docs/user-guide.md index 32aa449..86a21c1 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -772,8 +772,8 @@ lash add --interactive - `--file-title TEXT` - Title for new file - `--file-description TEXT` - Description for new file - `--parent ID` - Parent task ID -- `--after ID` - Insert after this task -- `--before ID` - Insert before this task +- `--after ID` - Insert after this task (bare `id` or qualified `file#id`) +- `--before ID` - Insert before this task (bare `id` or qualified `file#id`) - `--label TAG` - Add label (repeatable) - `--owner NAME` - Set owner - `--estimate DURATION` - Set estimate (e.g., 30m, 2h, 1d)