From 68fe573ee5aceb8d3a7a93dfe89eb16befde4f3e Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 14 Jul 2026 07:53:16 -0600 Subject: [PATCH 1/5] Fix @depends-on reference resolution in linter (#15, #16, #18) Add a shared reference resolver (dependency::reference) that understands bare @id, #task:id, file-id#task:id, path#task:id, and file-level forms, and route the linter's DependencyExistsRule through it so all documented and natural reference forms resolve. Split comma-separated @depends-on lists into multiple refs, and report E_LINK_NOT_FOUND at the offending @depends-on: line instead of :0:0. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/lash-core/src/dependency/mod.rs | 2 + crates/lash-core/src/dependency/reference.rs | 342 +++++++++++++++ .../rules/crossfile/dependency_exists.rs | 403 +++++++++--------- crates/lash-core/src/parser/annotations.rs | 55 ++- tasks/tasks.dependency-resolution-fixes.md | 20 + 5 files changed, 631 insertions(+), 191 deletions(-) create mode 100644 crates/lash-core/src/dependency/reference.rs create mode 100644 tasks/tasks.dependency-resolution-fixes.md 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/tasks/tasks.dependency-resolution-fixes.md b/tasks/tasks.dependency-resolution-fixes.md new file mode 100644 index 0000000..8fec819 --- /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 + +- [ ] Shared reference resolver (`lash-core/src/dependency/reference.rs`) + - [ ] 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) + - [ ] Unit tests for every form +- [ ] #16 Comma-separated `@depends-on` split into multiple refs +- [ ] #15 Linter rule resolves all documented + natural forms via shared resolver +- [ ] #18 `E_LINK_NOT_FOUND` reports the `@depends-on:` line, not `:0:0` +- [ ] #19 `check-links` validates `@depends-on` via the same linter rule +- [ ] #14 `show`/`start`/`complete` resolve bare `@id` targets; fix `show` E_INTERNAL→E_NOT_FOUND +- [ ] #17 `complete` refuses when a resolvable dependency is unmet (with `--force`) +- [ ] Verify all six issue repros; update devlog From b445d74b1e2390a5a865099dee912824399f2605 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 14 Jul 2026 07:56:29 -0600 Subject: [PATCH 2/5] Make check-links validate @depends-on like lint (#19) Explicit @depends-on edges are not stored as NULL-target rows, so the DB query behind check-links never saw them and reported clean while lint flagged the same references. Reparse the tree and resolve each reference through the shared resolver, merging broken ones into the report. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../lash-cli/src/commands/check_links/core.rs | 137 ++++++++++++++++++ .../lash-cli/src/commands/check_links/mod.rs | 18 +++ 2 files changed, 155 insertions(+) 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 From f0f3e3de11b0a85bad9b11ff21ba2ddf2a74bf07 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 14 Jul 2026 08:04:22 -0600 Subject: [PATCH 3/5] Resolve bare @id as a command target (#14) show/start/complete now accept a task's @id (its local_id), not just the file-id#slug full id, via a shared task_target resolver backed by a new TaskRepository::get_by_local_id. Ambiguous @ids list candidate full ids. show now reports a missing task as a not-found diagnostic (exit 5) instead of the miscategorized E_INTERNAL. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/lash-cli/src/commands/complete.rs | 23 +++- crates/lash-cli/src/commands/show.rs | 72 ++++++++----- crates/lash-cli/src/commands/start.rs | 22 +++- crates/lash-cli/src/utils/mod.rs | 1 + crates/lash-cli/src/utils/task_target.rs | 129 +++++++++++++++++++++++ crates/lash-db/src/repository/tasks.rs | 61 +++++++++++ 6 files changed, 269 insertions(+), 39 deletions(-) create mode 100644 crates/lash-cli/src/utils/task_target.rs diff --git a/crates/lash-cli/src/commands/complete.rs b/crates/lash-cli/src/commands/complete.rs index 3d588c1..ac34e5d 100644 --- a/crates/lash-cli/src/commands/complete.rs +++ b/crates/lash-cli/src/commands/complete.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 complete command #[derive(Debug, Clone)] @@ -223,6 +224,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, @@ -231,10 +233,10 @@ fn process_task( dry_run: bool, cascade: 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); @@ -246,7 +248,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(), 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/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-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(); From 6d780429ba6cd4d7af65f4bc1deb3bcb2fb7fa91 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 14 Jul 2026 08:11:57 -0600 Subject: [PATCH 4/5] Enforce @depends-on on complete with --force override (#17) lash complete now refuses when a task has a resolvable @depends-on target that isn't done or waived, listing the unmet dependencies (E_DEP_UNMET). Pass --force to override. Dependencies are resolved from the current Markdown via the shared resolver, since explicit edges aren't indexed. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/lash-cli/src/cli.rs | 18 ++ crates/lash-cli/src/commands/complete.rs | 220 ++++++++++++++++++++++- crates/lash-cli/src/main.rs | 2 + 3 files changed, 237 insertions(+), 3 deletions(-) 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/complete.rs b/crates/lash-cli/src/commands/complete.rs index ac34e5d..3535924 100644 --- a/crates/lash-cli/src/commands/complete.rs +++ b/crates/lash-cli/src/commands/complete.rs @@ -7,18 +7,22 @@ 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 @@ -35,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 @@ -82,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 @@ -175,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(); @@ -187,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), @@ -232,6 +342,7 @@ fn process_task( project_root: &Path, dry_run: bool, cascade: bool, + project: Option<&(LashConfig, HashMap)>, ) -> std::result::Result { // 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) { @@ -292,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, @@ -779,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/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, From 63222db9bde393b37adb27475671887540808475 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 14 Jul 2026 08:21:53 -0600 Subject: [PATCH 5/5] Document dependency-resolution fixes and update skill docs Update references/dependencies.md to document the natural @depends-on forms (bare @id, file-id#task:id, comma lists) and the completion gate. Record the work in devlog.md and the tasks index. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/lash-agent/src/content.rs | 22 ++++++++++++------ devlog.md | 26 ++++++++++++++++++++++ tasks/tasks.dependency-resolution-fixes.md | 20 ++++++++--------- 3 files changed, 51 insertions(+), 17 deletions(-) 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/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 index 8fec819..8180733 100644 --- a/tasks/tasks.dependency-resolution-fixes.md +++ b/tasks/tasks.dependency-resolution-fixes.md @@ -7,14 +7,14 @@ understood the undocumented `file-id#fragment-slug` form. ## Tasks -- [ ] Shared reference resolver (`lash-core/src/dependency/reference.rs`) - - [ ] Canonical resolution: bare `@id`, `#task:id`, `#id`, `file-id#task:id`, +- [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) - - [ ] Unit tests for every form -- [ ] #16 Comma-separated `@depends-on` split into multiple refs -- [ ] #15 Linter rule resolves all documented + natural forms via shared resolver -- [ ] #18 `E_LINK_NOT_FOUND` reports the `@depends-on:` line, not `:0:0` -- [ ] #19 `check-links` validates `@depends-on` via the same linter rule -- [ ] #14 `show`/`start`/`complete` resolve bare `@id` targets; fix `show` E_INTERNAL→E_NOT_FOUND -- [ ] #17 `complete` refuses when a resolvable dependency is unmet (with `--force`) -- [ ] Verify all six issue repros; update devlog + - [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