Merge remote-tracking branch 'origin/main' into fix/54-id-derivation-drift

# Conflicts:
#	CHANGELOG.md
#	devlog.md

Author: fohara

CHANGELOG.md

@@ -42,6 +42,19 @@ While the major version is 0, minor version bumps may contain breaking changes.
   derivation change moved, rather than at a task that is missing. Without it
   the error reads as a false positive: the ID it names is exactly the one
   `lash show` prints back.
+- `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
 

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<i32> {
         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<i32> {
-    // 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<i32> {
+    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<i32>
         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 {

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<String> {
+    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"
+    );
+}