diff --git a/crates/lash-agent/src/content.rs b/crates/lash-agent/src/content.rs index 1297d00..c504abf 100644 --- a/crates/lash-agent/src/content.rs +++ b/crates/lash-agent/src/content.rs @@ -256,13 +256,21 @@ is `done` or `waived`. @depends-on: features/auth.md#task:auth.session ``` -Forms: -- `path/to/file.md` — depend on the entire file -- `path/to/file.md#task:` — depend on a specific task -- `#task:` — depend on a task in the same file - -Validate references with `lash check-links`. Auto-fix typos in references -with `lash check-links --fix`. +Forms (the `task:` prefix on a fragment is optional everywhere): +- `` — a task by its `@id` (same file, or unique in the project) +- `#task:` / `#` — a task in the same file +- `#task:` / `#` — a task in another file, by file `@id` +- `path/to/file.md#task:` — a task in another file, by path +- `path/to/file.md` — the entire file +- `` — the entire file, by file `@id` + +List several dependencies with commas or repeated lines: +`@depends-on: auth.session, payment.gateway`. + +`lash complete` refuses while any dependency is still open (pass `--force` +to override). Validate references with `lash check-links` — it reports the +same broken references as `lash lint`. Auto-fix typos with +`lash check-links --fix`. ## `@doc:` — Documentation References diff --git a/crates/lash-cli/src/cli.rs b/crates/lash-cli/src/cli.rs index 224d7ef..ef9cf51 100644 --- a/crates/lash-cli/src/cli.rs +++ b/crates/lash-cli/src/cli.rs @@ -420,6 +420,12 @@ pub enum Commands { /// and are never auto-completed. #[arg(long)] cascade: bool, + + /// Complete even when a resolvable @depends-on target is still open. + /// By default completion is refused until dependencies are done or + /// waived. + #[arg(long)] + force: bool, }, /// Mark one or more tasks as in-progress @@ -1113,11 +1119,13 @@ mod tests { task_ids, dry_run, cascade, + force, } = cli.command { assert_eq!(task_ids, vec!["test#task-1"]); assert!(!dry_run); assert!(!cascade); + assert!(!force); } else { panic!("Expected Complete command"); } @@ -1160,6 +1168,16 @@ mod tests { } } + #[test] + fn test_cli_parse_complete_force() { + let cli = LashCli::try_parse_from(["lash", "complete", "--force", "test#task-1"]).unwrap(); + if let Commands::Complete { force, .. } = cli.command { + assert!(force); + } else { + panic!("Expected Complete command"); + } + } + #[test] fn test_cli_parse_complete_requires_task_id() { let result = LashCli::try_parse_from(["lash", "complete"]); diff --git a/crates/lash-cli/src/commands/check_links/core.rs b/crates/lash-cli/src/commands/check_links/core.rs index 4abf9f1..9eb88d2 100644 --- a/crates/lash-cli/src/commands/check_links/core.rs +++ b/crates/lash-cli/src/commands/check_links/core.rs @@ -6,10 +6,12 @@ use anyhow::{Context, Result}; use lash_cli::error_reporter::{ErrorDisplayMode, ErrorReporter, ErrorReporterConfig}; use lash_cli::formatter::{OutputFormat, Verbosity}; use lash_cli::theme::CliTheme; +use lash_core::dependency::reference::resolve_reference; use lash_core::linter::rules::semantic::BrokenDocFragmentRule; use lash_core::linter::{LintContext, LintDiagnostic, LintRule}; use lash_core::parser::parse_file; use lash_db::open_database; +use lash_types::dependency::DependencyKind; use lash_types::error::LashError; use lash_types::{LashConfig, TaskFile}; use serde::{Deserialize, Serialize}; @@ -208,6 +210,77 @@ pub fn find_broken_links(conn: impl AsRef) -> Result Vec { + let Ok(markdown_files) = discover_markdown_files(&[project_root.to_path_buf()], true) else { + return Vec::new(); + }; + + let config = LashConfig::from_root(project_root).unwrap_or_default(); + + let mut files: HashMap = HashMap::new(); + for path in &markdown_files { + let Ok(file) = parse_file(path, &config) else { + continue; + }; + let relative = path + .strip_prefix(project_root) + .unwrap_or(path) + .to_path_buf(); + files.insert(relative, file); + } + + let mut by_file: Vec = Vec::new(); + + for (rel_path, file) in &files { + let ctx = LintContext::new(&config, rel_path.clone(), &files); + let mut links: Vec = Vec::new(); + + for task in file.tasks.tasks() { + for dep in &task.metadata.depends_on { + if matches!( + dep.kind, + DependencyKind::Hierarchy | DependencyKind::Directory + ) { + continue; + } + let resolve_path = |rel: &str| ctx.resolve_path(Path::new(rel)); + if resolve_reference(&dep.target, rel_path, &file.id, &files, resolve_path).is_err() + { + links.push(BrokenLink { + from_task_full_id: lash_types::make_full_id(&file.id, &task.id), + from_file_path: rel_path.display().to_string(), + raw_ref: dep.target.clone(), + kind: dep.kind.as_str().to_string(), + }); + } + } + } + + if !links.is_empty() { + by_file.push(FileLinks { + file_path: rel_path.display().to_string(), + count: links.len(), + links, + }); + } + } + + by_file.sort_by(|a, b| a.file_path.cmp(&b.file_path)); + by_file +} + /// Walk every task file under `project_root` and report `@doc:` annotations /// whose `#fragment` does not resolve to a heading in the target document. /// @@ -974,6 +1047,70 @@ mod tests { ); } + // Issue #19: check-links must flag @depends-on references that lint flags. + #[test] + fn test_find_broken_dependencies_flags_unresolvable_ref() { + use tempfile::TempDir; + + let temp = TempDir::new().unwrap(); + let root = temp.path(); + + std::fs::write(root.join("lash.index.md"), "# Index\n").unwrap(); + std::fs::write( + root.join("tasks.md"), + "# Tasks\n\n@id: repro\n\n## Tasks\n\n\ + - [ ] Base\n \ + @id: base-task\n\ + - [ ] Dependent\n \ + @id: dep\n \ + @depends-on: does-not-exist\n", + ) + .unwrap(); + + let by_file = find_broken_dependencies(root); + assert_eq!(by_file.len(), 1, "expected one file with a broken dep"); + assert_eq!(by_file[0].count, 1); + assert_eq!(by_file[0].links[0].raw_ref, "does-not-exist"); + } + + // Issue #19: forms that lint accepts must NOT be reported by check-links. + #[test] + fn test_find_broken_dependencies_accepts_valid_forms() { + use tempfile::TempDir; + + let temp = TempDir::new().unwrap(); + let root = temp.path(); + + std::fs::write(root.join("lash.index.md"), "# Index\n").unwrap(); + std::fs::write( + root.join("tasks.md"), + "# Tasks\n\n@id: repro\n\n## Tasks\n\n\ + - [ ] Base\n \ + @id: base-task\n\ + - [ ] Other\n \ + @id: other-task\n\ + - [ ] Bare id\n \ + @id: d1\n \ + @depends-on: base-task\n\ + - [ ] Samefile form\n \ + @id: d2\n \ + @depends-on: #task:base-task\n\ + - [ ] File id form\n \ + @id: d3\n \ + @depends-on: repro#task:base-task\n\ + - [ ] Comma list\n \ + @id: d4\n \ + @depends-on: base-task, other-task\n", + ) + .unwrap(); + + let by_file = find_broken_dependencies(root); + assert!( + by_file.is_empty(), + "all references resolve, expected no broken deps, got {by_file:?}" + ); + } + #[test] fn test_find_broken_doc_fragments_clean_project_returns_empty() { use tempfile::TempDir; diff --git a/crates/lash-cli/src/commands/check_links/mod.rs b/crates/lash-cli/src/commands/check_links/mod.rs index 58a81ca..a14a009 100644 --- a/crates/lash-cli/src/commands/check_links/mod.rs +++ b/crates/lash-cli/src/commands/check_links/mod.rs @@ -84,6 +84,24 @@ pub fn execute(args: &CheckLinksArgs) -> Result { // Find all broken links let mut report = core::find_broken_links(&db_path).context("Failed to find broken links")?; + // Explicit @depends-on edges aren't stored as NULL-target rows, so the DB + // query above never sees them. Reparse the tree and validate each + // reference with the same resolver `lash lint` uses, then merge the + // results in so the two commands agree (GitHub issue #19). + for file_links in core::find_broken_dependencies(&project_root) { + report.total_broken += file_links.count; + if let Some(existing) = report + .by_file + .iter_mut() + .find(|fl| fl.file_path == file_links.file_path) + { + existing.count += file_links.count; + existing.links.extend(file_links.links); + } else { + report.by_file.push(file_links); + } + } + // Additionally scan for broken @doc: fragment references. These are // semantic warnings raised by `lash lint`, but `check-links` is the // natural canonical "are my references intact?" command, so it would be diff --git a/crates/lash-cli/src/commands/complete.rs b/crates/lash-cli/src/commands/complete.rs index 3d588c1..3535924 100644 --- a/crates/lash-cli/src/commands/complete.rs +++ b/crates/lash-cli/src/commands/complete.rs @@ -7,18 +7,23 @@ use anyhow::{Context, Result}; use lash_cli::error_reporter::{ErrorDisplayMode, ErrorReporter, ErrorReporterConfig}; use lash_cli::formatter::{OutputFormat, Verbosity}; use lash_cli::theme::CliTheme; +use lash_core::dependency::reference::resolve_reference; use lash_core::fuzzy::FuzzyMatcher; +use lash_core::linter::LintContext; +use lash_core::parser::parse_file; use lash_db::{open_database, FileRepository, Indexer, IndexerConfig, TaskRepository}; use lash_types::config::LashConfig; +use lash_types::dependency::DependencyKind; use lash_types::error::LashError; -use lash_types::TaskStatus; +use lash_types::{make_full_id, Task, TaskFile, TaskStatus}; use regex::Regex; use serde::Serialize; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::fs; use std::path::{Path, PathBuf}; -use crate::utils::file_discovery::find_project_root; +use crate::utils::file_discovery::{discover_markdown_files, find_project_root}; +use crate::utils::task_target::TargetError; /// Arguments for the complete command #[derive(Debug, Clone)] @@ -34,6 +39,10 @@ pub struct CompleteArgs { /// the silent footgun where the parent flips to `[x]` while its visible /// sub-checkboxes stay `[ ]`. pub cascade: bool, + /// Complete even when a resolvable `@depends-on` target is still open. + /// When false (the default) completion is refused until every dependency + /// is done or waived. + pub force: bool, /// Output JSON diagnostics pub json: bool, /// Disable colored output @@ -81,6 +90,99 @@ pub struct CompleteError { pub suggestions: Vec, } +/// A dependency of the task being completed that is not yet done or waived. +#[derive(Debug, Clone)] +struct UnmetDep { + /// Target task full id (`file-id#task-id`). + full_id: String, + /// Target task title, for the refusal message. + title: String, + /// Target task status string (e.g. `open`, `in-progress`). + status: String, +} + +/// Reparse every task file under `project_root` into a resolver-shaped map. +/// +/// Explicit `@depends-on` edges aren't stored in the index, so enforcing them +/// (GitHub issue #17) requires reading the current Markdown. Unparseable files +/// are skipped — `lash lint` surfaces those separately. +fn load_project(project_root: &Path) -> (LashConfig, HashMap) { + let config = LashConfig::from_root(project_root).unwrap_or_default(); + let mut files: HashMap = HashMap::new(); + if let Ok(markdown_files) = discover_markdown_files(&[project_root.to_path_buf()], true) { + for path in &markdown_files { + if let Ok(file) = parse_file(path, &config) { + let relative = path + .strip_prefix(project_root) + .unwrap_or(path) + .to_path_buf(); + files.insert(relative, file); + } + } + } + (config, files) +} + +/// Find the parsed task (and its file path) whose full id equals `full_id`. +fn find_task_by_full_id<'a>( + project: &'a HashMap, + full_id: &str, +) -> Option<(&'a PathBuf, &'a TaskFile, &'a Task)> { + for (path, file) in project { + for task in file.tasks.tasks() { + if make_full_id(&file.id, &task.id) == full_id { + return Some((path, file, task)); + } + } + } + None +} + +/// Compute the dependencies of `source_full_id` that are not yet done or +/// waived, resolving each `@depends-on` reference with the shared resolver so +/// the gate matches what `lash lint`/`check-links` report. +fn find_unmet_dependencies( + config: &LashConfig, + project: &HashMap, + source_full_id: &str, +) -> Vec { + let Some((src_path, src_file, src_task)) = find_task_by_full_id(project, source_full_id) else { + return Vec::new(); + }; + + let ctx = LintContext::new(config, src_path.clone(), project); + let mut unmet = Vec::new(); + + for dep in &src_task.metadata.depends_on { + if matches!( + dep.kind, + DependencyKind::Hierarchy | DependencyKind::Directory + ) { + continue; + } + let resolve_path = |rel: &str| ctx.resolve_path(Path::new(rel)); + let Ok(resolution) = + resolve_reference(&dep.target, src_path, &src_file.id, project, resolve_path) + else { + // Broken references are reported by lint/check-links, not here. + continue; + }; + for target_full_id in resolution.full_ids() { + if let Some((_, _, target)) = find_task_by_full_id(project, &target_full_id) { + if !matches!(target.status, TaskStatus::Done | TaskStatus::Waived) { + unmet.push(UnmetDep { + full_id: target_full_id, + title: target.title.clone(), + status: target.status.as_str().to_string(), + }); + } + } + } + } + + unmet +} + /// Execute the complete command /// /// # Arguments @@ -174,6 +276,14 @@ pub fn execute(args: &CompleteArgs) -> Result { .collect() }; + // Unless --force, reparse the tree once so completion can be gated on + // unmet @depends-on targets (GitHub issue #17). + let project = if args.force { + None + } else { + Some(load_project(&project_root)) + }; + // Process each task ID let mut results: Vec = Vec::new(); let mut errors: Vec = Vec::new(); @@ -186,6 +296,7 @@ pub fn execute(args: &CompleteArgs) -> Result { &project_root, args.dry_run, args.cascade, + project.as_ref(), ) { Ok(result) => results.push(result), Err(error) => errors.push(error), @@ -223,6 +334,7 @@ pub fn execute(args: &CompleteArgs) -> Result { } /// Process a single task ID +#[allow(clippy::too_many_lines)] fn process_task( task_id: &str, task_repo: &TaskRepository, @@ -230,11 +342,12 @@ fn process_task( project_root: &Path, dry_run: bool, cascade: bool, + project: Option<&(LashConfig, HashMap)>, ) -> std::result::Result { - // Try to find the task - let task = match task_repo.get_by_full_id(task_id) { - Ok(Some(task)) => task, - Ok(None) => { + // Try to find the task by full id or bare @id. + let task = match crate::utils::task_target::resolve_task_target(task_repo, task_id) { + Ok(task) => task, + Err(TargetError::NotFound) => { // Try fuzzy matching let all_task_ids = task_repo.get_all_full_ids().unwrap_or_default(); let suggestions = find_similar_task_ids(task_id, &all_task_ids); @@ -246,7 +359,18 @@ fn process_task( suggestions: suggestions.into_iter().map(|(id, _)| id).collect(), }); } - Err(e) => { + Err(TargetError::Ambiguous(candidates)) => { + return Err(CompleteError { + task_id: task_id.to_string(), + code: "E_AMBIGUOUS".to_string(), + message: format!( + "Task @id '{task_id}' is ambiguous; matches {} tasks", + candidates.len() + ), + suggestions: candidates, + }); + } + Err(TargetError::Db(e)) => { return Err(CompleteError { task_id: task_id.to_string(), code: "E_DB_ERROR".to_string(), @@ -279,6 +403,30 @@ fn process_task( } } + // Refuse completion while a resolvable @depends-on target is still open + // (GitHub issue #17). Skipped entirely when --force was passed (project + // is None in that case). + if let Some((config, proj)) = project { + let unmet = find_unmet_dependencies(config, proj, &task.full_id); + if !unmet.is_empty() { + let list = unmet + .iter() + .map(|d| format!("{} '{}' [{}]", d.full_id, d.title, d.status)) + .collect::>() + .join(", "); + return Err(CompleteError { + task_id: task_id.to_string(), + code: "E_DEP_UNMET".to_string(), + message: format!( + "Task '{}' has {} unmet dependency(ies): {list}. Pass --force to override.", + task.full_id, + unmet.len() + ), + suggestions: vec![], + }); + } + } + // Get file information let file = match file_repo.get_by_db_id(task.file_id) { Ok(Some(file)) => file, @@ -766,6 +914,85 @@ fn output_text_results( mod tests { use super::*; + fn task(id: &str, status: TaskStatus, deps: &[&str]) -> Task { + use lash_types::dependency::{DependencyKind, DependencyRef}; + use lash_types::TaskMetadata; + Task { + id: id.to_string(), + has_explicit_id: true, + title: id.to_string(), + status, + depth: 0, + parent_id: None, + order_index: 0, + line_number: 0, + metadata: TaskMetadata { + depends_on: deps + .iter() + .map(|d| DependencyRef::new((*d).to_string(), DependencyKind::ExplicitId)) + .collect(), + ..Default::default() + }, + body: None, + contextual_notes: Vec::new(), + } + } + + fn project_with(tasks: Vec) -> HashMap { + use lash_types::{FileMetadata, TaskTree}; + use std::time::SystemTime; + let mut tree = TaskTree::new(); + for t in tasks { + tree.add_task(t).unwrap(); + } + let file = TaskFile { + path: PathBuf::from("tasks.md"), + title: "T".to_string(), + id: "repro".to_string(), + metadata: FileMetadata::default(), + description: None, + description_agent_notes: Vec::new(), + tasks: tree, + hash: "h".to_string(), + mtime: SystemTime::now(), + }; + let mut map = HashMap::new(); + map.insert(PathBuf::from("tasks.md"), file); + map + } + + // Issue #17: an open dependency must be reported as unmet. + #[test] + fn test_find_unmet_dependencies_flags_open_dependency() { + let config = LashConfig::default(); + let project = project_with(vec![ + task("base-task", TaskStatus::Open, &[]), + task("dep-task", TaskStatus::Open, &["base-task"]), + ]); + + let unmet = find_unmet_dependencies(&config, &project, "repro#dep-task"); + assert_eq!(unmet.len(), 1); + assert_eq!(unmet[0].full_id, "repro#base-task"); + } + + // Issue #17: a done (or waived) dependency is satisfied. + #[test] + fn test_find_unmet_dependencies_satisfied_when_done_or_waived() { + let config = LashConfig::default(); + + let done = project_with(vec![ + task("base-task", TaskStatus::Done, &[]), + task("dep-task", TaskStatus::Open, &["base-task"]), + ]); + assert!(find_unmet_dependencies(&config, &done, "repro#dep-task").is_empty()); + + let waived = project_with(vec![ + task("base-task", TaskStatus::Waived, &[]), + task("dep-task", TaskStatus::Open, &["base-task"]), + ]); + assert!(find_unmet_dependencies(&config, &waived, "repro#dep-task").is_empty()); + } + #[test] fn test_status_checkbox_char() { assert_eq!(status_checkbox_char(TaskStatus::Open), ' '); diff --git a/crates/lash-cli/src/commands/show.rs b/crates/lash-cli/src/commands/show.rs index 22f66fe..7ac7b41 100644 --- a/crates/lash-cli/src/commands/show.rs +++ b/crates/lash-cli/src/commands/show.rs @@ -323,19 +323,53 @@ fn show_task( args: &ShowArgs, theme: Option<&CliTheme>, ) -> Result { - // Get task record - let task = match task_repo.get_by_full_id(&args.target) { - Ok(Some(task)) => task, - Ok(None) => { + // Get task record by full id or bare @id (GitHub issue #14). + let task = match crate::utils::task_target::resolve_task_target(task_repo, &args.target) { + Ok(task) => task, + Err(crate::utils::task_target::TargetError::Db(e)) => { + let error = LashError::internal( + format!("Database query failed: {e}"), + Some("get_by_full_id".to_string()), + ); + if args.json { + output_json_error(&error)?; + } else { + let reporter_config = ErrorReporterConfig { + verbosity: args.verbosity, + output_format: OutputFormat::Text, + display_mode: ErrorDisplayMode::Streaming, + theme: theme.cloned(), + show_summary: false, + }; + let mut reporter = ErrorReporter::new(reporter_config); + reporter.report_error(&error); + } + return Ok(3); // Exit code 3 for DB error + } + Err(target_err) => { + // Not found or ambiguous @id. Report as a not-found diagnostic + // (exit 5) — never E_INTERNAL, which is miscategorized for a + // missing task and matters to scripts branching on error class. + let ambiguous = matches!( + target_err, + crate::utils::task_target::TargetError::Ambiguous(_) + ); // Try fuzzy matching to suggest similar task IDs let all_task_ids = task_repo.get_all_full_ids().unwrap_or_default(); - let suggestions = find_similar_task_ids(&args.target, &all_task_ids); + let suggestions = + if let crate::utils::task_target::TargetError::Ambiguous(cands) = &target_err { + cands.iter().map(|c| (c.clone(), 1.0)).collect() + } else { + find_similar_task_ids(&args.target, &all_task_ids) + }; - let error = LashError::internal( - format!("Task not found: {}", args.target), - Some("Task may not exist or hasn't been indexed".to_string()), - ); + let error = LashError::query_no_results(&args.target); let mut diag = error.to_diagnostic(); + diag.message = if ambiguous { + format!("Task @id '{}' is ambiguous", args.target) + } else { + format!("Task not found: {}", args.target) + }; // Build help message with suggestions if available let help_msg = if let Some((best_match, _score)) = suggestions.first() { @@ -380,26 +414,6 @@ fn show_task( } return Ok(5); // Exit code 5 for not found } - Err(e) => { - let error = LashError::internal( - format!("Database query failed: {e}"), - Some("get_by_full_id".to_string()), - ); - if args.json { - output_json_error(&error)?; - } else { - let reporter_config = ErrorReporterConfig { - verbosity: args.verbosity, - output_format: OutputFormat::Text, - display_mode: ErrorDisplayMode::Streaming, - theme: theme.cloned(), - show_summary: false, - }; - let mut reporter = ErrorReporter::new(reporter_config); - reporter.report_error(&error); - } - return Ok(3); // Exit code 3 for DB error - } }; // Get file information from the database diff --git a/crates/lash-cli/src/commands/start.rs b/crates/lash-cli/src/commands/start.rs index 296aade..9dff507 100644 --- a/crates/lash-cli/src/commands/start.rs +++ b/crates/lash-cli/src/commands/start.rs @@ -19,6 +19,7 @@ use std::fs; use std::path::{Path, PathBuf}; use crate::utils::file_discovery::find_project_root; +use crate::utils::task_target::TargetError; /// Arguments for the start command #[derive(Debug, Clone)] @@ -204,10 +205,10 @@ fn process_task( project_root: &Path, dry_run: bool, ) -> std::result::Result { - // Try to find the task - let task = match task_repo.get_by_full_id(task_id) { - Ok(Some(task)) => task, - Ok(None) => { + // Try to find the task by full id or bare @id. + let task = match crate::utils::task_target::resolve_task_target(task_repo, task_id) { + Ok(task) => task, + Err(TargetError::NotFound) => { // Try fuzzy matching let all_task_ids = task_repo.get_all_full_ids().unwrap_or_default(); let suggestions = find_similar_task_ids(task_id, &all_task_ids); @@ -219,7 +220,18 @@ fn process_task( suggestions: suggestions.into_iter().map(|(id, _)| id).collect(), }); } - Err(e) => { + Err(TargetError::Ambiguous(candidates)) => { + return Err(StartError { + task_id: task_id.to_string(), + code: "E_AMBIGUOUS".to_string(), + message: format!( + "Task @id '{task_id}' is ambiguous; matches {} tasks", + candidates.len() + ), + suggestions: candidates, + }); + } + Err(TargetError::Db(e)) => { return Err(StartError { task_id: task_id.to_string(), code: "E_DB_ERROR".to_string(), diff --git a/crates/lash-cli/src/main.rs b/crates/lash-cli/src/main.rs index cdcdece..23d9458 100644 --- a/crates/lash-cli/src/main.rs +++ b/crates/lash-cli/src/main.rs @@ -594,11 +594,13 @@ fn run(cli: LashCli) -> Result<()> { task_ids, dry_run, cascade, + force, } => { let args = commands::complete::CompleteArgs { task_ids, dry_run, cascade, + force, json: cli.json, no_color: cli.no_color, project_root, diff --git a/crates/lash-cli/src/utils/mod.rs b/crates/lash-cli/src/utils/mod.rs index 31d5bc3..349d731 100644 --- a/crates/lash-cli/src/utils/mod.rs +++ b/crates/lash-cli/src/utils/mod.rs @@ -2,3 +2,4 @@ pub mod file_discovery; pub mod output; +pub mod task_target; diff --git a/crates/lash-cli/src/utils/task_target.rs b/crates/lash-cli/src/utils/task_target.rs new file mode 100644 index 0000000..5bef66a --- /dev/null +++ b/crates/lash-cli/src/utils/task_target.rs @@ -0,0 +1,129 @@ +//! Resolve a user-supplied task handle to a task record. +//! +//! Commands that take a "Task ID" (`show`, `start`, `complete`) historically +//! only accepted the `file-id#task-id` full id (the truncated heading slug), +//! never the `@id` the tool displays most prominently. This helper accepts +//! either, so the natural loop — read an `@id` from `lash status`, run +//! `lash show ` — works (GitHub issue #14). +//! +//! Resolution order for a target string: +//! 1. Exact `full_id` (`file-id#task-id`) match. +//! 2. Exact `@id` (`local_id`) match — unique across the project. +//! 3. Otherwise not found (ambiguous `@id`s are reported as candidates). + +use lash_db::repository::tasks::TaskRecord; +use lash_db::TaskRepository; + +/// Outcome of resolving a task target that did not uniquely identify a task. +#[derive(Debug)] +pub enum TargetError { + /// No task matched by full id or `@id`. + NotFound, + /// The `@id` matched tasks in more than one file; the caller must + /// disambiguate using one of the listed full ids. + Ambiguous(Vec), + /// The database query failed. + Db(String), +} + +/// Resolve `target` to a single [`TaskRecord`]. +/// +/// # Errors +/// +/// Returns [`TargetError`] when the target does not resolve to exactly one task. +pub fn resolve_task_target( + task_repo: &TaskRepository, + target: &str, +) -> Result { + // 1. Full id (file-id#task-id). + match task_repo.get_by_full_id(target) { + Ok(Some(task)) => return Ok(task), + Ok(None) => {} + Err(e) => return Err(TargetError::Db(e.to_string())), + } + + // 2. Bare @id (local_id). + match task_repo.get_by_local_id(target) { + Ok(mut matches) if matches.len() == 1 => Ok(matches.remove(0)), + Ok(matches) if matches.is_empty() => Err(TargetError::NotFound), + Ok(matches) => Err(TargetError::Ambiguous( + matches.into_iter().map(|t| t.full_id).collect(), + )), + Err(e) => Err(TargetError::Db(e.to_string())), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use lash_db::{init_database, FileRepository}; + use lash_types::{FileMetadata, Task, TaskFile, TaskMetadata, TaskStatus, TaskTree}; + use std::path::PathBuf; + use std::time::SystemTime; + use tempfile::NamedTempFile; + + fn test_file(id: &str) -> TaskFile { + TaskFile { + path: PathBuf::from(format!("{id}.md")), + title: "T".to_string(), + id: id.to_string(), + metadata: FileMetadata::default(), + description: None, + description_agent_notes: Vec::new(), + tasks: TaskTree::new(), + hash: "h".to_string(), + mtime: SystemTime::now(), + } + } + + fn test_task(id: &str, order: usize) -> Task { + Task { + id: id.to_string(), + has_explicit_id: true, + title: id.to_string(), + status: TaskStatus::Open, + depth: 0, + parent_id: None, + order_index: order, + line_number: 0, + metadata: TaskMetadata::default(), + body: None, + contextual_notes: Vec::new(), + } + } + + #[test] + fn resolves_full_id_and_bare_id_reports_ambiguity_and_missing() { + let db = NamedTempFile::new().unwrap(); + let conn = init_database(db.path()).unwrap(); + let files = FileRepository::new(&conn); + let tasks = TaskRepository::new(&conn); + + let fa = files.insert(&test_file("a")).unwrap(); + let fb = files.insert(&test_file("b")).unwrap(); + tasks.insert(&test_task("unique", 0), fa, "a").unwrap(); + tasks.insert(&test_task("shared", 1), fa, "a").unwrap(); + tasks.insert(&test_task("shared", 0), fb, "b").unwrap(); + + // Full id resolves directly. + assert_eq!( + resolve_task_target(&tasks, "a#unique").unwrap().full_id, + "a#unique" + ); + // Bare unique @id resolves. + assert_eq!( + resolve_task_target(&tasks, "unique").unwrap().full_id, + "a#unique" + ); + // Ambiguous bare @id lists candidates. + match resolve_task_target(&tasks, "shared") { + Err(TargetError::Ambiguous(c)) => assert_eq!(c.len(), 2), + _ => panic!("expected Ambiguous"), + } + // Missing target is NotFound. + assert!(matches!( + resolve_task_target(&tasks, "ghost"), + Err(TargetError::NotFound) + )); + } +} diff --git a/crates/lash-core/src/dependency/mod.rs b/crates/lash-core/src/dependency/mod.rs index 13c46d3..e528c18 100644 --- a/crates/lash-core/src/dependency/mod.rs +++ b/crates/lash-core/src/dependency/mod.rs @@ -32,6 +32,7 @@ pub mod blocker_analyzer; pub mod cycle_detector; pub mod graph; pub mod graph_exporter; +pub mod reference; pub mod resolver; pub mod status_computer; @@ -43,6 +44,7 @@ pub use graph::{ DependencyGraph, EdgeData, EdgeId, EdgeRef, GraphChanges, GraphError, GraphResult, NodeData, }; pub use graph_exporter::{FilterOptions, GraphExporter}; +pub use reference::{resolve_reference, RefError, RefResolution}; pub use resolver::{ DependencyResolver, ResolutionError, ResolutionErrorKind, ResolvedDependency, ResolverResult, }; diff --git a/crates/lash-core/src/dependency/reference.rs b/crates/lash-core/src/dependency/reference.rs new file mode 100644 index 0000000..f0d298f --- /dev/null +++ b/crates/lash-core/src/dependency/reference.rs @@ -0,0 +1,342 @@ +//! Canonical `@depends-on` reference resolution +//! +//! This module is the **single source of truth** for turning a `@depends-on` +//! reference string into concrete target task IDs. Historically the linter, +//! the graph resolver, and the CLI each resolved references differently, so +//! the same reference could lint clean in one surface and fail in another +//! (GitHub issues #15 and #19). Routing every surface through +//! [`resolve_reference`] keeps them in agreement. +//! +//! # Supported forms +//! +//! Given a project of parsed [`TaskFile`]s, a reference resolves in this order: +//! +//! | Form | Example | Meaning | +//! |------|---------|---------| +//! | bare `@id` | `base-task` | file with that id (file-level), else task with that `@id` | +//! | same-file task | `#task:base-task` or `#base-task` | task in the current file | +//! | file-id + task | `repro-file#task:base-task` or `repro-file#base-task` | task in another file (by id) | +//! | path + task | `core/api.md#task:setup` | task in another file (by path) | +//! | path (file-level) | `core/api.md` | every top-level task in that file | +//! +//! The `task:` prefix on a fragment is optional everywhere — both the +//! documented `file.md#task:id` form and the natural `file.md#id` form work. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use lash_types::dependency::make_full_id; +use lash_types::TaskFile; + +/// A successfully resolved reference. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RefResolution { + /// Reference points at a single task; the string is its `file-id#task-id`. + Task(String), + /// File-level reference; every top-level task's `file-id#task-id`. + File(Vec), +} + +impl RefResolution { + /// Flatten to the list of resolved target full IDs. + #[must_use] + pub fn full_ids(self) -> Vec { + match self { + Self::Task(id) => vec![id], + Self::File(ids) => ids, + } + } +} + +/// Why a reference could not be resolved. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RefError { + /// The file part (id or path) did not match any file in the project. + FileNotFound { + /// The file id or path as written in the reference. + reference: String, + }, + /// The file was found but does not contain the referenced task. + TaskNotFound { + /// Human-readable label for the file that was searched. + file_label: String, + /// The task id that was looked for. + task: String, + /// A few available task ids in that file, for a helpful message. + available: Vec, + }, +} + +/// Collect the `file-id#task-id` of every top-level (depth 0) task in a file. +fn file_level_targets(file: &TaskFile) -> Vec { + file.tasks + .tasks() + .iter() + .filter(|t| t.depth == 0) + .map(|t| make_full_id(&file.id, &t.id)) + .collect() +} + +/// First few task ids in a file, for error messages. +fn available_task_ids(file: &TaskFile) -> Vec { + file.tasks + .tasks() + .iter() + .map(|t| t.id.clone()) + .take(5) + .collect() +} + +/// Resolve a task inside a known target file, or produce a `TaskNotFound`. +fn resolve_task_in_file(file: &TaskFile, task_key: &str) -> Result { + // Accept both `task:id` and `id`. + let key = task_key.strip_prefix("task:").unwrap_or(task_key); + if file.tasks.get_task(key).is_some() { + Ok(RefResolution::Task(make_full_id(&file.id, key))) + } else { + Err(RefError::TaskNotFound { + file_label: file.id.clone(), + task: key.to_string(), + available: available_task_ids(file), + }) + } +} + +/// Does this reference's file part look like a path rather than a file id? +fn looks_like_path(s: &str) -> bool { + s.contains('/') || s.contains('\\') || s.to_lowercase().ends_with(".md") +} + +/// Resolve a single `@depends-on` reference against the project's files. +/// +/// * `target` — the reference string (already comma-split and trimmed). +/// * `source_path` — the current file's path (the map key), for same-file refs. +/// * `_source_id` — the current file's `@id` (reserved for future diagnostics). +/// * `files` — every parsed file in the project, keyed by their map path. +/// * `resolve_path` — resolves a path reference (relative to the source file) +/// to the key used in `files` (root-relative, normalized). +/// +/// Directory references (trailing `/`) are **not** handled here; callers detect +/// and skip them before calling. +/// +/// # Errors +/// +/// Returns [`RefError`] when the file or task part cannot be found. +#[allow(clippy::implicit_hasher)] +pub fn resolve_reference( + target: &str, + source_path: &Path, + _source_id: &str, + files: &HashMap, + resolve_path: impl Fn(&str) -> PathBuf, +) -> Result { + let target = target.trim(); + + // Split into a file part and an optional task fragment. + let (before, fragment) = match target.split_once('#') { + Some((b, f)) => (b, Some(f)), + None => (target, None), + }; + + if let Some(fragment) = fragment { + // --- Task reference: `#` (file part may be empty) --- + if before.is_empty() { + // Same-file reference (`#task:id` / `#id`). + let file = files + .get(source_path) + .ok_or_else(|| RefError::FileNotFound { + reference: source_path.display().to_string(), + })?; + return resolve_task_in_file(file, fragment); + } + + // Cross-file: resolve the file part by id, then by path. + if let Some(file) = files.values().find(|f| f.id == before) { + return resolve_task_in_file(file, fragment); + } + if looks_like_path(before) { + let resolved = resolve_path(before); + if let Some(file) = files.get(&resolved) { + return resolve_task_in_file(file, fragment); + } + } + return Err(RefError::FileNotFound { + reference: before.to_string(), + }); + } + + // --- Bare reference (no `#`): file-level, or a task by @id --- + + // 1. A file id → depend on the whole file. + if let Some(file) = files.values().find(|f| f.id == target) { + return Ok(RefResolution::File(file_level_targets(file))); + } + + // 2. A path to a file → depend on the whole file. + if looks_like_path(target) { + let resolved = resolve_path(target); + if let Some(file) = files.get(&resolved) { + return Ok(RefResolution::File(file_level_targets(file))); + } + return Err(RefError::FileNotFound { + reference: target.to_string(), + }); + } + + // 3. A task `@id` — prefer the same file, then a unique match project-wide. + if let Some(file) = files.get(source_path) { + if file.tasks.get_task(target).is_some() { + return Ok(RefResolution::Task(make_full_id(&file.id, target))); + } + } + let mut matches = files + .values() + .filter(|f| f.tasks.get_task(target).is_some()) + .collect::>(); + matches.sort_by(|a, b| a.id.cmp(&b.id)); + match matches.as_slice() { + [file] => Ok(RefResolution::Task(make_full_id(&file.id, target))), + [] => Err(RefError::FileNotFound { + reference: target.to_string(), + }), + // Ambiguous bare id across files: pick deterministically (first by + // file id). Ambiguity is separately lintable; resolving avoids a + // spurious broken-link error for a reference that does exist. + [first, ..] => Ok(RefResolution::Task(make_full_id(&first.id, target))), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use lash_types::file::FileMetadata; + use lash_types::task::{TaskBuilder, TaskTree}; + use std::time::SystemTime; + + fn file_with_tasks(path: &str, id: &str, task_ids: &[&str]) -> (PathBuf, TaskFile) { + let mut tree = TaskTree::new(); + for tid in task_ids { + tree.add_task( + TaskBuilder::new(format!("Task {tid}")) + .id(*tid) + .build() + .unwrap(), + ) + .unwrap(); + } + let pb = PathBuf::from(path); + let file = TaskFile { + path: pb.clone(), + title: "T".to_string(), + id: id.to_string(), + metadata: FileMetadata::default(), + description: None, + description_agent_notes: Vec::new(), + tasks: tree, + hash: "h".to_string(), + mtime: SystemTime::now(), + }; + (pb, file) + } + + fn project(files: Vec<(PathBuf, TaskFile)>) -> HashMap { + files.into_iter().collect() + } + + /// Identity path resolver (paths are already root-relative in tests). + fn id_resolver(s: &str) -> PathBuf { + PathBuf::from(s) + } + + #[test] + fn resolves_bare_task_id_same_file() { + let (p, f) = file_with_tasks("tasks.md", "repro", &["base-task", "dep"]); + let files = project(vec![(p.clone(), f)]); + let r = resolve_reference("base-task", &p, "repro", &files, id_resolver).unwrap(); + assert_eq!(r, RefResolution::Task("repro#base-task".to_string())); + } + + #[test] + fn resolves_samefile_task_form() { + let (p, f) = file_with_tasks("tasks.md", "repro", &["base-task"]); + let files = project(vec![(p.clone(), f)]); + let r = resolve_reference("#task:base-task", &p, "repro", &files, id_resolver).unwrap(); + assert_eq!(r, RefResolution::Task("repro#base-task".to_string())); + // Also without the task: prefix. + let r2 = resolve_reference("#base-task", &p, "repro", &files, id_resolver).unwrap(); + assert_eq!(r2, RefResolution::Task("repro#base-task".to_string())); + } + + #[test] + fn resolves_file_id_task_forms() { + let (p, f) = file_with_tasks("tasks.md", "repro", &["base-task"]); + let files = project(vec![(p.clone(), f)]); + // Documented `file-id#task:id` form. + let r = + resolve_reference("repro#task:base-task", &p, "repro", &files, id_resolver).unwrap(); + assert_eq!(r, RefResolution::Task("repro#base-task".to_string())); + // Natural `file-id#id` form. + let r2 = resolve_reference("repro#base-task", &p, "repro", &files, id_resolver).unwrap(); + assert_eq!(r2, RefResolution::Task("repro#base-task".to_string())); + } + + #[test] + fn resolves_path_task_form() { + let (p1, f1) = file_with_tasks("core/api.md", "core.api", &["setup"]); + let (p2, f2) = file_with_tasks("feat.md", "feat", &["x"]); + let files = project(vec![(p1, f1), (p2.clone(), f2)]); + let r = + resolve_reference("core/api.md#task:setup", &p2, "feat", &files, id_resolver).unwrap(); + assert_eq!(r, RefResolution::Task("core.api#setup".to_string())); + } + + #[test] + fn resolves_cross_file_bare_task_id() { + let (p1, f1) = file_with_tasks("a.md", "a", &["only-here"]); + let (p2, f2) = file_with_tasks("b.md", "b", &["other"]); + let files = project(vec![(p1, f1), (p2.clone(), f2)]); + // `only-here` lives in file a, referenced from b. + let r = resolve_reference("only-here", &p2, "b", &files, id_resolver).unwrap(); + assert_eq!(r, RefResolution::Task("a#only-here".to_string())); + } + + #[test] + fn resolves_bare_file_id_to_file_level() { + let (p1, f1) = file_with_tasks("a.md", "a", &["t1", "t2"]); + let (p2, f2) = file_with_tasks("b.md", "b", &["s"]); + let files = project(vec![(p1, f1), (p2.clone(), f2)]); + let r = resolve_reference("a", &p2, "b", &files, id_resolver).unwrap(); + match r { + RefResolution::File(ids) => { + assert!(ids.contains(&"a#t1".to_string())); + assert!(ids.contains(&"a#t2".to_string())); + } + RefResolution::Task(_) => panic!("expected file-level"), + } + } + + #[test] + fn missing_file_errors() { + let (p, f) = file_with_tasks("tasks.md", "repro", &["base"]); + let files = project(vec![(p.clone(), f)]); + let err = resolve_reference("nope#task:x", &p, "repro", &files, id_resolver).unwrap_err(); + assert!(matches!(err, RefError::FileNotFound { .. })); + } + + #[test] + fn missing_task_errors_with_available() { + let (p, f) = file_with_tasks("tasks.md", "repro", &["base"]); + let files = project(vec![(p.clone(), f)]); + let err = + resolve_reference("repro#task:ghost", &p, "repro", &files, id_resolver).unwrap_err(); + match err { + RefError::TaskNotFound { + task, available, .. + } => { + assert_eq!(task, "ghost"); + assert!(available.contains(&"base".to_string())); + } + RefError::FileNotFound { .. } => panic!("expected TaskNotFound"), + } + } +} diff --git a/crates/lash-core/src/linter/rules/crossfile/dependency_exists.rs b/crates/lash-core/src/linter/rules/crossfile/dependency_exists.rs index b49341f..3e82c19 100644 --- a/crates/lash-core/src/linter/rules/crossfile/dependency_exists.rs +++ b/crates/lash-core/src/linter/rules/crossfile/dependency_exists.rs @@ -6,9 +6,10 @@ //! //! Error code: `E_LINK_NOT_FOUND` -use lash_types::{dependency::DependencyKind, Severity, Task, TaskFile}; +use lash_types::{dependency::DependencyKind, Severity, Task}; use std::path::Path; +use crate::dependency::reference::{resolve_reference, RefError}; use crate::linter::{LintContext, LintDiagnostic, LintRule}; /// Rule that checks dependency references exist @@ -39,14 +40,34 @@ impl DependencyExistsRule { Self } - /// Check if a file exists in the context - fn file_exists(ctx: &LintContext, file_path: &Path) -> bool { - ctx.get_file(file_path).is_some() - } + /// Best-effort lookup of the source line that authored a `@depends-on` + /// reference. `DependencyRef` doesn't carry its origin line, so we reopen + /// the source markdown and scan for a `@depends-on:` annotation whose + /// (comma-split) values include `target`. Returns 1-indexed `(line, col)` + /// when the source is readable; falls back to `(0, 0)` otherwise. + /// + /// Mirrors `BrokenDocFragmentRule::locate_doc_annotation` so that + /// `E_LINK_NOT_FOUND` reports the offending line like `W_SEM_DOC_FRAGMENT` + /// does (GitHub issue #18). + fn locate_depends_on(ctx: &LintContext, target: &str) -> (usize, usize) { + let absolute_source = ctx.config.root_path.join(&ctx.file_path); + let Ok(content) = std::fs::read_to_string(&absolute_source) else { + return (0, 0); + }; + + for (idx, line) in content.lines().enumerate() { + let trimmed = line.trim_start(); + let Some(value) = trimmed.strip_prefix("@depends-on:") else { + continue; + }; + let has_target = value.split(',').map(str::trim).any(|part| part == target); + if has_target { + let col = line.find('@').unwrap_or(0).saturating_add(1); + return (idx.saturating_add(1), col); + } + } - /// Check if a task exists in a file - fn task_exists_in_file(file: &TaskFile, task_id: &str) -> bool { - file.tasks.tasks().iter().any(|t| t.id == task_id) + (0, 0) } } @@ -73,188 +94,37 @@ impl LintRule for DependencyExistsRule { fn check_task(&self, task: &Task, ctx: &LintContext) -> Vec { let mut diagnostics = Vec::new(); - // Check all dependencies in this task's metadata + // Check all dependencies in this task's metadata. All reference forms + // are resolved through the shared resolver so that `lint`, + // `check-links`, and the CLI agree (GitHub issues #15, #19). for dep_ref in &task.metadata.depends_on { - match dep_ref.kind { - DependencyKind::ExplicitPath => { - // Check if this is a task reference with path format (file.md#task:id) - if let Some((path_part, task_part)) = dep_ref.target.split_once("#task:") { - // This is a task reference - let target_path = ctx.resolve_path(Path::new(path_part)); - - if !Self::file_exists(ctx, &target_path) { - diagnostics.push( - LintDiagnostic::error( - self.code(), - format!( - "File '{path_part}' not found (resolved to: {})", - target_path.display() - ), - ctx.file_path.clone(), - 0, - 0, - ) - .with_help(format!( - "Check that the file exists. Expected file at: {}", - target_path.display() - )), - ); - } else if let Some(target_file) = ctx.get_file(&target_path) { - if !Self::task_exists_in_file(target_file, task_part) { - diagnostics.push( - LintDiagnostic::error( - self.code(), - format!( - "Task '{task_part}' not found in file '{}'", - target_path.display() - ), - ctx.file_path.clone(), - 0, - 0, - ) - .with_help(format!( - "Check that the task ID exists in {}. Available tasks: {}", - target_path.display(), - target_file - .tasks - .tasks() - .iter() - .map(|t| t.id.as_str()) - .take(5) - .collect::>() - .join(", ") - )), - ); - } - } - } else { - // This is just a file reference - let target_path = ctx.resolve_path(Path::new(&dep_ref.target)); - - if !Self::file_exists(ctx, &target_path) { - diagnostics.push(LintDiagnostic::error( - self.code(), - format!( - "Dependency reference to file '{}' not found (resolved to: {})", - dep_ref.target, - target_path.display() - ), - ctx.file_path.clone(), - 0, - 0, - ).with_help( - format!( - "Check that the file exists, or fix the path. Expected file at: {}", - target_path.display() - ) - )); - } - } - } - DependencyKind::ExplicitId => { - // For ID references, check if it contains a task reference - if let Some((file_id, task_id)) = dep_ref.target.split_once('#') { - // Find file by ID or path - // First try to find by file ID (match file's id field) - let target_file = ctx.all_files.values().find(|f| f.id == file_id); - - if let Some(target_file) = target_file { - // File found, check if task exists - if !Self::task_exists_in_file(target_file, task_id) { - diagnostics.push(LintDiagnostic::error( - self.code(), - format!( - "Task '{task_id}' not found in file with ID '{file_id}'" - ), - ctx.file_path.clone(), - 0, - 0, - ).with_help( - format!( - "Check that the task ID exists in file '{}'. Available tasks: {}", - target_file.path.display(), - target_file.tasks.tasks().iter() - .map(|t| t.id.as_str()) - .take(5) - .collect::>() - .join(", ") - ) - )); - } - } else { - // Try as a path reference - let as_path = Path::new(file_id); - if let Some(target_file) = ctx.get_file(as_path) { - if !Self::task_exists_in_file(target_file, task_id) { - diagnostics.push(LintDiagnostic::error( - self.code(), - format!( - "Task '{task_id}' not found in file '{}'", - as_path.display() - ), - ctx.file_path.clone(), - 0, - 0, - ).with_help( - format!( - "Check that the task ID exists in {}. Available tasks: {}", - as_path.display(), - target_file.tasks.tasks().iter() - .map(|t| t.id.as_str()) - .take(5) - .collect::>() - .join(", ") - ) - )); - } - } else { - // Neither file ID nor path found - diagnostics.push( - LintDiagnostic::error( - self.code(), - format!("File with ID '{file_id}' not found in project"), - ctx.file_path.clone(), - 0, - 0, - ) - .with_help( - "Check the file ID or path in the dependency reference", - ), - ); - } - } - } else { - // Bare file ID reference (no task specified) - let target_file = ctx.all_files.values().find(|f| f.id == dep_ref.target); - - if target_file.is_none() { - // Try as a path - let as_path = Path::new(&dep_ref.target); - if !Self::file_exists(ctx, as_path) { - diagnostics.push( - LintDiagnostic::error( - self.code(), - format!( - "File with ID or path '{}' not found in project", - dep_ref.target - ), - ctx.file_path.clone(), - 0, - 0, - ) - .with_help( - "Check the file ID or path in the dependency reference", - ), - ); - } - } - } - } - DependencyKind::Hierarchy | DependencyKind::Directory => { - // Hierarchy dependencies are implicit and always valid - // Directory dependencies are not currently validated for existence - // (validated during parsing / could be added in future if needed) - } + // Hierarchy deps are implicit; directory deps aren't validated here. + if matches!( + dep_ref.kind, + DependencyKind::Hierarchy | DependencyKind::Directory + ) { + continue; + } + + let resolve_path = |rel: &str| ctx.resolve_path(Path::new(rel)); + let result = resolve_reference( + &dep_ref.target, + &ctx.file_path, + "", + ctx.all_files, + resolve_path, + ); + + if let Err(err) = result { + let (line, column) = Self::locate_depends_on(ctx, &dep_ref.target); + diagnostics.push(Self::diagnostic_for_error( + self.code(), + ctx, + &dep_ref.target, + &err, + line, + column, + )); } } @@ -262,13 +132,53 @@ impl LintRule for DependencyExistsRule { } } +impl DependencyExistsRule { + /// Build an `E_LINK_NOT_FOUND` diagnostic from a resolution failure. + fn diagnostic_for_error( + code: &'static str, + ctx: &LintContext, + target: &str, + err: &RefError, + line: usize, + column: usize, + ) -> LintDiagnostic { + match err { + RefError::FileNotFound { reference } => LintDiagnostic::error( + code, + format!("Dependency reference '{reference}' not found in project"), + ctx.file_path.clone(), + line, + column, + ) + .with_help(format!( + "'{target}' does not match any file id, file path, or task @id in the project" + )), + RefError::TaskNotFound { + file_label, + task, + available, + } => LintDiagnostic::error( + code, + format!("Task '{task}' not found in file '{file_label}'"), + ctx.file_path.clone(), + line, + column, + ) + .with_help(format!( + "Available tasks in '{file_label}': {}", + available.join(", ") + )), + } + } +} + #[cfg(test)] mod tests { use super::*; use lash_types::{ dependency::{parse_dependency_ref, DependencyKind}, task::{Task, TaskMetadata, TaskTree}, - FileMetadata, LashConfig, TaskStatus, + FileMetadata, LashConfig, TaskFile, TaskStatus, }; use std::collections::HashMap; use std::path::PathBuf; @@ -662,6 +572,119 @@ mod tests { assert_eq!(diagnostics.len(), 0); } + /// Build a task carrying an arbitrary dependency kind + target. + fn task_with_dep(target: &str, kind: DependencyKind) -> Task { + let metadata = TaskMetadata { + depends_on: vec![lash_types::dependency::DependencyRef::new( + target.to_string(), + kind, + )], + ..Default::default() + }; + Task { + id: "dependent".to_string(), + has_explicit_id: true, + title: "Dependent".to_string(), + status: TaskStatus::Open, + depth: 0, + parent_id: None, + order_index: 1, + line_number: 0, + metadata, + body: None, + contextual_notes: Vec::new(), + } + } + + // Issue #15: the documented same-file form `#task:` must resolve. + #[test] + fn test_samefile_task_form_resolves() { + let rule = DependencyExistsRule::new(); + let config = LashConfig::default(); + + let mut files = HashMap::new(); + files.insert( + PathBuf::from("current.md"), + make_test_file("current.md", "repro", &["base-task"]), + ); + + let ctx = make_context(&config, PathBuf::from("current.md"), &files); + let task = task_with_dep("#task:base-task", DependencyKind::ExplicitId); + assert_eq!(rule.check_task(&task, &ctx).len(), 0); + } + + // Issue #15: the documented cross-file form `file-id#task:` must resolve. + #[test] + fn test_file_id_task_prefix_form_resolves() { + let rule = DependencyExistsRule::new(); + let config = LashConfig::default(); + + let mut files = HashMap::new(); + files.insert( + PathBuf::from("other.md"), + make_test_file("other.md", "repro-file", &["base-task"]), + ); + + let ctx = make_context(&config, PathBuf::from("current.md"), &files); + let task = task_with_dep("repro-file#task:base-task", DependencyKind::ExplicitId); + assert_eq!(rule.check_task(&task, &ctx).len(), 0); + } + + // Issue #15: a bare `@id` naming a task (not a file) must resolve. + #[test] + fn test_bare_task_id_resolves() { + let rule = DependencyExistsRule::new(); + let config = LashConfig::default(); + + let mut files = HashMap::new(); + files.insert( + PathBuf::from("current.md"), + make_test_file("current.md", "repro", &["base-task"]), + ); + + let ctx = make_context(&config, PathBuf::from("current.md"), &files); + let task = task_with_dep("base-task", DependencyKind::ExplicitId); + assert_eq!(rule.check_task(&task, &ctx).len(), 0); + } + + // Issue #18: a broken reference reports the @depends-on: line, not :0:0. + #[test] + fn test_broken_reference_reports_annotation_line() { + use tempfile::TempDir; + + let temp = TempDir::new().unwrap(); + let root = temp.path(); + let source = "# Repro\n\n@id: repro\n\n## Tasks\n\n\ + - [ ] Dependent\n \ + @id: dep\n \ + @depends-on: does-not-exist\n"; + std::fs::write(root.join("tasks.md"), source).unwrap(); + + let config = LashConfig { + root_path: root.to_path_buf(), + index_file: "index.md".to_string(), + max_depth: 3, + indent_spaces: 2, + db_path: PathBuf::from(".lash/test.db"), + custom_annotation_keys: vec![], + }; + + let rule = DependencyExistsRule::new(); + let files = HashMap::new(); + let ctx = make_context(&config, PathBuf::from("tasks.md"), &files); + let task = task_with_dep("does-not-exist", DependencyKind::ExplicitId); + + let diags = rule.check_task(&task, &ctx); + assert_eq!(diags.len(), 1); + assert_eq!( + diags[0].location.line, + Some(9), + "should point at the @depends-on: line, got {:?}", + diags[0].location.line + ); + assert!(diags[0].location.column.unwrap_or(0) > 0); + } + #[test] fn test_bare_file_id_reference() { let rule = DependencyExistsRule::new(); diff --git a/crates/lash-core/src/parser/annotations.rs b/crates/lash-core/src/parser/annotations.rs index 42819b2..362a515 100644 --- a/crates/lash-core/src/parser/annotations.rs +++ b/crates/lash-core/src/parser/annotations.rs @@ -166,7 +166,14 @@ impl AnnotationBlock { pub fn get_dependencies(&self) -> Result> { let mut deps = Vec::new(); for value in self.get_list(known_keys::DEPENDS_ON) { - deps.push(parse_dependency_ref(value)?); + // A single `@depends-on` line may list several comma-separated + // references (e.g. `@depends-on: a, b`). Split them so each edge + // is resolved independently rather than as one bogus reference + // (GitHub issue #16). Directory references legitimately contain no + // commas, so this split never breaks a valid single reference. + for part in split_dependency_values(value) { + deps.push(parse_dependency_ref(&part)?); + } } Ok(deps) } @@ -513,6 +520,20 @@ pub fn parse_annotation_block<'a>( Ok(block) } +/// Split a `@depends-on` annotation value into individual references. +/// +/// Values may be comma-separated (`a, b, c`). Each element is trimmed and +/// empty elements are dropped, so trailing commas and extra whitespace are +/// tolerated. A value with no comma yields a single element. +fn split_dependency_values(value: &str) -> Vec { + value + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(String::from) + .collect() +} + /// Validate annotation key against known and custom keys /// /// # Arguments @@ -790,6 +811,38 @@ mod tests { assert_eq!(deps.len(), 2); } + #[test] + fn test_annotation_block_get_dependencies_comma_split() { + // Issue #16: a comma-separated list on a single @depends-on line must + // become multiple references, not one bogus "a, b" reference. + let mut block = AnnotationBlock::new(); + block.add("depends-on", "base-task, other-task"); + + let deps = block.get_dependencies().unwrap(); + assert_eq!(deps.len(), 2); + assert_eq!(deps[0].target, "base-task"); + assert_eq!(deps[1].target, "other-task"); + } + + #[test] + fn test_annotation_block_get_dependencies_comma_and_repeated_mix() { + let mut block = AnnotationBlock::new(); + block.add("depends-on", "a, b"); + block.add("depends-on", "c"); + + let deps = block.get_dependencies().unwrap(); + let targets: Vec<&str> = deps.iter().map(|d| d.target.as_str()).collect(); + assert_eq!(targets, vec!["a", "b", "c"]); + } + + #[test] + fn test_split_dependency_values_trims_and_drops_empty() { + assert_eq!(split_dependency_values("a, b"), vec!["a", "b"]); + assert_eq!(split_dependency_values(" solo "), vec!["solo"]); + assert_eq!(split_dependency_values("a, ,b,"), vec!["a", "b"]); + assert!(split_dependency_values("").is_empty()); + } + #[test] fn test_annotation_block_validate_single_values_ok() { let mut block = AnnotationBlock::new(); diff --git a/crates/lash-db/src/repository/tasks.rs b/crates/lash-db/src/repository/tasks.rs index 96be467..042d735 100644 --- a/crates/lash-db/src/repository/tasks.rs +++ b/crates/lash-db/src/repository/tasks.rs @@ -243,6 +243,30 @@ impl<'conn> TaskRepository<'conn> { .map_err(Into::into) } + /// Get all tasks whose `local_id` (the task's `@id`, or its synthesized + /// slug when no `@id` was given) matches `local_id`. + /// + /// A `local_id` is unique within a file but may repeat across files, so + /// this returns every match. Callers resolving a user-supplied handle use + /// this to accept the `@id` shown in `lash status`/`lash list` output as a + /// command target (GitHub issue #14). + /// + /// # Errors + /// + /// Returns error if query fails or metadata deserialization fails + pub fn get_by_local_id(&self, local_id: &str) -> DbResult> { + let mut stmt = self.conn.prepare( + "SELECT id, file_id, local_id, full_id, title, status, depth, parent_id, order_index, owner, estimate, body, metadata, contextual_notes + FROM tasks WHERE local_id = ?1 ORDER BY full_id", + )?; + + let tasks = stmt + .query_map([local_id], Self::row_to_task_record)? + .collect::, _>>()?; + + Ok(tasks) + } + /// Get a task by its database ID /// /// # Errors @@ -879,6 +903,43 @@ mod tests { assert!(task_id > 0); } + #[test] + fn test_get_by_local_id_unique_and_across_files() { + let temp_db = NamedTempFile::new().unwrap(); + let conn = init_database(temp_db.path()).unwrap(); + let file_repo = FileRepository::new(&conn); + let task_repo = TaskRepository::new(&conn); + + // Two files, each with a task whose local_id is "shared", plus a + // "unique" task only in file A. + let file_a = file_repo.insert(&create_test_file("a.md", "a")).unwrap(); + let file_b = file_repo.insert(&create_test_file("b.md", "b")).unwrap(); + task_repo + .insert(&create_test_task("unique", "U", 0, None, 0), file_a, "a") + .unwrap(); + task_repo + .insert(&create_test_task("shared", "SA", 0, None, 1), file_a, "a") + .unwrap(); + task_repo + .insert(&create_test_task("shared", "SB", 0, None, 0), file_b, "b") + .unwrap(); + + // Unique local_id → exactly one record with the expected full_id. + let unique = task_repo.get_by_local_id("unique").unwrap(); + assert_eq!(unique.len(), 1); + assert_eq!(unique[0].full_id, "a#unique"); + + // Shared local_id → one record per file. + let shared = task_repo.get_by_local_id("shared").unwrap(); + assert_eq!(shared.len(), 2); + let full_ids: Vec<&str> = shared.iter().map(|t| t.full_id.as_str()).collect(); + assert!(full_ids.contains(&"a#shared")); + assert!(full_ids.contains(&"b#shared")); + + // Missing local_id → empty. + assert!(task_repo.get_by_local_id("ghost").unwrap().is_empty()); + } + #[test] fn test_get_by_full_id() { let temp_db = NamedTempFile::new().unwrap(); diff --git a/devlog.md b/devlog.md index b0b92a5..a835d10 100644 --- a/devlog.md +++ b/devlog.md @@ -1885,3 +1885,29 @@ All workspace tests pass (691 total at time of implementation). All benchmarks complete successfully with performance exceeding targets. --- + +## Dependency & ID resolution fixes (GitHub issues #14–#19) + +Fixed a cluster of `@depends-on` / `@id` resolution bugs. Root cause: three +divergent resolution paths (the linter rule, an unused graph resolver, and +DB full-id lookup) plus a resolver that only understood the undocumented +`file-id#fragment-slug` form. Explicit `@depends-on` edges were also never +inserted into the index, so `check-links` (which queried the DB) never saw +them. + +- New shared resolver `lash-core::dependency::reference::resolve_reference` + understands bare `@id`, `#task:id`/`#id`, `file-id#task:id`/`file-id#id`, + `file.md#task:id`, and file-level forms. The linter rule, check-links, and + the complete-gate all route through it, so the surfaces agree. +- #16: `@depends-on: a, b` splits into two references at parse time. +- #15: linter resolves all documented + natural forms (commit 68fe573). +- #18: `E_LINK_NOT_FOUND` now points at the `@depends-on:` line, not `:0:0`. +- #19: `check-links` reparses and validates `@depends-on` like `lint` + (commit b445d74). +- #14: `show`/`start`/`complete` accept a task's bare `@id` (new + `TaskRepository::get_by_local_id`); `show` reports a missing task as a + not-found diagnostic (exit 5) instead of `E_INTERNAL` (commit f0f3e3d). +- #17: `lash complete` refuses while a resolvable dependency is still open + (`E_DEP_UNMET`), with `--force` to override (commit 6d78042). +- Skill docs (`references/dependencies.md`) updated to document the natural + forms and the completion gate. diff --git a/tasks/tasks.dependency-resolution-fixes.md b/tasks/tasks.dependency-resolution-fixes.md new file mode 100644 index 0000000..8180733 --- /dev/null +++ b/tasks/tasks.dependency-resolution-fixes.md @@ -0,0 +1,20 @@ +# Dependency & ID Resolution Fixes (GitHub issues #14–#19) + +Fixes for the cluster of `@depends-on` / `@id` resolution bugs reported in +GitHub issues #14–#19. Root cause: three divergent resolution paths (linter +rule, unused graph resolver, DB full-id lookup) and a resolver that only +understood the undocumented `file-id#fragment-slug` form. + +## Tasks + +- [x] Shared reference resolver (`lash-core/src/dependency/reference.rs`) + - [x] Canonical resolution: bare `@id`, `#task:id`, `#id`, `file-id#task:id`, + `file-id#id`, `file.md`, `file.md#task:id`, bare file-id (file-level) + - [x] Unit tests for every form +- [x] #16 Comma-separated `@depends-on` split into multiple refs +- [x] #15 Linter rule resolves all documented + natural forms via shared resolver +- [x] #18 `E_LINK_NOT_FOUND` reports the `@depends-on:` line, not `:0:0` +- [x] #19 `check-links` validates `@depends-on` via the same shared resolver +- [x] #14 `show`/`start`/`complete` resolve bare `@id` targets; fix `show` E_INTERNAL→not-found +- [x] #17 `complete` refuses when a resolvable dependency is unmet (with `--force`) +- [x] Verify all six issue repros; skill docs (`dependencies.md`) updated