From df0a7ef6ec25b8ea87d372f77e080326e37f7d50 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 11 Aug 2026 15:49:20 -0600 Subject: [PATCH 1/3] fix(index): re-derive task IDs when the derivation rules change The index now records the ID-derivation version it was built under and re-derives every file on a mismatch, since a rules change moves every unpinned ID while leaving content hashes identical. check-index compares stored IDs against freshly derived ones, lint names the cause, and the new lash migrate-ids rewrites references left dangling. Fixes #54 --- CHANGELOG.md | 32 + crates/lash-agent/src/content.rs | 19 + crates/lash-cli/src/cli.rs | 12 + crates/lash-cli/src/commands/check_index.rs | 11 + crates/lash-cli/src/commands/index.rs | 122 +++ crates/lash-cli/src/commands/lint.rs | 49 +- crates/lash-cli/src/commands/migrate_ids.rs | 818 ++++++++++++++++++ crates/lash-cli/src/commands/mod.rs | 1 + crates/lash-cli/src/main.rs | 16 + .../tests/id_derivation_drift_test.rs | 597 +++++++++++++ ...regression_tests__agent_prompt_output.snap | 19 +- crates/lash-db/schema.sql | 36 +- crates/lash-db/src/connection.rs | 78 ++ crates/lash-db/src/indexer.rs | 344 +++++++- crates/lash-db/src/lib.rs | 9 +- crates/lash-db/src/migrations.rs | 5 +- .../src/migrations/v9_id_migrations.rs | 160 ++++ .../lash-db/src/repository/id_migrations.rs | 250 ++++++ crates/lash-db/src/repository/mod.rs | 3 + crates/lash-db/src/repository/tasks.rs | 22 + crates/lash-db/src/verifier.rs | 255 +++++- crates/lash-types/src/task.rs | 25 + devlog.md | 72 ++ docs/indexing-architecture.md | 68 +- docs/user-guide.md | 42 + 25 files changed, 3047 insertions(+), 18 deletions(-) create mode 100644 crates/lash-cli/src/commands/migrate_ids.rs create mode 100644 crates/lash-cli/tests/id_derivation_drift_test.rs create mode 100644 crates/lash-db/src/migrations/v9_id_migrations.rs create mode 100644 crates/lash-db/src/repository/id_migrations.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1534bbc..2bc3791 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,38 @@ While the major version is 0, minor version bumps may contain breaking changes. ## [Unreleased] +### Added + +- `lash migrate-ids` rewrites `@depends-on` references left dangling by a + task-ID derivation change. It reports by default and writes only with + `--write`; `--forget` discards the pending renames for a project that would + rather repair by hand. Only whole references on `@depends-on:` lines are + rewritten — prose mentioning an old ID is left alone, and so is the + unqualified `old-id` form, since a bare token can name a file as readily as + a task. + +### Fixed + +- Stale task IDs no longer survive `lash index`. A task's ID is derived from + its title and is not written to the Markdown unless pinned with `@id:`, so a + release that changes the derivation rules moves every unpinned ID while every + content hash stays identical — and incremental indexing, which keys off those + hashes, never re-derives. A file nobody had edited kept serving IDs from + rules no longer in force: `lash show` printed the stored ID, `lash lint` + derived a different one and refused to resolve it, and `lash check-index` + called the index in sync throughout. The index now records the derivation + version it was built under and re-derives every file when that does not + match, so an upgrade repairs itself on the next `lash index`. The IDs that + moved are reported, and recorded for `lash migrate-ids` — the re-derive is + the only moment both spellings exist. +- `lash check-index` compares stored task IDs against freshly derived ones + instead of only comparing content hashes, which by construction cannot see a + change in how IDs are derived from unchanged content. +- `lash lint` now says when an unresolved reference points at a task ID that a + derivation change moved, rather than at a task that is missing. Without it + the error reads as a false positive: the ID it names is exactly the one + `lash show` prints back. + ## [0.3.1] - 2026-08-11 Two fixes to the root cause the 0.3.0 sweep left standing. The parsed model diff --git a/crates/lash-agent/src/content.rs b/crates/lash-agent/src/content.rs index c433bf3..caaa6b5 100644 --- a/crates/lash-agent/src/content.rs +++ b/crates/lash-agent/src/content.rs @@ -123,6 +123,8 @@ lash format [PATH...] # Normalize formatting # Indexing lash index # Update SQLite index after changes lash check-index # Verify database consistency +lash migrate-ids # Show references left dangling by an ID rule change +lash migrate-ids --write # Rewrite those references # Dependencies & Links lash graph # Show dependency graph (ascii) @@ -196,6 +198,22 @@ lash index --force # Force full reindex lash check-index # Verify consistency ``` +### Task IDs That Moved + +A task without an explicit `@id:` gets its ID derived from its title, so a +release that changes the derivation rules moves every such ID. `lash index` +detects this, re-derives the stored IDs, and records what each one used to be. +References written against the old IDs stop resolving at that moment: + +```bash +lash migrate-ids # Show what changed and which references it affects +lash migrate-ids --write # Rewrite the references, then re-index +``` + +`lash check-index` reports stored IDs that no longer match the current rules, +and `lash lint` says when an unresolved reference is one of these rather than a +typo. Pin an ID with `@id:` to keep it stable across future changes. + ### Broken References If `@depends-on` or `@doc` references are broken: @@ -340,6 +358,7 @@ pub const TOP_LEVEL_SUBCOMMANDS: &[&str] = &[ "init", "lint", "list", + "migrate-ids", "playground", "search", "show", diff --git a/crates/lash-cli/src/cli.rs b/crates/lash-cli/src/cli.rs index ac1e54d..f802af3 100644 --- a/crates/lash-cli/src/cli.rs +++ b/crates/lash-cli/src/cli.rs @@ -203,6 +203,18 @@ pub enum Commands { diff: bool, }, + /// Rewrite references to task IDs that a derivation change moved + #[command()] + MigrateIds { + /// Rewrite the references (without this, only reports what would change) + #[arg(long)] + write: bool, + + /// Discard the pending renames without rewriting anything + #[arg(long, conflicts_with = "write")] + forget: bool, + }, + /// List tasks matching specified criteria List { /// Filter by task ID (supports fuzzy matching) diff --git a/crates/lash-cli/src/commands/check_index.rs b/crates/lash-cli/src/commands/check_index.rs index ef0a2a2..18a181e 100644 --- a/crates/lash-cli/src/commands/check_index.rs +++ b/crates/lash-cli/src/commands/check_index.rs @@ -95,6 +95,11 @@ pub fn execute(args: CheckIndexArgs) -> Result { verifier_config = verifier_config.with_paths(absolute_paths); } + // Re-deriving task IDs has to use the project's own parser settings, or + // the IDs compared against are not the ones this project would get. + let parser_config = lash_types::LashConfig::from_root(&project_root).unwrap_or_default(); + verifier_config = verifier_config.with_parser_config(parser_config); + let verifier = IndexVerifier::new(&conn, verifier_config); // Run verification @@ -159,6 +164,7 @@ fn output_json_report(report: &lash_db::VerificationReport) -> Result<()> { "hash_mismatches": report.count_by_kind(lash_db::IssueKind::HashMismatch), "orphaned_tasks": report.count_by_kind(lash_db::IssueKind::OrphanedTasks), "orphaned_dependencies": report.count_by_kind(lash_db::IssueKind::OrphanedDependencies), + "stale_task_ids": report.count_by_kind(lash_db::IssueKind::StaleTaskIds), }), }); @@ -224,6 +230,11 @@ fn output_text_report( report.count_by_kind(lash_db::IssueKind::OrphanedDependencies), theme, ); + print_issue_count_if_any( + "Stale task IDs (derived under older ID rules)", + report.count_by_kind(lash_db::IssueKind::StaleTaskIds), + theme, + ); // Detailed issue list if requested if show_diff { diff --git a/crates/lash-cli/src/commands/index.rs b/crates/lash-cli/src/commands/index.rs index f9c869b..30b3075 100644 --- a/crates/lash-cli/src/commands/index.rs +++ b/crates/lash-cli/src/commands/index.rs @@ -239,6 +239,13 @@ fn output_json_report(report: &lash_db::IndexReport, error_reporter: &ErrorRepor "files_deleted": report.files_deleted, "files_unchanged": report.files_unchanged, "has_changes": report.has_changes, + "id_derivation_rebuild": report.id_derivation_rebuild, + "id_renames": report.id_renames.iter().map(|r| json!({ + "file": r.file_path.display().to_string(), + "old_id": r.old_full_id(), + "new_id": r.new_full_id(), + "title": r.title, + })).collect::>(), "errors": { "count": summary.error_count, "files_affected": summary.files_affected.len(), @@ -253,6 +260,12 @@ fn output_json_report(report: &lash_db::IndexReport, error_reporter: &ErrorRepor Ok(()) } +/// How many renamed IDs `lash index` spells out before summarising the rest +/// +/// Past a handful the list stops being readable in a terminal, and +/// `lash migrate-ids` shows the whole thing anyway. +const MAX_LISTED_RENAMES: usize = 10; + /// Output indexing report as human-readable text fn output_text_report( report: &lash_db::IndexReport, @@ -308,6 +321,46 @@ fn output_text_report( println!(" Unchanged: {}", report.files_unchanged); } + // A re-derive that actually moved IDs is not a routine reindex: every + // reference written against one of the old IDs stopped resolving in this + // same moment. Say so here rather than letting `lash lint` be the first + // thing that mentions it, several commands later, without the context. + // + // Keyed on renames rather than on the re-derive itself, because a rule + // change need not affect any title in a given project, and warning about + // a repair that changed nothing would train people to ignore it. + if !report.id_renames.is_empty() { + println!(); + let notice = format!( + "{} task ID{} changed: this index was built under older ID rules.", + report.id_renames.len(), + if report.id_renames.len() == 1 { + "" + } else { + "s" + } + ); + if let Some(t) = theme { + println!("{}", t.style_warning(¬ice)); + } else { + println!("{notice}"); + } + for rename in report.id_renames.iter().take(MAX_LISTED_RENAMES) { + println!(" {} → {}", rename.old_full_id(), rename.new_full_id()); + } + if report.id_renames.len() > MAX_LISTED_RENAMES { + println!( + " … and {} more", + report.id_renames.len() - MAX_LISTED_RENAMES + ); + } + println!(); + println!("Stored IDs now match what lash derives today. References written"); + println!("against the old IDs will not resolve until they are updated:"); + println!(" lash migrate-ids # show what would change"); + println!(" lash migrate-ids --write # rewrite the references"); + } + // Print error summary let summary = error_reporter.summary(); if summary.error_count > 0 { @@ -433,6 +486,8 @@ mod tests { error: "Parse error".to_string(), }], has_changes: true, + id_derivation_rebuild: false, + id_renames: vec![], profile: None, }; @@ -1023,6 +1078,8 @@ mod tests { error: "Unexpected token".to_string(), }], has_changes: false, + id_derivation_rebuild: false, + id_renames: vec![], profile: None, }; @@ -1528,6 +1585,8 @@ mod tests { files_skipped: 0, errors: vec![], has_changes: false, + id_derivation_rebuild: false, + id_renames: vec![], profile: None, }; output_text_report(&report_zero, false, &reporter, None); @@ -1546,9 +1605,72 @@ mod tests { files_skipped: 0, errors: vec![], has_changes: added > 0 || updated > 0 || deleted > 0, + id_derivation_rebuild: false, + id_renames: vec![], profile: None, }; output_text_report(&r, false, &reporter, None); } } + + /// Build a report carrying `count` renamed IDs. + fn report_with_renames(count: usize) -> lash_db::IndexReport { + let id_renames = (0..count) + .map(|i| lash_db::TaskIdRename { + file_path: PathBuf::from("tasks.md"), + file_id: "tasks".to_string(), + old_local_id: format!("old-{i}"), + new_local_id: format!("new-{i}"), + title: format!("Task {i}"), + }) + .collect(); + + lash_db::IndexReport { + files_processed: 1, + files_added: 0, + files_updated: 1, + files_deleted: 0, + files_unchanged: 0, + files_skipped: 0, + errors: vec![], + has_changes: true, + id_derivation_rebuild: true, + id_renames, + profile: None, + } + } + + #[test] + fn test_json_report_carries_the_renamed_ids() { + // Machine consumers need the mapping, not just the count: it is the + // only record of what a reference used to mean. + let report = report_with_renames(2); + let reporter = ErrorReporter::new(ErrorReporterConfig { + verbosity: Verbosity::Normal, + output_format: OutputFormat::JsonPretty, + display_mode: ErrorDisplayMode::Batch, + theme: None, + show_summary: false, + }); + + assert!(output_json_report(&report, &reporter).is_ok()); + } + + #[test] + fn test_a_long_rename_list_is_truncated_not_dropped() { + // Silently printing only the first ten would read as "that was all". + let report = report_with_renames(MAX_LISTED_RENAMES + 3); + assert!(report.id_renames.len() > MAX_LISTED_RENAMES); + + // Exercised for panics and for the truncation arithmetic; the text + // itself is asserted end-to-end in the integration tests. + output_text_report(&report, false, &text_reporter(), None); + } + + #[test] + fn test_report_with_no_renames_is_the_quiet_path() { + let report = report_with_renames(0); + assert!(report.id_renames.is_empty()); + output_text_report(&report, false, &text_reporter(), None); + } } diff --git a/crates/lash-cli/src/commands/lint.rs b/crates/lash-cli/src/commands/lint.rs index 2e97976..2deec5c 100644 --- a/crates/lash-cli/src/commands/lint.rs +++ b/crates/lash-cli/src/commands/lint.rs @@ -240,11 +240,21 @@ pub fn execute(args: LintArgs) -> anyhow::Result { } // Convert LintDiagnostic to Diagnostic for output - let diagnostics: Vec = filtered_diagnostics + let mut diagnostics: Vec = filtered_diagnostics .iter() .map(lint_diagnostic_to_diagnostic) .collect(); + // An unresolved reference whose target the index still recognises is not + // an ordinary broken link; it is the signature of a task-ID derivation + // change. Saying so here saves the reader from concluding lint is wrong, + // which is the natural reading when `lash show` prints the very ID lint + // just rejected (GitHub issue #54). + let id_drift = project_root.as_deref().map_or_else( + crate::commands::migrate_ids::IdDriftNotice::default, + |root| crate::commands::migrate_ids::annotate_id_drift(&mut diagnostics, root), + ); + // Output results if args.json { // JSON output to stdout @@ -258,6 +268,12 @@ pub fn execute(args: LintArgs) -> anyhow::Result { args.verbosity, args.suggest, )?; + + // The per-diagnostic help only surfaces under `-v`. This note is not + // routine help — it is the reason the errors above are not the typos + // they look like — so it goes after the summary, once, at normal + // verbosity. + print_id_drift_notice(id_drift, theme.as_ref()); } // Determine exit code based on errors @@ -266,6 +282,37 @@ pub fn execute(args: LintArgs) -> anyhow::Result { Ok(if has_errors { 2 } else { 0 }) } +/// Print the ID-drift footer, if any references were explained by one +/// +/// Kept out of `print_summary` because it is not a count of anything — it +/// reframes errors already printed, and only appears when there is something +/// to reframe. +fn print_id_drift_notice( + notice: crate::commands::migrate_ids::IdDriftNotice, + theme: Option<&CliTheme>, +) { + let Some(advice) = notice.advice() else { + return; + }; + + // The references above were correct when they were written; the rules + // moved underneath them. Saying which of the two it is turns a list of + // apparent typos into one known problem with one known fix. + let explained = notice.pending_migration + notice.stale_index; + let headline = format!( + "{explained} of these reference(s) point at task IDs that a derivation change \ + moved, not at tasks that are missing." + ); + + println!(); + if let Some(t) = theme { + println!("{}", t.style_warning(&headline)); + } else { + println!("{headline}"); + } + println!(" {advice}"); +} + /// Output diagnostics in JSON format to stdout fn output_json_diagnostics( diagnostics: &[Diagnostic], diff --git a/crates/lash-cli/src/commands/migrate_ids.rs b/crates/lash-cli/src/commands/migrate_ids.rs new file mode 100644 index 0000000..3e51d78 --- /dev/null +++ b/crates/lash-cli/src/commands/migrate_ids.rs @@ -0,0 +1,818 @@ +//! Migrate-ids command implementation +//! +//! `lash migrate-ids` finishes the repair that `lash index` starts when the +//! task-ID derivation rules change. +//! +//! The index fixes itself: it notices it was built under older rules, +//! re-derives every file, and records what each affected task's ID used to be +//! against what it is now. What it cannot fix is the Markdown. A +//! `@depends-on` written against an old ID is just text in a file, and it +//! stops resolving the moment the stored IDs move — all of them at once, which +//! makes the rebuild look like the thing that caused the damage. +//! +//! This command reads those recorded renames and rewrites the references. +//! It reports by default and only writes when asked, because it edits files +//! the user owns. + +use anyhow::{Context, Result}; +use clap::Args; +use lash::theme::CliTheme; +use lash_db::{ + open_database, IdMigrationRepository, Indexer, IndexerConfig, TaskIdRename, TaskRepository, +}; +use lash_types::error::Diagnostic; +use lash_types::LashConfig; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use crate::utils::file_discovery::find_project_root; + +/// Arguments for the migrate-ids command +#[derive(Args, Debug, Clone)] +pub struct MigrateIdsArgs { + /// Rewrite the references (without this, only reports what would change) + #[arg(long)] + pub write: bool, + + /// Discard the pending renames without rewriting anything + #[arg(long, conflicts_with = "write")] + pub forget: bool, + + /// Output format (text, json) + #[arg(long, default_value = "text")] + pub format: String, + + /// Disable colored output + #[arg(long)] + pub no_color: bool, + + /// Project root (detected automatically if None) + #[arg(skip)] + pub project_root: Option, +} + +/// One reference that a rename applies to +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReferenceRewrite { + /// File the reference is written in, relative to the project root + pub source_path: PathBuf, + + /// 1-indexed line the reference sits on + pub line_number: usize, + + /// The reference as written + pub old_reference: String, + + /// The reference as it would be written + pub new_reference: String, +} + +/// Execute the migrate-ids command +/// +/// # Arguments +/// +/// * `args` - Migrate-ids command arguments +/// +/// # Returns +/// +/// Exit code: 0 (nothing pending, or rewrite succeeded), 1 (renames are +/// pending and nothing was written yet) +/// +/// # Errors +/// +/// Returns error if the project root cannot be determined, the index cannot be +/// opened, or a file cannot be read or written. +pub fn execute(args: &MigrateIdsArgs) -> Result { + let theme = if args.format == "json" { + None + } else { + CliTheme::load(None, !args.no_color)? + }; + + let project_root = if let Some(root) = &args.project_root { + root.clone() + } else { + let cwd = std::env::current_dir().context("Failed to get current directory")?; + find_project_root(&cwd) + }; + + let db_path = project_root.join(".lash").join("lash.db"); + if !db_path.exists() { + return report_no_pending_index(args, theme.as_ref()); + } + + let conn = open_database(&db_path).context("Failed to open the index")?; + let migrations = IdMigrationRepository::new(&conn); + let renames = migrations.list_pending()?; + + if renames.is_empty() { + return report_nothing_pending(args, theme.as_ref()); + } + + if args.forget { + migrations.clear_all()?; + return report_forgotten(args, renames.len(), theme.as_ref()); + } + + let rewrites = find_rewrites(&project_root, &renames)?; + + if args.write { + apply_rewrites(&project_root, &rewrites)?; + migrations.clear_all()?; + drop(conn); + + // The rewritten references have to be re-resolved before anything + // queries them, or `lash list --blocked` and the dependency graph keep + // answering from edges built against IDs that no longer appear + // anywhere. + reindex(&db_path, &project_root)?; + } + + output(args, &renames, &rewrites, theme.as_ref())?; + + // Pending renames are unfinished work, and an exit code is how a script + // notices. Once they are written they are done, so 0. + Ok(i32::from(!args.write)) +} + +/// Re-index the project after rewriting references +/// +/// Opens its own connection so the caller's can be dropped first: the rewrite +/// changed files on disk, and the index has to be rebuilt from what is there +/// now, not from what the calling connection last read. +fn reindex(db_path: &Path, project_root: &Path) -> Result<()> { + let conn = open_database(db_path).context("Failed to reopen the index")?; + let config = LashConfig::from_root(project_root).unwrap_or_default(); + let indexer_config = IndexerConfig::new(project_root.to_path_buf()) + .with_incremental(true) + .with_progress(false); + let mut indexer = Indexer::new(&conn, indexer_config, &config); + indexer + .index_project() + .context("Failed to re-index after rewriting references")?; + Ok(()) +} + +/// Every reference in the project that one of `renames` applies to +/// +/// Only `@depends-on:` annotation lines are considered, and only whole +/// comma-separated references on them. Prose that happens to contain an old ID +/// is left alone: it is not a reference, and rewriting it would be editing +/// someone's notes. +/// +/// # Errors +/// +/// Returns error if the project cannot be walked or a file cannot be read. +pub fn find_rewrites( + project_root: &Path, + renames: &[TaskIdRename], +) -> Result> { + let lookup = RenameLookup::new(renames); + let mut rewrites = Vec::new(); + + for absolute_path in markdown_files(project_root)? { + let Ok(content) = std::fs::read_to_string(&absolute_path) else { + continue; + }; + let relative_path = absolute_path + .strip_prefix(project_root) + .unwrap_or(&absolute_path) + .to_path_buf(); + + for (index, line) in content.lines().enumerate() { + let Some(value) = depends_on_value(line) else { + continue; + }; + + for reference in value.split(',') { + let trimmed = reference.trim(); + if trimmed.is_empty() { + continue; + } + if let Some(new_reference) = lookup.rewrite(trimmed, &relative_path) { + rewrites.push(ReferenceRewrite { + source_path: relative_path.clone(), + line_number: index + 1, + old_reference: trimmed.to_string(), + new_reference, + }); + } + } + } + } + + Ok(rewrites) +} + +/// The value of a `@depends-on:` annotation, if this line is one +fn depends_on_value(line: &str) -> Option<&str> { + line.trim_start().strip_prefix("@depends-on:") +} + +/// Every Markdown file under the project root +/// +/// Deliberately not restricted to files the indexer considers task files: a +/// file with no `## Tasks` section of its own can still carry a `@depends-on` +/// pointing at one that does. +fn markdown_files(project_root: &Path) -> Result> { + let mut files = Vec::new(); + collect_markdown(project_root, &mut files)?; + files.sort(); + Ok(files) +} + +/// Recursive half of [`markdown_files`] +fn collect_markdown(dir: &Path, out: &mut Vec) -> Result<()> { + let Ok(entries) = std::fs::read_dir(dir) else { + return Ok(()); + }; + + for entry in entries.flatten() { + let path = entry.path(); + let name = entry.file_name(); + let name = name.to_string_lossy(); + + // `.lash` holds the index, `.git` holds history, and neither contains + // task references. Other dot-directories are skipped for the same + // reason `lash index` ignores them. + if name.starts_with('.') { + continue; + } + if name == "target" || name == "node_modules" { + continue; + } + + if path.is_dir() { + collect_markdown(&path, out)?; + } else if path.extension().is_some_and(|ext| ext == "md") { + out.push(path); + } + } + + Ok(()) +} + +/// Matches a written reference against the recorded renames +struct RenameLookup<'a> { + /// Keyed by `(file spelling, old local id)`, where the file spelling is + /// whatever the reference used to name the file. + by_qualifier: HashMap<(String, String), &'a TaskIdRename>, + + /// Keyed by old local id, for same-file (`#task:id`) references + by_local_id: HashMap>, +} + +impl<'a> RenameLookup<'a> { + fn new(renames: &'a [TaskIdRename]) -> Self { + let mut by_qualifier = HashMap::new(); + let mut by_local_id: HashMap> = HashMap::new(); + + for rename in renames { + for spelling in file_spellings(rename) { + by_qualifier.insert((spelling, rename.old_local_id.clone()), rename); + } + by_local_id + .entry(rename.old_local_id.clone()) + .or_default() + .push(rename); + } + + Self { + by_qualifier, + by_local_id, + } + } + + /// What `reference`, written in `source_path`, should become — if anything + /// + /// Returns `None` for a reference no rename applies to, and for the + /// unqualified `old-id` form, which is left alone on purpose: a bare token + /// can name a file as readily as a task, and rewriting one that turned out + /// to be a file id would break a reference that currently works. + fn rewrite(&self, reference: &str, source_path: &Path) -> Option { + let (qualifier, local_part) = reference.split_once('#')?; + let old_local_id = local_part.strip_prefix("task:").unwrap_or(local_part); + let keep_task_prefix = local_part.starts_with("task:"); + + let rename = if qualifier.trim().is_empty() { + // `#task:id` — same file as the reference, by definition. + self.by_local_id + .get(old_local_id)? + .iter() + .find(|rename| rename.file_path == source_path)? + } else { + let key = (normalize_spelling(qualifier), old_local_id.to_string()); + self.by_qualifier.get(&key)? + }; + + let new_local = &rename.new_local_id; + Some(if keep_task_prefix { + format!("{qualifier}#task:{new_local}") + } else { + format!("{qualifier}#{new_local}") + }) + } +} + +/// Every way a reference could have spelled the file a rename belongs to +/// +/// `lash show` prints the file's `@id`; `@depends-on` is documented as a path. +/// Both forms are in the wild, often in the same project. +fn file_spellings(rename: &TaskIdRename) -> Vec { + let path = rename.file_path.to_string_lossy().replace('\\', "/"); + let mut spellings = vec![ + normalize_spelling(&rename.file_id), + normalize_spelling(&path), + ]; + + if let Some(stem) = path.strip_suffix(".md") { + spellings.push(normalize_spelling(stem)); + } + if let Some(name) = rename.file_path.file_name() { + spellings.push(normalize_spelling(&name.to_string_lossy())); + } + + spellings.sort(); + spellings.dedup(); + spellings +} + +/// Fold a file spelling to a comparable form +fn normalize_spelling(spelling: &str) -> String { + spelling + .trim() + .trim_start_matches("./") + .replace('\\', "/") + .to_lowercase() +} + +/// Rewrite the references in place +/// +/// Each file is read, edited and written once. Only the exact reference tokens +/// found by [`find_rewrites`] are replaced, on the lines they were found on. +fn apply_rewrites(project_root: &Path, rewrites: &[ReferenceRewrite]) -> Result<()> { + let mut by_file: HashMap<&PathBuf, Vec<&ReferenceRewrite>> = HashMap::new(); + for rewrite in rewrites { + by_file + .entry(&rewrite.source_path) + .or_default() + .push(rewrite); + } + + for (relative_path, file_rewrites) in by_file { + let absolute_path = project_root.join(relative_path); + let content = std::fs::read_to_string(&absolute_path) + .with_context(|| format!("Failed to read {}", absolute_path.display()))?; + + let ends_with_newline = content.ends_with('\n'); + let mut lines: Vec = content.lines().map(String::from).collect(); + + for rewrite in file_rewrites { + let Some(line) = lines.get_mut(rewrite.line_number - 1) else { + continue; + }; + *line = replace_reference(line, &rewrite.old_reference, &rewrite.new_reference); + } + + let mut updated = lines.join("\n"); + if ends_with_newline { + updated.push('\n'); + } + + std::fs::write(&absolute_path, updated) + .with_context(|| format!("Failed to write {}", absolute_path.display()))?; + } + + Ok(()) +} + +/// Replace one whole reference on a `@depends-on:` line +/// +/// Splits on commas and swaps the matching token rather than doing a substring +/// replace, so a reference that is a prefix of another (`a#task-1` inside +/// `a#task-10`) cannot be corrupted, and the line's spacing survives. +fn replace_reference(line: &str, old_reference: &str, new_reference: &str) -> String { + let Some((prefix, value)) = line.split_once("@depends-on:") else { + return line.to_string(); + }; + + let rewritten: Vec = value + .split(',') + .map(|part| { + if part.trim() == old_reference { + part.replace(old_reference, new_reference) + } else { + part.to_string() + } + }) + .collect(); + + format!("{prefix}@depends-on:{}", rewritten.join(",")) +} + +/// Report when there is no index to read renames from +#[allow(clippy::unnecessary_wraps)] +fn report_no_pending_index(args: &MigrateIdsArgs, theme: Option<&CliTheme>) -> Result { + if args.format == "json" { + println!( + "{}", + serde_json::json!({ "pending_renames": 0, "rewrites": [], "written": false }) + ); + } else if let Some(t) = theme { + println!( + "{} run `lash index` first.", + t.style_info("No index found;") + ); + } else { + println!("No index found; run `lash index` first."); + } + Ok(0) +} + +/// Report when nothing is pending +#[allow(clippy::unnecessary_wraps)] +fn report_nothing_pending(args: &MigrateIdsArgs, theme: Option<&CliTheme>) -> Result { + if args.format == "json" { + println!( + "{}", + serde_json::json!({ "pending_renames": 0, "rewrites": [], "written": false }) + ); + } else if let Some(t) = theme { + println!("{}", t.style_success("No task IDs are pending migration.")); + } else { + println!("No task IDs are pending migration."); + } + Ok(0) +} + +/// Report a `--forget` +#[allow(clippy::unnecessary_wraps)] +fn report_forgotten(args: &MigrateIdsArgs, count: usize, theme: Option<&CliTheme>) -> Result { + if args.format == "json" { + println!( + "{}", + serde_json::json!({ "pending_renames": count, "rewrites": [], "forgotten": true }) + ); + } else { + let message = format!("Discarded {count} pending rename(s) without rewriting anything."); + if let Some(t) = theme { + println!("{}", t.style_warning(&message)); + } else { + println!("{message}"); + } + } + Ok(0) +} + +/// Report the renames and the references they apply to +fn output( + args: &MigrateIdsArgs, + renames: &[TaskIdRename], + rewrites: &[ReferenceRewrite], + theme: Option<&CliTheme>, +) -> Result<()> { + if args.format == "json" { + let json = serde_json::json!({ + "pending_renames": renames.len(), + "renames": renames.iter().map(|r| serde_json::json!({ + "file": r.file_path.display().to_string(), + "old_id": r.old_full_id(), + "new_id": r.new_full_id(), + "title": r.title, + })).collect::>(), + "rewrites": rewrites.iter().map(|r| serde_json::json!({ + "file": r.source_path.display().to_string(), + "line": r.line_number, + "old_reference": r.old_reference, + "new_reference": r.new_reference, + })).collect::>(), + "written": args.write, + }); + println!("{}", serde_json::to_string_pretty(&json)?); + return Ok(()); + } + + let heading = if args.write { + format!("Migrated {} task ID(s):", renames.len()) + } else { + format!( + "{} task ID(s) changed when the index was re-derived:", + renames.len() + ) + }; + if let Some(t) = theme { + println!("{}", t.style_warning(&heading)); + } else { + println!("{heading}"); + } + + for rename in renames { + println!( + " {} → {} ({})", + rename.old_full_id(), + rename.new_full_id(), + rename.title + ); + } + + println!(); + if rewrites.is_empty() { + println!("No `@depends-on` reference uses an old ID, so nothing needs rewriting."); + if !args.write { + println!("Run `lash migrate-ids --write` to clear the pending list."); + } + } else { + let verb = if args.write { + "Rewrote" + } else { + "Would rewrite" + }; + println!("{verb} {} reference(s):", rewrites.len()); + for rewrite in rewrites { + println!( + " {}:{} {} → {}", + rewrite.source_path.display(), + rewrite.line_number, + rewrite.old_reference, + rewrite.new_reference + ); + } + if !args.write { + println!(); + println!("Nothing has been written. Run `lash migrate-ids --write` to apply."); + } + } + + // A reference lash cannot see is one it cannot fix, and staying quiet + // about that would leave the author believing the migration was complete. + if !args.write { + println!(); + println!("Unqualified references (a bare `old-id` with no `file#`) are not rewritten:"); + println!("a bare token can name a file as readily as a task. Check those by hand"); + println!("with `lash lint` after migrating."); + } + + Ok(()) +} + +/// What an ID-drift annotation pass found +/// +/// Returned so the caller can say something once, at normal verbosity, rather +/// than relying on per-diagnostic help that only appears under `-v`. This +/// particular note is the difference between "lint is wrong" and "here is what +/// happened", so it has to be visible by default. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct IdDriftNotice { + /// References explained by a rename already recorded and awaiting a rewrite + pub pending_migration: usize, + + /// References explained by an index that has not been re-derived yet + pub stale_index: usize, +} + +impl IdDriftNotice { + /// The one-line advice that follows from what was found + #[must_use] + pub fn advice(self) -> Option<&'static str> { + if self.stale_index > 0 { + // Re-deriving comes first: it is what produces the rename records + // that `migrate-ids` then works from. + Some("Run `lash index` to re-derive them, then `lash migrate-ids --write`.") + } else if self.pending_migration > 0 { + Some("Run `lash migrate-ids --write` to update these references.") + } else { + None + } + } +} + +/// Explain unresolved references that are the signature of an ID drift +/// +/// An `E_LINK_NOT_FOUND` whose target the index still recognises is not an +/// ordinary broken link — it is a reference that was correct when it was +/// written and stopped being correct because the derivation rules moved +/// underneath it. Without saying so, the error reads as a false positive: the +/// ID it names is exactly the one `lash show` prints back. +/// +/// Appends to the `help` of any matching diagnostic and reports what it found. +/// Silently does nothing when there is no index to consult, since lint works +/// fine without one. +pub fn annotate_id_drift(diagnostics: &mut [Diagnostic], project_root: &Path) -> IdDriftNotice { + let unresolved: Vec = diagnostics + .iter() + .filter(|d| d.code == "E_LINK_NOT_FOUND") + .filter_map(|d| quoted_task_id(&d.message)) + .collect(); + if unresolved.is_empty() { + return IdDriftNotice::default(); + } + + let db_path = project_root.join(".lash").join("lash.db"); + let Ok(conn) = open_database(&db_path) else { + return IdDriftNotice::default(); + }; + + let pending: HashMap = IdMigrationRepository::new(&conn) + .list_pending() + .unwrap_or_default() + .into_iter() + .map(|r| (r.old_local_id.clone(), r.new_full_id())) + .collect(); + + let stored: std::collections::HashSet = TaskRepository::new(&conn) + .get_all_local_ids() + .unwrap_or_default() + .into_iter() + .collect(); + + let mut notice = IdDriftNotice::default(); + + for diagnostic in diagnostics + .iter_mut() + .filter(|d| d.code == "E_LINK_NOT_FOUND") + { + let Some(task_id) = quoted_task_id(&diagnostic.message) else { + continue; + }; + + let note = if let Some(new_full_id) = pending.get(&task_id) { + notice.pending_migration += 1; + format!( + "'{task_id}' was renamed to '{new_full_id}' when the task-ID derivation \ + rules changed. Run `lash migrate-ids --write` to update references." + ) + } else if stored.contains(&task_id) { + notice.stale_index += 1; + format!( + "The index still stores '{task_id}', so `lash show` prints it while this \ + check rejects it — the index was built under older ID rules. Run \ + `lash index` to re-derive, then `lash migrate-ids`." + ) + } else { + continue; + }; + + diagnostic.help = Some(match diagnostic.help.take() { + Some(existing) => format!("{existing}\n{note}"), + None => note, + }); + } + + notice +} + +/// The task ID an `E_LINK_NOT_FOUND` message names +/// +/// The message reads `Task 'some-id' not found in file 'some-file'`, so the +/// first quoted run is the ID. +fn quoted_task_id(message: &str) -> Option { + let (_, rest) = message.split_once('\'')?; + let (id, _) = rest.split_once('\'')?; + Some(id.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn rename(file: &str, file_id: &str, old: &str, new: &str) -> TaskIdRename { + TaskIdRename { + file_path: PathBuf::from(file), + file_id: file_id.to_string(), + old_local_id: old.to_string(), + new_local_id: new.to_string(), + title: "A task".to_string(), + } + } + + #[test] + fn test_rewrites_a_reference_qualified_with_the_file_id() { + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!( + lookup.rewrite("tasks#old-id", Path::new("other.md")), + Some("tasks#new-id".to_string()) + ); + } + + #[test] + fn test_rewrites_a_reference_qualified_with_the_path() { + // The documented `@depends-on` spelling. + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!( + lookup.rewrite("tasks.md#task:old-id", Path::new("other.md")), + Some("tasks.md#task:new-id".to_string()) + ); + } + + #[test] + fn test_keeps_the_task_prefix_it_found() { + // Rewriting `#task:id` as `#id` would be a second, gratuitous change + // to a line the author wrote. + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!( + lookup.rewrite("tasks#task:old-id", Path::new("other.md")), + Some("tasks#task:new-id".to_string()) + ); + assert_eq!( + lookup.rewrite("tasks#old-id", Path::new("other.md")), + Some("tasks#new-id".to_string()) + ); + } + + #[test] + fn test_same_file_reference_matches_only_within_that_file() { + // `#task:id` means "in this file", so the same text in another file + // refers to a different task and must not be touched. + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!( + lookup.rewrite("#task:old-id", Path::new("tasks.md")), + Some("#task:new-id".to_string()) + ); + assert_eq!(lookup.rewrite("#task:old-id", Path::new("other.md")), None); + } + + #[test] + fn test_leaves_a_bare_id_alone() { + // A bare token can name a file, and rewriting one that did would break + // a reference that currently resolves. + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!(lookup.rewrite("old-id", Path::new("tasks.md")), None); + } + + #[test] + fn test_leaves_an_unrelated_reference_alone() { + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let lookup = RenameLookup::new(&renames); + + assert_eq!(lookup.rewrite("tasks#other-id", Path::new("a.md")), None); + assert_eq!(lookup.rewrite("elsewhere#old-id", Path::new("a.md")), None); + } + + #[test] + fn test_nested_paths_match_by_id_and_by_path() { + let renames = vec![rename( + "area/backend.md", + "area.backend", + "old-id", + "new-id", + )]; + let lookup = RenameLookup::new(&renames); + + for spelling in [ + "area.backend#old-id", + "area/backend.md#task:old-id", + "area/backend#old-id", + ] { + assert!( + lookup.rewrite(spelling, Path::new("a.md")).is_some(), + "expected '{spelling}' to match" + ); + } + } + + #[test] + fn test_replace_reference_leaves_the_rest_of_the_line_intact() { + let line = " @depends-on: a#one, tasks#old-id, b#two"; + assert_eq!( + replace_reference(line, "tasks#old-id", "tasks#new-id"), + " @depends-on: a#one, tasks#new-id, b#two" + ); + } + + #[test] + fn test_replace_reference_does_not_corrupt_a_prefix_match() { + // A substring replace would turn `tasks#old-id-2` into + // `tasks#new-id-2`, silently repointing a task nobody renamed. + let line = "@depends-on: tasks#old-id-2"; + assert_eq!( + replace_reference(line, "tasks#old-id", "tasks#new-id"), + "@depends-on: tasks#old-id-2" + ); + } + + #[test] + fn test_replace_reference_ignores_a_line_without_the_annotation() { + let line = "- [ ] A task mentioning tasks#old-id in prose"; + assert_eq!( + replace_reference(line, "tasks#old-id", "tasks#new-id"), + line + ); + } + + #[test] + fn test_depends_on_value_requires_the_annotation() { + assert_eq!(depends_on_value(" @depends-on: a, b"), Some(" a, b")); + assert_eq!(depends_on_value("- [ ] not an annotation"), None); + assert_eq!(depends_on_value("@doc: something.md"), None); + } +} diff --git a/crates/lash-cli/src/commands/mod.rs b/crates/lash-cli/src/commands/mod.rs index b93d376..2a51c13 100644 --- a/crates/lash-cli/src/commands/mod.rs +++ b/crates/lash-cli/src/commands/mod.rs @@ -15,6 +15,7 @@ pub mod index; pub mod init; pub mod lint; pub mod list; +pub mod migrate_ids; pub mod playground; pub mod search; pub mod show; diff --git a/crates/lash-cli/src/main.rs b/crates/lash-cli/src/main.rs index 174d2e3..f2b1d58 100644 --- a/crates/lash-cli/src/main.rs +++ b/crates/lash-cli/src/main.rs @@ -296,6 +296,22 @@ fn run(cli: LashCli) -> Result<()> { process::exit(exit_code); } + Commands::MigrateIds { write, forget } => { + let args = commands::migrate_ids::MigrateIdsArgs { + write, + forget, + format: if cli.json { + "json".to_string() + } else { + "text".to_string() + }, + no_color: cli.no_color, + project_root, + }; + let exit_code = commands::migrate_ids::execute(&args)?; + process::exit(exit_code); + } + Commands::List { filter, label, diff --git a/crates/lash-cli/tests/id_derivation_drift_test.rs b/crates/lash-cli/tests/id_derivation_drift_test.rs new file mode 100644 index 0000000..7c6719e --- /dev/null +++ b/crates/lash-cli/tests/id_derivation_drift_test.rs @@ -0,0 +1,597 @@ +//! Integration tests for task-ID derivation drift (GitHub issue #54) +//! +//! When the rules that derive a task ID from its title change, every unpinned +//! ID moves. Nothing in the Markdown records the old value, and incremental +//! indexing keys off content hashes — so a file nobody has touched keeps +//! serving IDs derived under rules that are no longer in force, and +//! `check-index` calls that in sync. +//! +//! These tests stand in for that state the only way it can honestly be +//! reached: by writing a stored ID that the current rules would not derive, +//! and clearing the version stamp that says which rules built the index. + +mod common; + +use common::{run_lash_command, TestProject}; +use predicates::prelude::*; +use rusqlite::Connection; +use std::fs; +use std::path::Path; + +/// A project whose index file depends on a task by its derived ID. +/// +/// The title slugs to `founder-add-releases-mirror-token-secret` under the +/// current rules and to `founder-add-releasesmirrortoken-secret-t` under the +/// pre-0.3.0 ones — underscores used to vanish rather than become separators. +fn project_with_a_reference() -> TestProject { + TestProject::builder() + .with_file( + "lash.index.md", + r"# Test Project + +@id: index +@created: 2024-01-15 + +## Tasks + +- [ ] Founder: add RELEASES_MIRROR_TOKEN secret +- [ ] Merge the mirror PR + @depends-on: index#founder-add-releasesmirrortoken-secret-t +", + ) + .build() +} + +const LEGACY_ID: &str = "founder-add-releasesmirrortoken-secret-t"; +const CURRENT_ID: &str = "founder-add-releases-mirror-token-secret"; + +fn index(project: &TestProject) { + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success(); +} + +fn db(project: &TestProject) -> Connection { + Connection::open(project.path().join(".lash/lash.db")).expect("index should be readable") +} + +/// Rewrite a stored ID to the value the old rules produced, and forget which +/// rules built the index — the state an upgrade leaves behind. +fn simulate_pre_upgrade_index(project: &TestProject) { + let conn = db(project); + conn.execute( + "UPDATE tasks SET local_id = ?1, full_id = 'index#' || ?1 WHERE local_id = ?2", + [LEGACY_ID, CURRENT_ID], + ) + .unwrap(); + conn.execute( + "DELETE FROM metadata WHERE key = 'id_derivation_version'", + [], + ) + .unwrap(); +} + +fn stored_ids(project: &TestProject) -> Vec { + let conn = db(project); + let mut stmt = conn.prepare("SELECT local_id FROM tasks").unwrap(); + let ids = stmt + .query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::, _>>() + .unwrap(); + ids +} + +fn index_file(project: &TestProject) -> String { + fs::read_to_string(project.file_path("lash.index.md")).unwrap() +} + +// --------------------------------------------------------------------- +// The index repairs itself +// --------------------------------------------------------------------- + +#[test] +fn test_incremental_index_re_derives_ids_when_the_rules_changed() { + // The file has not changed, so hash comparison finds nothing to do — + // which is exactly why keying re-derivation on hashes alone leaves the + // stale ID in place indefinitely. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + assert!(stored_ids(&project).contains(&LEGACY_ID.to_string())); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success(); + + let ids = stored_ids(&project); + assert!( + ids.contains(&CURRENT_ID.to_string()), + "expected the current ID to be stored, got: {ids:?}" + ); + assert!( + !ids.contains(&LEGACY_ID.to_string()), + "the stale ID should be gone, got: {ids:?}" + ); +} + +#[test] +fn test_index_reports_the_ids_it_moved() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success() + .stdout(predicate::str::contains("1 task ID changed")) + .stdout(predicate::str::contains(LEGACY_ID)) + .stdout(predicate::str::contains(CURRENT_ID)) + .stdout(predicate::str::contains("lash migrate-ids")); +} + +#[test] +fn test_an_ordinary_index_says_nothing_about_derivation() { + // Warning when nothing moved would train people to ignore the warning. + let project = project_with_a_reference(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success() + .stdout(predicate::str::contains("task ID changed").not()) + .stdout(predicate::str::contains("older ID rules").not()); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success() + .stdout(predicate::str::contains("older ID rules").not()); +} + +#[test] +fn test_the_repair_does_not_repeat_itself() { + // The version stamp is what stops the next run from re-deriving + // everything again, and re-recording the same rename. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + index(&project); + index(&project); + + let conn = db(&project); + let pending: i64 = conn + .query_row("SELECT COUNT(*) FROM id_migrations", [], |row| row.get(0)) + .unwrap(); + assert_eq!(pending, 1, "the same rename must not accumulate"); +} + +// --------------------------------------------------------------------- +// check-index reports the drift instead of passing +// --------------------------------------------------------------------- + +#[test] +fn test_check_index_reports_stale_ids() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("check-index") + .assert() + .failure() + .stdout(predicate::str::contains("Stale task IDs")) + .stdout(predicate::str::contains("Index is in sync").not()); +} + +#[test] +fn test_check_index_names_the_stale_id_with_diff() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("check-index") + .arg("--diff") + .assert() + .failure() + .stdout(predicate::str::contains(LEGACY_ID)) + .stdout(predicate::str::contains("lash migrate-ids")); +} + +#[test] +fn test_check_index_still_passes_on_a_healthy_index() { + let project = project_with_a_reference(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("check-index") + .assert() + .success() + .stdout(predicate::str::contains("Stale task IDs").not()); +} + +// --------------------------------------------------------------------- +// lint names the cause +// --------------------------------------------------------------------- + +#[test] +fn test_lint_explains_a_reference_the_index_still_recognises() { + // The reporter's confusion: `lash show` prints the ID that lint has just + // rejected, so lint reads as the thing that is wrong. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("lint") + .assert() + .code(2) + .stdout(predicate::str::contains("a derivation change moved")) + .stdout(predicate::str::contains("lash index")); +} + +#[test] +fn test_lint_explains_a_reference_with_a_rename_pending() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); // re-derives, records the rename + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("lint") + .assert() + .code(2) + .stdout(predicate::str::contains("a derivation change moved")) + .stdout(predicate::str::contains("lash migrate-ids --write")); +} + +#[test] +fn test_lint_says_nothing_extra_about_an_ordinary_broken_reference() { + // A genuine typo must not be dressed up as a derivation change. + let project = TestProject::builder() + .with_file( + "lash.index.md", + r"# Test Project + +@id: index +@created: 2024-01-15 + +## Tasks + +- [ ] Real task +- [ ] Dependent task + @depends-on: index#no-such-task-ever +", + ) + .build(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("lint") + .assert() + .code(2) + .stdout(predicate::str::contains("a derivation change moved").not()); +} + +// --------------------------------------------------------------------- +// migrate-ids rewrites the references +// --------------------------------------------------------------------- + +#[test] +fn test_migrate_ids_previews_without_writing() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + let before = index_file(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .failure() // pending work is unfinished work + .stdout(predicate::str::contains("Would rewrite 1 reference")) + .stdout(predicate::str::contains("Nothing has been written")); + + assert_eq!(index_file(&project), before, "preview must not write"); +} + +#[test] +fn test_migrate_ids_write_rewrites_the_reference() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success() + .stdout(predicate::str::contains("Rewrote 1 reference")); + + let content = index_file(&project); + assert!( + content.contains(&format!("@depends-on: index#{CURRENT_ID}")), + "expected the rewritten reference, got:\n{content}" + ); + assert!( + !content.contains(LEGACY_ID), + "the old ID should be gone, got:\n{content}" + ); +} + +#[test] +fn test_lint_passes_after_migrating() { + // The whole point: the four commands stop disagreeing. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("lint") + .assert() + .success(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("check-index") + .assert() + .success(); +} + +#[test] +fn test_migrate_ids_is_a_no_op_when_nothing_is_pending() { + let project = project_with_a_reference(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .success() + .stdout(predicate::str::contains("No task IDs are pending")); +} + +#[test] +fn test_migrate_ids_clears_the_pending_list_after_writing() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .success() + .stdout(predicate::str::contains("No task IDs are pending")); +} + +#[test] +fn test_migrate_ids_forget_discards_without_touching_files() { + // For a project that would rather fix its references by hand, or has + // already done so. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + let before = index_file(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--forget") + .assert() + .success() + .stdout(predicate::str::contains("Discarded 1 pending rename")); + + assert_eq!(index_file(&project), before, "--forget must not write"); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .success() + .stdout(predicate::str::contains("No task IDs are pending")); +} + +#[test] +fn test_migrate_ids_rewrites_a_reference_in_another_file() { + // The case that makes hand repair expensive: references to a renamed task + // live wherever anyone wrote them, not next to the task. + let project = TestProject::builder() + .with_file( + "lash.index.md", + r"# Test Project + +@id: index +@created: 2024-01-15 + +## Tasks + +- [ ] Founder: add RELEASES_MIRROR_TOKEN secret +", + ) + .with_file( + "other.md", + &format!( + r"# Other + +@id: other +@created: 2024-01-15 + +## Tasks + +- [ ] Merge the mirror PR + @depends-on: index#{LEGACY_ID} +" + ), + ) + .build(); + + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success(); + + let other = fs::read_to_string(project.file_path("other.md")).unwrap(); + assert!( + other.contains(&format!("@depends-on: index#{CURRENT_ID}")), + "expected the cross-file reference rewritten, got:\n{other}" + ); +} + +#[test] +fn test_migrate_ids_leaves_prose_mentioning_an_old_id_alone() { + // Only `@depends-on` lines are references. Rewriting a task's own text + // would be editing someone's notes. + let project = TestProject::builder() + .with_file( + "lash.index.md", + &format!( + r"# Test Project + +@id: index +@created: 2024-01-15 + +## Tasks + +- [ ] Founder: add RELEASES_MIRROR_TOKEN secret +- [ ] Merge the mirror PR + @depends-on: index#{LEGACY_ID} + + The old note here still mentions index#{LEGACY_ID} on purpose. +" + ), + ) + .build(); + + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success(); + + let content = index_file(&project); + assert!( + content.contains(&format!( + "The old note here still mentions index#{LEGACY_ID}" + )), + "prose must be untouched, got:\n{content}" + ); + assert!( + content.contains(&format!("@depends-on: index#{CURRENT_ID}")), + "the reference must still be rewritten, got:\n{content}" + ); +} + +#[test] +fn test_a_file_edited_since_the_last_index_is_not_guessed_at() { + // Matching stored rows to parsed tasks is only exact while the file is + // unchanged. Once it has been edited, the stored rows describe a + // different arrangement of lines and no rename can be claimed from them — + // the file is re-indexed on its own hash anyway. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + let path = project.file_path("lash.index.md"); + let content = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{content}- [ ] A task added later\n")).unwrap(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .assert() + .success() + .stdout(predicate::str::contains("task ID changed").not()); + + // The stored ID is still corrected — the file was re-parsed either way. + assert!(stored_ids(&project).contains(&CURRENT_ID.to_string())); +} + +#[test] +fn test_migrate_ids_without_an_index_is_not_an_error() { + let project = project_with_a_reference(); + assert!(!Path::new(&project.file_path(".lash/lash.db")).exists()); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .success() + .stdout(predicate::str::contains("No index found")); +} diff --git a/crates/lash-cli/tests/snapshots/regression_tests__agent_prompt_output.snap b/crates/lash-cli/tests/snapshots/regression_tests__agent_prompt_output.snap index 412f4e2..cff03a8 100644 --- a/crates/lash-cli/tests/snapshots/regression_tests__agent_prompt_output.snap +++ b/crates/lash-cli/tests/snapshots/regression_tests__agent_prompt_output.snap @@ -1,6 +1,5 @@ --- source: crates/lash-cli/tests/regression_tests.rs -assertion_line: 343 expression: normalized --- ┓ ┓ @@ -277,6 +276,8 @@ lash format [PATH...] # Normalize formatting # Indexing lash index # Update SQLite index after changes lash check-index # Verify database consistency +lash migrate-ids # Show references left dangling by an ID rule change +lash migrate-ids --write # Rewrite those references # Dependencies & Links lash graph # Show dependency graph (ascii) @@ -338,6 +339,22 @@ lash index --force # Force full reindex lash check-index # Verify consistency ``` +### Task IDs That Moved + +A task without an explicit `@id:` gets its ID derived from its title, so a +release that changes the derivation rules moves every such ID. `lash index` +detects this, re-derives the stored IDs, and records what each one used to be. +References written against the old IDs stop resolving at that moment: + +```bash +lash migrate-ids # Show what changed and which references it affects +lash migrate-ids --write # Rewrite the references, then re-index +``` + +`lash check-index` reports stored IDs that no longer match the current rules, +and `lash lint` says when an unresolved reference is one of these rather than a +typo. Pin an ID with `@id:` to keep it stable across future changes. + ### Broken References If `@depends-on` or `@doc` references are broken: diff --git a/crates/lash-db/schema.sql b/crates/lash-db/schema.sql index 966cd4b..d03e2cf 100644 --- a/crates/lash-db/schema.sql +++ b/crates/lash-db/schema.sql @@ -20,7 +20,7 @@ CREATE TABLE metadata ( ); -- Initialize schema version -INSERT INTO metadata (key, value) VALUES ('schema_version', '8'); +INSERT INTO metadata (key, value) VALUES ('schema_version', '9'); -- ============================================================================ -- Files table (task files from the project) @@ -387,3 +387,37 @@ CREATE TRIGGER task_labels_ad AFTER DELETE ON task_labels BEGIN JOIN files f ON f.id = t.file_id WHERE t.id = old.task_id; END; + +-- ============================================================================ +-- ID migrations table (task IDs moved by a derivation-rule change) +-- ============================================================================ + +-- A derived task ID is a function of the derivation rules, and those rules can +-- change between releases. The re-derive that follows such a change is the only +-- moment both spellings of an ID exist at once, so it is the only moment the +-- old->new mapping can be recorded exactly rather than guessed at. Rows here +-- are pending work for `lash migrate-ids`, which rewrites references and clears +-- them. + +CREATE TABLE id_migrations ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + + -- Path of the file the task lives in, relative to project root + file_path TEXT NOT NULL, + + -- The file's own id, the left half of a qualified task id + file_id TEXT NOT NULL, + + -- The id stored before the derivation rules changed + old_local_id TEXT NOT NULL, + + -- The id the current rules derive for the same task + new_local_id TEXT NOT NULL, + + -- Task title, so the record is legible without re-reading the file + title TEXT NOT NULL, + + UNIQUE(file_path, old_local_id) +); + +CREATE INDEX idx_id_migrations_old ON id_migrations(file_id, old_local_id); diff --git a/crates/lash-db/src/connection.rs b/crates/lash-db/src/connection.rs index f577d5d..47e3608 100644 --- a/crates/lash-db/src/connection.rs +++ b/crates/lash-db/src/connection.rs @@ -176,12 +176,90 @@ pub fn set_metadata(conn: &Connection, key: &str, value: &str) -> DbResult<()> { Ok(()) } +/// Metadata key holding the task-ID derivation version the index was built under +/// +/// See [`lash_types::task::ID_DERIVATION_VERSION`] for what the version means +/// and why the index has to record it. +pub const ID_DERIVATION_VERSION_KEY: &str = "id_derivation_version"; + +/// The task-ID derivation version this index was built under +/// +/// Returns `None` for an index written before the version was recorded at all +/// — which is the case this exists to catch, since those are exactly the +/// indexes that may hold IDs derived by rules no longer in force. Callers +/// treat `None` the same as a mismatch. +/// +/// # Errors +/// +/// Returns error if the metadata query fails. A value that is present but not +/// a number is reported as `None` rather than an error: an unreadable version +/// is not a version, and forcing a re-derive is the safe response. +/// +/// # Example +/// +/// ```no_run +/// # use lash_db::connection::{init_database, get_id_derivation_version}; +/// # use std::path::Path; +/// # let conn = init_database(Path::new("/tmp/lash.db")).unwrap(); +/// // A freshly created index has not been stamped until it is first indexed. +/// assert_eq!(get_id_derivation_version(&conn).unwrap(), None); +/// ``` +pub fn get_id_derivation_version(conn: &Connection) -> DbResult> { + Ok(get_metadata(conn, ID_DERIVATION_VERSION_KEY)?.and_then(|value| value.trim().parse().ok())) +} + +/// Record the task-ID derivation version this index was built under +/// +/// Only correct to call after every task file in the project has been +/// re-derived. Stamping it after a partial index would claim freshness for +/// files that still hold IDs from the old rules. +/// +/// # Errors +/// +/// Returns error if the metadata write fails +pub fn set_id_derivation_version(conn: &Connection, version: u32) -> DbResult<()> { + set_metadata(conn, ID_DERIVATION_VERSION_KEY, &version.to_string()) +} + #[cfg(test)] mod tests { use super::*; use crate::migrations::CURRENT_SCHEMA_VERSION; use tempfile::NamedTempFile; + #[test] + fn test_id_derivation_version_absent_on_a_fresh_database() { + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + // An index that was never stamped is indistinguishable from one built + // under rules that have since changed, and must be treated that way. + assert_eq!(get_id_derivation_version(&conn).unwrap(), None); + } + + #[test] + fn test_id_derivation_version_round_trip() { + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + set_id_derivation_version(&conn, 2).unwrap(); + assert_eq!(get_id_derivation_version(&conn).unwrap(), Some(2)); + + set_id_derivation_version(&conn, 3).unwrap(); + assert_eq!(get_id_derivation_version(&conn).unwrap(), Some(3)); + } + + #[test] + fn test_unparseable_id_derivation_version_reads_as_absent() { + // Garbage in the metadata table must force a re-derive, not an error + // that blocks indexing entirely. + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + set_metadata(&conn, ID_DERIVATION_VERSION_KEY, "not-a-number").unwrap(); + assert_eq!(get_id_derivation_version(&conn).unwrap(), None); + } + #[test] fn test_init_database() { let temp_file = NamedTempFile::new().unwrap(); diff --git a/crates/lash-db/src/indexer.rs b/crates/lash-db/src/indexer.rs index 6b91f22..187d93a 100644 --- a/crates/lash-db/src/indexer.rs +++ b/crates/lash-db/src/indexer.rs @@ -33,16 +33,19 @@ //! # Ok::<(), lash_db::DbError>(()) //! ``` +use crate::connection::{get_id_derivation_version, set_id_derivation_version}; use crate::dependency_updater::DependencyUpdater; use crate::diff::{compute_index_diff_scoped, IndexDiff}; use crate::error::{DbError, DbResult}; use crate::profiler::{IndexProfiler, ProfileReport}; -use crate::repository::{FileRepository, TaskRepository}; +use crate::repository::{FileRepository, IdMigrationRepository, TaskIdRename, TaskRepository}; use crate::walker::{FileMetadata, FileWalker, FileWalkerConfig}; use lash_core::parser::{is_valid_task_file, parse_file}; +use lash_types::task::ID_DERIVATION_VERSION; use lash_types::{LashConfig, TaskFile}; use rayon::prelude::*; use rusqlite::Connection; +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -318,6 +321,23 @@ pub struct IndexReport { /// Whether any changes were made to the database pub has_changes: bool, + /// Whether every file was re-derived because the ID rules had changed + /// + /// Set when the index was built under a different + /// [`ID_DERIVATION_VERSION`] than this build derives IDs with, which + /// forces a full re-parse regardless of content hashes. Worth reporting: + /// stored IDs move, and any `@depends-on` written against the old ones + /// starts failing to resolve in the same moment. + pub id_derivation_rebuild: bool, + + /// Task IDs the re-derive actually moved + /// + /// Empty on an ordinary index, and empty even on a re-derive when the rule + /// change happened not to affect any title in this project. Non-empty is + /// the signal worth acting on: every reference written against one of + /// these old IDs stopped resolving at this moment. + pub id_renames: Vec, + /// Performance profile (if profiling was enabled) pub profile: Option, } @@ -335,6 +355,8 @@ impl IndexReport { files_skipped: 0, errors: Vec::new(), has_changes: false, + id_derivation_rebuild: false, + id_renames: Vec::new(), profile: None, } } @@ -355,6 +377,8 @@ impl IndexReport { /// files_skipped: 0, /// errors: vec![], /// has_changes: true, + /// id_derivation_rebuild: false, + /// id_renames: vec![], /// profile: None, /// }; /// @@ -381,6 +405,8 @@ impl IndexReport { /// files_skipped: 0, /// errors: vec![], /// has_changes: true, + /// id_derivation_rebuild: false, + /// id_renames: vec![], /// profile: None, /// }; /// @@ -553,9 +579,18 @@ impl<'conn> Indexer<'conn> { Some(scope_paths.as_slice()) }; + // An index built under different ID-derivation rules holds IDs this + // build would not derive, and no amount of hash comparison will + // notice: the files did not change, the rules did. Re-derive + // everything in that case, so an upgrade repairs itself here rather + // than lying until someone happens to edit each file. + let derivation_is_current = + get_id_derivation_version(self.conn)? == Some(ID_DERIVATION_VERSION); + report.id_derivation_rebuild = !derivation_is_current; + let diff = { let _guard = profiler.start_phase("diff"); - if self.config.incremental { + if self.config.incremental && derivation_is_current { compute_index_diff_scoped(self.conn, &files, scope)? } else { // For full reindex, treat all files as new @@ -619,7 +654,7 @@ impl<'conn> Indexer<'conn> { } // Phase 4: Update database - let (normalized_files, path_to_id) = { + let (normalized_files, path_to_id, existing_hashes) = { let _db_guard = profiler.start_phase("database"); let file_repo = FileRepository::new(self.conn); @@ -642,15 +677,19 @@ impl<'conn> Indexer<'conn> { }) .collect(); - // Get list of existing paths BEFORE upsert for accurate reporting - let existing_paths: std::collections::HashSet<_> = normalized_files + // Record each file's stored hash BEFORE the upsert overwrites it. + // Presence answers "added or updated?" for the report; the hash + // itself is what tells a re-derive whether the file is unchanged + // on disk, which is the precondition for matching old task rows to + // new ones by line number. + let existing_hashes: HashMap = normalized_files .iter() .filter_map(|file| { file_repo .get_by_path(&file.path) .ok() .flatten() - .map(|_| file.path.clone()) + .map(|record| (file.path.clone(), record.hash)) }) .collect(); @@ -660,14 +699,14 @@ impl<'conn> Indexer<'conn> { // Count updates vs inserts based on which files existed before upsert for file in &normalized_files { - if existing_paths.contains(&file.path) { + if existing_hashes.contains_key(&file.path) { report.files_updated += 1; } else { report.files_added += 1; } } - (normalized_files, path_to_id) + (normalized_files, path_to_id, existing_hashes) }; // Now process tasks for each file (outside the phase guard to allow profiling) @@ -681,6 +720,20 @@ impl<'conn> Indexer<'conn> { .get(&task_file.path) .ok_or_else(|| DbError::Other("File ID not found after upsert".to_string()))?; + // The rows about to be deleted are the only surviving record of + // what these tasks' IDs used to be. If the rules moved them, the + // mapping has to come out now or it is gone. + if report.id_derivation_rebuild { + let unchanged_on_disk = existing_hashes + .get(&task_file.path) + .is_some_and(|stored| stored == &task_file.hash); + if unchanged_on_disk { + report + .id_renames + .extend(self.detect_id_renames(*file_db_id, &task_file)?); + } + } + // Delete existing tasks and doc refs for this file (ensures clean re-index) // Note: Deleting tasks will cascade delete task-level doc refs via foreign key self.conn @@ -777,6 +830,23 @@ impl<'conn> Indexer<'conn> { dep_repo.rebuild_closure()?; } + // Park the renames for `lash migrate-ids`. The stored IDs have just + // been corrected; the references in the Markdown still say what they + // said, and this is the last point at which anything knows what they + // used to mean. + if !report.id_renames.is_empty() { + IdMigrationRepository::new(self.conn).record_all(&report.id_renames)?; + } + + // Stamp the derivation version, but only when this run can vouch for + // the whole project. A scoped index re-derives some files and leaves + // the rest alone, and a run with parse errors leaves those files' old + // rows in place; stamping after either would claim a freshness the + // index does not have, and the next run would skip the repair. + if scope.is_none() && report.errors.is_empty() { + set_id_derivation_version(self.conn, ID_DERIVATION_VERSION)?; + } + // Final progress report if self.config.report_progress { if let Some(ref callback) = self.progress_callback { @@ -794,6 +864,94 @@ impl<'conn> Indexer<'conn> { Ok(report) } + /// Task IDs in this file that the current rules derive differently + /// + /// Called with the stored rows still in place, just before they are + /// deleted and rewritten — the one moment both spellings of an ID exist. + /// + /// The caller has already established that the file's content hash is + /// unchanged, so the stored rows and the freshly parsed tasks describe the + /// same file and stand in one-to-one correspondence. What is left is + /// naming that correspondence without using the ID, which is the thing + /// under suspicion. The key is title plus structural position (`depth`, + /// `order_index`): none of the three is derived from the ID rules, and + /// together they identify a task in a file that has not changed. + /// + /// Any key held by more than one task on either side is dropped rather + /// than guessed at. An ambiguous pairing would produce a rename that + /// `lash migrate-ids` then writes into someone's Markdown, so a missed + /// rename — which surfaces as an unresolved reference the author reads + /// and fixes — is the better failure. + fn detect_id_renames( + &self, + file_db_id: i64, + task_file: &TaskFile, + ) -> DbResult> { + /// Title plus position in the tree, neither of which the ID rules touch + type TaskKey = (String, i64, i64); + + let mut stmt = self + .conn + .prepare("SELECT local_id, title, depth, order_index FROM tasks WHERE file_id = ?1")?; + + let mut stored: HashMap> = HashMap::new(); + let rows = stmt.query_map([file_db_id], |row| { + let local_id: String = row.get(0)?; + let title: String = row.get(1)?; + let depth: i64 = row.get(2)?; + let order_index: i64 = row.get(3)?; + Ok(((title, depth, order_index), local_id)) + })?; + for row in rows { + let (key, local_id) = row?; + // `None` marks a key claimed by more than one task: ambiguous, so + // no task holding it can be matched. + stored + .entry(key) + .and_modify(|slot| *slot = None) + .or_insert(Some(local_id)); + } + + // The parsed side needs the same treatment, for the same reason. + let mut parsed_key_counts: HashMap = HashMap::new(); + for task in task_file.tasks.tasks() { + *parsed_key_counts.entry(Self::task_key(task)).or_insert(0) += 1; + } + + let mut renames = Vec::new(); + for task in task_file.tasks.tasks() { + let key = Self::task_key(task); + if parsed_key_counts.get(&key) != Some(&1) { + continue; + } + let Some(Some(old_local_id)) = stored.get(&key) else { + continue; + }; + if old_local_id == &task.id { + continue; + } + + renames.push(TaskIdRename { + file_path: task_file.path.clone(), + file_id: task_file.id.clone(), + old_local_id: old_local_id.clone(), + new_local_id: task.id.clone(), + title: task.title.clone(), + }); + } + + Ok(renames) + } + + /// The title-and-position key used to pair stored rows with parsed tasks + fn task_key(task: &lash_types::Task) -> (String, i64, i64) { + ( + task.title.clone(), + i64::from(task.depth), + i64::try_from(task.order_index).unwrap_or(i64::MAX), + ) + } + /// Parse files in parallel using rayon /// /// Returns a vector of parse results in the same order as the input files. @@ -910,6 +1068,176 @@ mod tests { temp_dir } + // ------------------------------------------------------------------ + // ID derivation drift (GitHub issue #54) + // ------------------------------------------------------------------ + + /// Index the project once, then put it in the state an upgrade leaves: + /// a stored ID the current rules would not derive, and no record of which + /// rules built the index. + fn indexed_project_with_a_stale_id(project_dir: &TempDir, conn: &Connection) { + let config = IndexerConfig::new(project_dir.path().to_path_buf()); + let parser_config = LashConfig::default(); + Indexer::new(conn, config, &parser_config) + .index_project() + .unwrap(); + + conn.execute( + "UPDATE tasks SET local_id = 'task-1-legacy', full_id = 'test1#task-1-legacy' + WHERE local_id = 'task-1'", + [], + ) + .unwrap(); + conn.execute( + "DELETE FROM metadata WHERE key = 'id_derivation_version'", + [], + ) + .unwrap(); + } + + fn stored_local_ids(conn: &Connection) -> Vec { + let mut stmt = conn.prepare("SELECT local_id FROM tasks").unwrap(); + let ids = stmt + .query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::, _>>() + .unwrap(); + ids + } + + #[test] + fn test_a_stale_derivation_version_forces_a_re_derive() { + // Neither file changed, so the hash-based diff has nothing to report. + // Only the recorded version says the stored IDs came from other rules. + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + indexed_project_with_a_stale_id(&project_dir, &conn); + + let config = IndexerConfig::new(project_dir.path().to_path_buf()).with_incremental(true); + let parser_config = LashConfig::default(); + let report = Indexer::new(&conn, config, &parser_config) + .index_project() + .unwrap(); + + assert!(report.id_derivation_rebuild); + assert_eq!(report.files_unchanged, 0, "nothing may be skipped"); + assert!(stored_local_ids(&conn).contains(&"task-1".to_string())); + assert!(!stored_local_ids(&conn).contains(&"task-1-legacy".to_string())); + } + + #[test] + fn test_the_re_derive_reports_the_exact_old_and_new_ids() { + // The mapping is the only thing that can repair a reference later, and + // this run is the last moment both halves of it exist. + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + indexed_project_with_a_stale_id(&project_dir, &conn); + + let config = IndexerConfig::new(project_dir.path().to_path_buf()); + let parser_config = LashConfig::default(); + let report = Indexer::new(&conn, config, &parser_config) + .index_project() + .unwrap(); + + assert_eq!(report.id_renames.len(), 1); + let rename = &report.id_renames[0]; + assert_eq!(rename.old_local_id, "task-1-legacy"); + assert_eq!(rename.new_local_id, "task-1"); + assert_eq!(rename.old_full_id(), "test1#task-1-legacy"); + assert_eq!(rename.new_full_id(), "test1#task-1"); + assert_eq!(rename.title, "Task 1"); + } + + #[test] + fn test_renames_are_persisted_for_migrate_ids() { + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + indexed_project_with_a_stale_id(&project_dir, &conn); + + let config = IndexerConfig::new(project_dir.path().to_path_buf()); + let parser_config = LashConfig::default(); + Indexer::new(&conn, config, &parser_config) + .index_project() + .unwrap(); + + let pending = IdMigrationRepository::new(&conn).list_pending().unwrap(); + assert_eq!(pending.len(), 1); + assert_eq!(pending[0].old_local_id, "task-1-legacy"); + } + + #[test] + fn test_the_version_stamp_stops_the_repair_repeating() { + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + indexed_project_with_a_stale_id(&project_dir, &conn); + + let parser_config = LashConfig::default(); + Indexer::new( + &conn, + IndexerConfig::new(project_dir.path().to_path_buf()), + &parser_config, + ) + .index_project() + .unwrap(); + + let second = Indexer::new( + &conn, + IndexerConfig::new(project_dir.path().to_path_buf()), + &parser_config, + ) + .index_project() + .unwrap(); + + assert!(!second.id_derivation_rebuild); + assert!(second.id_renames.is_empty()); + assert_eq!( + get_id_derivation_version(&conn).unwrap(), + Some(ID_DERIVATION_VERSION) + ); + } + + #[test] + fn test_a_scoped_index_does_not_claim_the_whole_project_is_fresh() { + // Only some files were re-derived, so stamping the version would make + // the next full run skip the repair the rest still need. + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + indexed_project_with_a_stale_id(&project_dir, &conn); + + let config = IndexerConfig::new(project_dir.path().to_path_buf()) + .with_paths(vec![project_dir.path().join("test1.md")]); + let parser_config = LashConfig::default(); + Indexer::new(&conn, config, &parser_config) + .index_project() + .unwrap(); + + assert_eq!( + get_id_derivation_version(&conn).unwrap(), + None, + "a scoped run must leave the version unstamped" + ); + } + + #[test] + fn test_an_ordinary_index_records_no_renames() { + let project_dir = create_test_project(); + let conn = init_database(&project_dir.path().join("test.db")).unwrap(); + + let config = IndexerConfig::new(project_dir.path().to_path_buf()); + let parser_config = LashConfig::default(); + let report = Indexer::new(&conn, config, &parser_config) + .index_project() + .unwrap(); + + // A fresh database has no version either, so the first index counts as + // a re-derive — but there are no stored IDs to have moved. + assert!(report.id_renames.is_empty()); + assert_eq!( + get_id_derivation_version(&conn).unwrap(), + Some(ID_DERIVATION_VERSION) + ); + } + #[test] fn test_indexer_config_new() { let config = IndexerConfig::new(PathBuf::from("/project")); diff --git a/crates/lash-db/src/lib.rs b/crates/lash-db/src/lib.rs index 155b712..db40789 100644 --- a/crates/lash-db/src/lib.rs +++ b/crates/lash-db/src/lib.rs @@ -25,7 +25,10 @@ pub mod search; pub mod verifier; pub mod walker; -pub use connection::{get_schema_version, init_database, open_database, set_schema_version}; +pub use connection::{ + get_id_derivation_version, get_schema_version, init_database, open_database, + set_id_derivation_version, set_schema_version, +}; pub use dependency_updater::DependencyUpdater; pub use diff::{compute_index_diff, compute_index_diff_parallel, IndexDiff}; pub use error::{DbError, DbResult}; @@ -38,8 +41,8 @@ pub use project_root::{ ProjectRootConfig, }; pub use repository::{ - DependencyRepository, DocRefRepository, FileRepository, LabelRepository, StatusCounts, - TaskRepository, + DependencyRepository, DocRefRepository, FileRepository, IdMigrationRepository, LabelRepository, + StatusCounts, TaskIdRename, TaskRepository, }; pub use search::{ parse_query, search, search_with_profiling, SearchMetrics, SearchQuery, SearchResult, diff --git a/crates/lash-db/src/migrations.rs b/crates/lash-db/src/migrations.rs index 9c9cfdb..bf86166 100644 --- a/crates/lash-db/src/migrations.rs +++ b/crates/lash-db/src/migrations.rs @@ -12,6 +12,7 @@ mod v5_fts_description; mod v6_contextual_notes; mod v7_fts_contextual_notes; mod v8_add_in_progress_status; +mod v9_id_migrations; use v2_enhanced_fts::MigrationV2EnhancedFts; use v3_doc_refs::MigrationV3DocRefs; @@ -20,9 +21,10 @@ use v5_fts_description::MigrationV5FtsDescription; use v6_contextual_notes::MigrationV6ContextualNotes; use v7_fts_contextual_notes::MigrationV7FtsContextualNotes; use v8_add_in_progress_status::MigrationV8AddInProgressStatus; +use v9_id_migrations::MigrationV9IdMigrations; /// Current schema version -pub const CURRENT_SCHEMA_VERSION: i32 = 8; +pub const CURRENT_SCHEMA_VERSION: i32 = 9; /// A database migration pub trait Migration { @@ -136,6 +138,7 @@ fn get_migrations() -> Vec> { Box::new(MigrationV6ContextualNotes), Box::new(MigrationV7FtsContextualNotes), Box::new(MigrationV8AddInProgressStatus), + Box::new(MigrationV9IdMigrations), ] } diff --git a/crates/lash-db/src/migrations/v9_id_migrations.rs b/crates/lash-db/src/migrations/v9_id_migrations.rs new file mode 100644 index 0000000..6f880ba --- /dev/null +++ b/crates/lash-db/src/migrations/v9_id_migrations.rs @@ -0,0 +1,160 @@ +//! Migration v9: Add `id_migrations` table for recording task IDs that moved +//! +//! A derived task ID is a function of the derivation rules, and those rules +//! can change between releases. When they do, the indexer re-derives every +//! file and the stored IDs shift underneath references written against the old +//! ones. The re-derive is the only moment both spellings exist at once, so it +//! is the only moment the old→new mapping can be recorded exactly rather than +//! guessed at. This table is where it goes, for `lash migrate-ids` to consume +//! afterwards. + +use rusqlite::Connection; + +use crate::error::DbResult; +use crate::migrations::Migration; + +/// Migration to add the `id_migrations` table +pub(super) struct MigrationV9IdMigrations; + +impl Migration for MigrationV9IdMigrations { + fn version(&self) -> i32 { + 9 + } + + fn description(&self) -> &'static str { + "Add id_migrations table recording task IDs moved by a derivation change" + } + + fn up(&self, conn: &Connection) -> DbResult<()> { + conn.execute_batch( + " + -- ============================================================================ + -- ID migrations table (task IDs moved by a derivation-rule change) + -- ============================================================================ + + CREATE TABLE IF NOT EXISTS id_migrations ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + + -- Path of the file the task lives in, relative to project root + file_path TEXT NOT NULL, + + -- The file's own id, the left half of a qualified task id + file_id TEXT NOT NULL, + + -- The id stored before the derivation rules changed + old_local_id TEXT NOT NULL, + + -- The id the current rules derive for the same task + new_local_id TEXT NOT NULL, + + -- Task title, so the record is legible without re-reading the file + title TEXT NOT NULL, + + -- One pending rename per (file, old id). Re-detecting the same + -- rename overwrites rather than accumulating duplicates. + UNIQUE(file_path, old_local_id) + ); + + -- Rewriting references means looking up by the id that appears in + -- them, which is the old one. + CREATE INDEX IF NOT EXISTS idx_id_migrations_old + ON id_migrations(file_id, old_local_id); + ", + )?; + + Ok(()) + } + + fn down(&self, conn: &Connection) -> DbResult<()> { + conn.execute_batch( + " + DROP INDEX IF EXISTS idx_id_migrations_old; + DROP TABLE IF EXISTS id_migrations; + ", + )?; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::connection::init_database; + use tempfile::NamedTempFile; + + #[test] + fn test_migration_v9_up() { + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + let migration = MigrationV9IdMigrations; + migration.up(&conn).unwrap(); + + let table_exists: i32 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='id_migrations'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(table_exists, 1); + } + + #[test] + fn test_migration_v9_is_idempotent() { + // `init_database` already creates the table from schema.sql, so the + // migration has to be a no-op on a fresh database as well as on an + // upgraded one. + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + let migration = MigrationV9IdMigrations; + migration.up(&conn).unwrap(); + migration.up(&conn).unwrap(); + } + + #[test] + fn test_migration_v9_down() { + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + + let migration = MigrationV9IdMigrations; + migration.up(&conn).unwrap(); + migration.down(&conn).unwrap(); + + let table_exists: i32 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='id_migrations'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(table_exists, 0); + } + + #[test] + fn test_one_pending_rename_per_file_and_old_id() { + // Re-running the detection must not accumulate duplicate rows, or + // `lash migrate-ids` would report the same rename several times. + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + MigrationV9IdMigrations.up(&conn).unwrap(); + + let insert = "INSERT OR REPLACE INTO id_migrations + (file_path, file_id, old_local_id, new_local_id, title) + VALUES ('tasks.md', 'tasks', 'old-id', ?1, 'A task')"; + conn.execute(insert, ["new-id"]).unwrap(); + conn.execute(insert, ["newer-id"]).unwrap(); + + let (count, new_id): (i64, String) = conn + .query_row( + "SELECT COUNT(*), MAX(new_local_id) FROM id_migrations", + [], + |row| Ok((row.get(0)?, row.get(1)?)), + ) + .unwrap(); + assert_eq!(count, 1); + assert_eq!(new_id, "newer-id"); + } +} diff --git a/crates/lash-db/src/repository/id_migrations.rs b/crates/lash-db/src/repository/id_migrations.rs new file mode 100644 index 0000000..9f23a6d --- /dev/null +++ b/crates/lash-db/src/repository/id_migrations.rs @@ -0,0 +1,250 @@ +//! Repository for task IDs moved by a derivation-rule change +//! +//! See [`crate::migrations`] v9 for why the mapping has to be captured at +//! re-derive time rather than reconstructed later. + +use rusqlite::Connection; + +use crate::error::DbResult; +use lash_types::dependency::make_full_id; +use std::path::{Path, PathBuf}; + +/// A task ID that moved because the derivation rules changed +/// +/// Both spellings refer to the same task in the same file at the same line. +/// The old one is what existing `@depends-on` references were written against; +/// the new one is what lash derives today. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TaskIdRename { + /// Path of the file the task lives in, relative to the project root + pub file_path: PathBuf, + + /// The file's own id — the left half of a qualified task id + pub file_id: String, + + /// The id stored before the derivation rules changed + pub old_local_id: String, + + /// The id the current rules derive for the same task + pub new_local_id: String, + + /// The task's title, so a rename is legible without re-reading the file + pub title: String, +} + +impl TaskIdRename { + /// The qualified id references were written against + #[must_use] + pub fn old_full_id(&self) -> String { + make_full_id(&self.file_id, &self.old_local_id) + } + + /// The qualified id that resolves today + #[must_use] + pub fn new_full_id(&self) -> String { + make_full_id(&self.file_id, &self.new_local_id) + } +} + +/// Reads and writes pending ID renames +pub struct IdMigrationRepository<'conn> { + conn: &'conn Connection, +} + +impl<'conn> IdMigrationRepository<'conn> { + /// Create a repository over an open connection + #[must_use] + pub fn new(conn: &'conn Connection) -> Self { + Self { conn } + } + + /// Record renames detected during a re-derive + /// + /// Re-detecting a rename already on file replaces it rather than adding a + /// duplicate, so running `lash index` repeatedly before migrating does not + /// inflate the pending list. + /// + /// # Errors + /// + /// Returns error if any insert fails + pub fn record_all(&self, renames: &[TaskIdRename]) -> DbResult<()> { + let mut stmt = self.conn.prepare( + "INSERT OR REPLACE INTO id_migrations + (file_path, file_id, old_local_id, new_local_id, title) + VALUES (?1, ?2, ?3, ?4, ?5)", + )?; + + for rename in renames { + stmt.execute(rusqlite::params![ + rename.file_path.to_string_lossy(), + rename.file_id, + rename.old_local_id, + rename.new_local_id, + rename.title, + ])?; + } + + Ok(()) + } + + /// Every rename still awaiting a reference rewrite + /// + /// # Errors + /// + /// Returns error if the query fails + pub fn list_pending(&self) -> DbResult> { + let mut stmt = self.conn.prepare( + "SELECT file_path, file_id, old_local_id, new_local_id, title + FROM id_migrations + ORDER BY file_path, old_local_id", + )?; + + let rows = stmt.query_map([], |row| { + let file_path: String = row.get(0)?; + Ok(TaskIdRename { + file_path: PathBuf::from(file_path), + file_id: row.get(1)?, + old_local_id: row.get(2)?, + new_local_id: row.get(3)?, + title: row.get(4)?, + }) + })?; + + rows.collect::, _>>().map_err(Into::into) + } + + /// How many renames are pending + /// + /// # Errors + /// + /// Returns error if the query fails + pub fn pending_count(&self) -> DbResult { + let count: i64 = self + .conn + .query_row("SELECT COUNT(*) FROM id_migrations", [], |row| row.get(0))?; + #[allow(clippy::cast_sign_loss)] + Ok(count as usize) + } + + /// Drop the renames for one file, once its references have been rewritten + /// + /// # Errors + /// + /// Returns error if the delete fails + pub fn clear_file(&self, file_path: &Path) -> DbResult<()> { + self.conn.execute( + "DELETE FROM id_migrations WHERE file_path = ?1", + [file_path.to_string_lossy()], + )?; + Ok(()) + } + + /// Drop every pending rename + /// + /// # Errors + /// + /// Returns error if the delete fails + pub fn clear_all(&self) -> DbResult<()> { + self.conn.execute("DELETE FROM id_migrations", [])?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::connection::init_database; + use tempfile::NamedTempFile; + + fn rename(file: &str, old: &str, new: &str) -> TaskIdRename { + TaskIdRename { + file_path: PathBuf::from(file), + file_id: file.trim_end_matches(".md").to_string(), + old_local_id: old.to_string(), + new_local_id: new.to_string(), + title: "A task".to_string(), + } + } + + fn test_conn() -> Connection { + let temp_file = NamedTempFile::new().unwrap(); + let conn = init_database(temp_file.path()).unwrap(); + // Keep the file alive for the connection's lifetime. + std::mem::forget(temp_file); + conn + } + + #[test] + fn test_record_and_list() { + let conn = test_conn(); + let repo = IdMigrationRepository::new(&conn); + + repo.record_all(&[ + rename("tasks.md", "old-b", "new-b"), + rename("tasks.md", "old-a", "new-a"), + ]) + .unwrap(); + + let pending = repo.list_pending().unwrap(); + assert_eq!(pending.len(), 2); + // Ordered, so output is stable between runs. + assert_eq!(pending[0].old_local_id, "old-a"); + assert_eq!(pending[1].old_local_id, "old-b"); + assert_eq!(repo.pending_count().unwrap(), 2); + } + + #[test] + fn test_recording_the_same_rename_twice_does_not_duplicate() { + // `lash index` may run several times before anyone gets around to + // migrating, and each run re-detects the same drift. + let conn = test_conn(); + let repo = IdMigrationRepository::new(&conn); + + repo.record_all(&[rename("tasks.md", "old-a", "new-a")]) + .unwrap(); + repo.record_all(&[rename("tasks.md", "old-a", "new-a")]) + .unwrap(); + + assert_eq!(repo.pending_count().unwrap(), 1); + } + + #[test] + fn test_clear_file_leaves_other_files_alone() { + let conn = test_conn(); + let repo = IdMigrationRepository::new(&conn); + + repo.record_all(&[ + rename("tasks.md", "old-a", "new-a"), + rename("other.md", "old-b", "new-b"), + ]) + .unwrap(); + + repo.clear_file(Path::new("tasks.md")).unwrap(); + + let pending = repo.list_pending().unwrap(); + assert_eq!(pending.len(), 1); + assert_eq!(pending[0].file_path, PathBuf::from("other.md")); + } + + #[test] + fn test_clear_all() { + let conn = test_conn(); + let repo = IdMigrationRepository::new(&conn); + + repo.record_all(&[rename("tasks.md", "old-a", "new-a")]) + .unwrap(); + repo.clear_all().unwrap(); + + assert_eq!(repo.pending_count().unwrap(), 0); + assert!(repo.list_pending().unwrap().is_empty()); + } + + #[test] + fn test_full_ids_are_qualified_with_the_file() { + // References are written qualified, so that is the form the rewrite + // has to search for and produce. + let r = rename("tasks.md", "old-a", "new-a"); + assert_eq!(r.old_full_id(), "tasks#old-a"); + assert_eq!(r.new_full_id(), "tasks#new-a"); + } +} diff --git a/crates/lash-db/src/repository/mod.rs b/crates/lash-db/src/repository/mod.rs index 416478d..79ba9ad 100644 --- a/crates/lash-db/src/repository/mod.rs +++ b/crates/lash-db/src/repository/mod.rs @@ -3,6 +3,7 @@ //! Provides high-level CRUD operations and queries for: //! - Files //! - Tasks +//! - Task ID renames pending a reference rewrite //! - Dependencies //! - Labels //! - Documentation References @@ -10,11 +11,13 @@ pub mod dependencies; pub mod doc_ref; pub mod files; +pub mod id_migrations; pub mod labels; pub mod tasks; pub use dependencies::DependencyRepository; pub use doc_ref::{DocRefRepository, DocRefRow}; pub use files::{normalize_path_for_db, FileRepository}; +pub use id_migrations::{IdMigrationRepository, TaskIdRename}; pub use labels::LabelRepository; pub use tasks::{StatusCounts, TaskRepository}; diff --git a/crates/lash-db/src/repository/tasks.rs b/crates/lash-db/src/repository/tasks.rs index 5a2dafb..a882a3a 100644 --- a/crates/lash-db/src/repository/tasks.rs +++ b/crates/lash-db/src/repository/tasks.rs @@ -357,6 +357,28 @@ impl<'conn> TaskRepository<'conn> { Ok(ids) } + /// Get all task local IDs in the database + /// + /// The unqualified half of a task's identity, which is the form that + /// appears in an unresolved-reference message. Used to tell an ordinary + /// broken reference apart from one the index still recognises — the latter + /// being the signature of an ID derivation change rather than a typo. + /// + /// # Errors + /// + /// Returns error if query fails + pub fn get_all_local_ids(&self) -> DbResult> { + let mut stmt = self + .conn + .prepare("SELECT DISTINCT local_id FROM tasks ORDER BY local_id")?; + + let ids = stmt + .query_map([], |row| row.get(0))? + .collect::, _>>()?; + + Ok(ids) + } + /// Find tasks by label /// /// Finds tasks that have the label directly (via `task_labels`) OR diff --git a/crates/lash-db/src/verifier.rs b/crates/lash-db/src/verifier.rs index 5e8ccc9..e9a6ace 100644 --- a/crates/lash-db/src/verifier.rs +++ b/crates/lash-db/src/verifier.rs @@ -38,9 +38,10 @@ use crate::error::DbResult; use crate::repository::files::FileRecord; use crate::repository::FileRepository; use crate::walker::{FileMetadata, FileWalker, FileWalkerConfig}; -use lash_core::parser::is_valid_task_file; +use lash_core::parser::{is_valid_task_file, parse_file}; +use lash_types::LashConfig; use rusqlite::Connection; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::path::{Path, PathBuf}; @@ -57,6 +58,14 @@ pub enum IssueKind { OrphanedTasks, /// Dependencies reference files that don't exist OrphanedDependencies, + /// Stored task IDs differ from what the current rules derive + /// + /// The file's content matches its stored hash, so nothing about the file + /// changed — the ID derivation rules did, in a release the index has not + /// caught up with. Hash comparison alone reports this as in sync, which is + /// how a project ends up where `lash show` prints an ID that `lash lint` + /// cannot resolve. + StaleTaskIds, } impl fmt::Display for IssueKind { @@ -67,6 +76,7 @@ impl fmt::Display for IssueKind { Self::HashMismatch => write!(f, "Hash Mismatch"), Self::OrphanedTasks => write!(f, "Orphaned Tasks"), Self::OrphanedDependencies => write!(f, "Orphaned Dependencies"), + Self::StaleTaskIds => write!(f, "Stale Task IDs"), } } } @@ -170,6 +180,33 @@ impl VerificationIssue { "Run `lash index` or use auto-fix to clean up orphaned dependencies".to_string(), ) } + + /// Create a stale task IDs issue + /// + /// `examples` are stored IDs the current rules no longer derive, listed so + /// the report is actionable without a second command — these are exactly + /// the IDs that `lash show` would still print and `lash lint` would + /// already refuse to resolve. + #[must_use] + pub fn stale_task_ids(path: &Path, stale_count: usize, examples: &[String]) -> Self { + let sample = if examples.is_empty() { + String::new() + } else { + format!(" (e.g. {})", examples.join(", ")) + }; + + Self::new( + IssueKind::StaleTaskIds, + path.to_path_buf(), + format!( + "{} stored task ID(s) in '{}' do not match what lash derives today{sample}", + stale_count, + path.display() + ), + "Run `lash index` to re-derive them, then `lash migrate-ids` to update references" + .to_string(), + ) + } } /// Result of index verification @@ -282,6 +319,18 @@ pub struct VerifierConfig { pub check_orphaned_tasks: bool, /// Whether to check for orphaned dependencies pub check_orphaned_dependencies: bool, + + /// Whether to compare stored task IDs against freshly derived ones + /// + /// Costs a parse of every file whose hash already matches — which is + /// otherwise the cheap path — but it is the only check that catches an + /// index whose IDs were derived under rules that have since changed. + /// Nothing about such a file's content differs, so hash comparison calls + /// it in sync. + pub check_task_ids: bool, + + /// Parser configuration used when re-deriving task IDs + pub parser_config: LashConfig, } impl VerifierConfig { @@ -305,9 +354,51 @@ impl VerifierConfig { walker_config, check_orphaned_tasks: true, check_orphaned_dependencies: true, + check_task_ids: true, + parser_config: LashConfig::default(), } } + /// Set the parser configuration used when re-deriving task IDs + /// + /// Defaults to [`LashConfig::default`]. Pass the project's own config so + /// the IDs compared against are the ones the project would actually get. + /// + /// # Example + /// + /// ``` + /// use lash_db::verifier::VerifierConfig; + /// use lash_types::LashConfig; + /// use std::path::PathBuf; + /// + /// let config = VerifierConfig::new(PathBuf::from("/project")) + /// .with_parser_config(LashConfig::default()); + /// assert!(config.check_task_ids); + /// ``` + #[must_use] + pub fn with_parser_config(mut self, parser_config: LashConfig) -> Self { + self.parser_config = parser_config; + self + } + + /// Set whether to compare stored task IDs against freshly derived ones + /// + /// # Example + /// + /// ``` + /// use lash_db::verifier::VerifierConfig; + /// use std::path::PathBuf; + /// + /// let config = VerifierConfig::new(PathBuf::from("/project")) + /// .with_task_id_check(false); + /// assert!(!config.check_task_ids); + /// ``` + #[must_use] + pub fn with_task_id_check(mut self, check: bool) -> Self { + self.check_task_ids = check; + self + } + /// Set custom walker configuration /// /// # Example @@ -522,9 +613,82 @@ impl<'conn> IndexVerifier<'conn> { self.check_orphaned_dependencies(&fs_map, &mut report)?; } + // Phase 7: Compare stored task IDs against freshly derived ones. + // Everything above compares content hashes, which by construction + // cannot see a change in how IDs are derived from unchanged content. + if self.config.check_task_ids { + self.check_task_ids(&fs_files, &db_map, &mut report)?; + } + Ok(report) } + /// Check that stored task IDs match what the current rules derive + /// + /// Only files whose content hash already matches are examined. A file with + /// a hash mismatch is reported by that check and will be re-parsed by the + /// next `lash index` anyway; a file whose hash matches is precisely the + /// one that gets skipped, and so the only one that can carry IDs from an + /// older derivation indefinitely. + fn check_task_ids( + &self, + fs_files: &[FileMetadata], + db_map: &HashMap, + report: &mut VerificationReport, + ) -> DbResult<()> { + /// Stale IDs named in the issue before it becomes unreadable + const MAX_EXAMPLES: usize = 3; + + for fs_file in fs_files { + let Some((db_hash, db_id)) = db_map.get(&fs_file.relative_path) else { + continue; + }; + if &fs_file.content_hash != db_hash { + continue; + } + + // A file that will not parse cannot be compared against. It is not + // silently fine, but it is a parse error rather than ID drift, and + // `lash index` reports it as one. + let Ok(parsed) = parse_file(&fs_file.absolute_path, &self.config.parser_config) else { + continue; + }; + + let derived: HashSet<&str> = + parsed.tasks.tasks().iter().map(|t| t.id.as_str()).collect(); + + let mut stmt = self + .conn + .prepare("SELECT local_id FROM tasks WHERE file_id = ?1")?; + let stored: Vec = stmt + .query_map([db_id], |row| row.get(0))? + .collect::, _>>()?; + + let stale: Vec<&String> = stored + .iter() + .filter(|id| !derived.contains(id.as_str())) + .collect(); + + if stale.is_empty() { + continue; + } + + let examples: Vec = stale + .iter() + .take(MAX_EXAMPLES) + .map(|id| (*id).clone()) + .collect(); + + report.issues.push(VerificationIssue::stale_task_ids( + &fs_file.relative_path, + stale.len(), + &examples, + )); + } + + Ok(()) + } + /// Check for orphaned tasks /// /// Orphaned tasks are tasks that exist in the database for files that @@ -708,6 +872,93 @@ mod tests { let config = VerifierConfig::new(PathBuf::from("/project")); assert!(config.check_orphaned_tasks); assert!(config.check_orphaned_dependencies); + assert!(config.check_task_ids); + } + + /// A one-task file, its index row, and the task row the index holds for it + /// + /// `stored_local_id` is written straight into the tasks table, so a test + /// can put an ID there that the current rules would not derive — which is + /// what a pre-upgrade index looks like. + fn project_with_stored_task_id(temp_dir: &TempDir, conn: &Connection, stored_local_id: &str) { + let path = temp_dir.path().join("test.md"); + fs::write(&path, "# Test\n\n@id: test\n\n## Tasks\n\n- [ ] Task one\n").unwrap(); + + let file_meta = FileMetadata::from_path(&path, temp_dir.path()).unwrap(); + let file_repo = FileRepository::new(conn); + let task_file = create_task_file("test.md", &file_meta.content_hash, file_meta.mtime); + let file_db_id = file_repo.insert(&task_file).unwrap(); + + conn.execute( + "INSERT INTO tasks (file_id, local_id, full_id, title, status, depth, order_index) + VALUES (?1, ?2, 'test#' || ?2, 'Task one', 'open', 0, 0)", + rusqlite::params![file_db_id, stored_local_id], + ) + .unwrap(); + } + + #[test] + fn test_verify_reports_a_stored_id_the_current_rules_do_not_derive() { + // The file's hash matches, so every other check calls this in sync. + // That is the whole bug: an unchanged file is exactly the one whose + // IDs never get re-derived. + let temp_dir = TempDir::new().unwrap(); + let temp_db = NamedTempFile::new().unwrap(); + let conn = init_database(temp_db.path()).unwrap(); + + project_with_stored_task_id(&temp_dir, &conn, "taskone-legacy"); + + let config = VerifierConfig::new(temp_dir.path().to_path_buf()); + let report = IndexVerifier::new(&conn, config).verify().unwrap(); + + assert_eq!(report.count_by_kind(IssueKind::HashMismatch), 0); + assert_eq!(report.count_by_kind(IssueKind::StaleTaskIds), 1); + + let issue = &report.issues_of_kind(IssueKind::StaleTaskIds)[0]; + assert!( + issue.description.contains("taskone-legacy"), + "the issue must name the stale ID: {}", + issue.description + ); + assert!(issue.fix_suggestion.contains("lash index")); + } + + #[test] + fn test_verify_passes_when_stored_ids_match_the_derivation() { + let temp_dir = TempDir::new().unwrap(); + let temp_db = NamedTempFile::new().unwrap(); + let conn = init_database(temp_db.path()).unwrap(); + + project_with_stored_task_id(&temp_dir, &conn, "task-one"); + + let config = VerifierConfig::new(temp_dir.path().to_path_buf()); + let report = IndexVerifier::new(&conn, config).verify().unwrap(); + + assert_eq!(report.count_by_kind(IssueKind::StaleTaskIds), 0); + } + + #[test] + fn test_task_id_check_can_be_turned_off() { + // It costs a parse per otherwise-cheap file, so callers that only want + // the hash comparison can say so. + let temp_dir = TempDir::new().unwrap(); + let temp_db = NamedTempFile::new().unwrap(); + let conn = init_database(temp_db.path()).unwrap(); + + project_with_stored_task_id(&temp_dir, &conn, "taskone-legacy"); + + let config = VerifierConfig::new(temp_dir.path().to_path_buf()).with_task_id_check(false); + let report = IndexVerifier::new(&conn, config).verify().unwrap(); + + assert_eq!(report.count_by_kind(IssueKind::StaleTaskIds), 0); + } + + #[test] + fn test_stale_task_ids_issue_without_examples_is_still_readable() { + let issue = VerificationIssue::stale_task_ids(Path::new("tasks.md"), 4, &[]); + assert_eq!(issue.kind, IssueKind::StaleTaskIds); + assert!(issue.description.contains("4 stored task ID")); + assert!(!issue.description.contains("e.g.")); } #[test] diff --git a/crates/lash-types/src/task.rs b/crates/lash-types/src/task.rs index 2a81af2..ce56e44 100644 --- a/crates/lash-types/src/task.rs +++ b/crates/lash-types/src/task.rs @@ -18,6 +18,31 @@ pub const NOTE_LENGTH_ERROR_THRESHOLD: usize = 500; /// Longest synthesized task ID, in characters. pub const MAX_SYNTHESIZED_ID_LENGTH: usize = 40; +/// Version of the rules [`synthesize_task_id`] applies +/// +/// A derived task ID is never written to the Markdown — only an explicit +/// `@id:` is. So the ID of every unpinned task is a function of this code, and +/// changing the code silently moves those IDs. Anything holding an ID from +/// before the change (an index record, an `@depends-on` written against it) +/// then disagrees with what lash derives today, with nothing to say why. +/// +/// The index records the version it was built under. When it does not match, +/// the indexer re-derives every file instead of trusting content hashes, +/// because the file did not change — the rules did. That makes an upgrade +/// repair itself on the next `lash index` rather than waiting for someone to +/// happen to edit each file. +/// +/// **Bump this whenever [`synthesize_task_id`] changes what it returns for any +/// input.** Version history: +/// +/// - `1` — through 0.2.x. Non-alphanumerics were dropped rather than replaced, +/// so `RELEASES_MIRROR_TOKEN` slugged to `releasesmirrortoken`, and +/// truncation could leave a trailing separator. +/// - `2` — 0.3.0 onward. Every non-alphanumeric becomes a separator, empty +/// parts collapse, inline labels are stripped, and truncation never leaves a +/// trailing `-`. +pub const ID_DERIVATION_VERSION: u32 = 2; + /// Derive a task ID from a task title /// /// This is the one place a task ID is derived from a title. Both the parser, diff --git a/devlog.md b/devlog.md index 9bffe20..a0fdffe 100644 --- a/devlog.md +++ b/devlog.md @@ -2614,3 +2614,75 @@ task with a body. The lint rule suggested on #48 is still not worth building. A misattributed body is syntactically indistinguishable from a correct one, so there is nothing for the linter to check against. + +## Stale task IDs survived `lash index` (#54, 2026-08-11) + +A derived value cached behind a hash of its input, where the derivation is the +other input and nothing watches it. + +A task's ID comes from its title and is not written to the Markdown unless the +author pins it with `@id:`. So the ID is a function of the derivation code as +much as of the file. 0.3.0 changed that code — underscores became separators +instead of vanishing — and every unpinned ID moved while every content hash +stayed byte-identical. Incremental indexing keys off those hashes, so a file +nobody had edited since the upgrade was never re-parsed and kept serving IDs +derived under rules no longer in force. + +The failure was silent in all four directions at once. `lash show` read the +stored record and printed the old ID. `lash lint` derived a new one and +rejected the reference. `lash check-index` compared hashes, found them equal, +and said in sync. `lash index` said "Unchanged: 1". The reporter assumed lint +was wrong, which is the only conclusion available from the output. + +Four changes, one per surface. + +**The index records what derived it.** `ID_DERIVATION_VERSION` names the +current rules and is stamped into the `metadata` table. On mismatch — or +absence, which is the same thing — the hash diff is ignored and every file is +re-parsed. An upgrade repairs itself on the next `lash index` instead of +waiting for someone to happen to edit each file. + +The stamp is written only after a run that can vouch for the whole project: +unscoped, no parse errors. A scoped run re-derives part of the project and a +failed parse leaves that file's old rows in place; stamping after either claims +a freshness the index does not have, and the next run skips the repair. + +**The repair captures what it moved.** Correcting the stored IDs is half of it. +A `@depends-on` written against an old ID is text in a file and stops resolving +the moment the stored IDs move — all of them together, which is why `--force` +made the rebuild look like the cause of the damage rather than the fix. The +re-derive is the only moment both spellings exist: old rows still in place, +new tasks in hand. So the mapping is taken there, into `id_migrations`. + +Matching old rows to new tasks is the part worth remembering. The obvious key +is the ID, which is the thing under suspicion. Line number would be exact, but +the tasks table does not persist one. What is left is title plus structural +position (`depth`, `order_index`) — none of which the ID rules touch — and that +is exact only because the caller has already established the file's hash is +unchanged. Any key claimed twice on either side is dropped rather than guessed: +an ambiguous pairing becomes a rename that `migrate-ids` writes into someone's +Markdown, whereas a missed rename surfaces as an unresolved reference the +author reads and fixes. + +**`check-index` re-derives instead of trusting hashes.** It now parses each +file whose hash already matches and compares the IDs. That is the expensive +path for the otherwise-cheap case, and it is the only one that catches this: an +unchanged file is precisely the file whose IDs never get re-derived. + +**`lint` names the cause.** An `E_LINK_NOT_FOUND` whose target the index still +recognises, or that matches a pending rename, is not a typo. The note goes in +the diagnostic's `help` for JSON and `-v`, and — because help is hidden at +normal verbosity, and this is the difference between "lint is wrong" and "here +is what happened" — once more after the summary where it will actually be read. + +`lash migrate-ids` consumes the recorded renames. It previews by default and +writes only when asked, since it edits files the user owns. It touches whole +references on `@depends-on:` lines and nothing else: prose mentioning an old ID +is someone's notes, and the unqualified `old-id` form is left alone because a +bare token can name a file as readily as a task — rewriting one that turned out +to be a file id would break a reference that currently works. That gap is +printed rather than left implicit. + +The deeper fix available to any project is `@id:`. A pinned ID is the only one +a future derivation change cannot move, and the docs now say so in the three +places someone would be reading when they care. diff --git a/docs/indexing-architecture.md b/docs/indexing-architecture.md index 98fe885..c44207a 100644 --- a/docs/indexing-architecture.md +++ b/docs/indexing-architecture.md @@ -11,7 +11,9 @@ This document defines the Rust architecture for the Lash indexing engine, which ## Design Goals 1. **Performance**: Index 1000 files in <5 seconds -2. **Incremental**: Only reparse files that have changed (hash-based detection) +2. **Incremental**: Only reparse files that have changed (hash-based detection), + *plus* a full re-derive when the ID rules themselves change — see + [ID derivation versioning](#id-derivation-versioning) 3. **Parallel**: Parse files concurrently to maximize throughput 4. **Robust**: Aggregate errors rather than fail-fast; leave DB in consistent state 5. **Verifiable**: Detect drift between filesystem and database @@ -1272,3 +1274,67 @@ This architecture provides: - **Extensibility**: Easy to add watch mode, incremental dep resolution, etc. The design prioritizes simplicity and maintainability while meeting all performance targets. The public API is minimal and easy to use from the CLI, while the internal architecture is modular and testable. + +## ID Derivation Versioning + +Hash-based incremental indexing rests on an assumption that is true of file +content and false of derived data: *if the input has not changed, the output +has not changed either*. A task's ID is derived from its title and is not +written to the Markdown unless the author pins it with `@id:`, so the ID is a +function of the derivation code as much as of the file. Change that code and +every unpinned ID moves — while every content hash stays exactly the same. + +Without a guard, the result is silent and long-lived. A file nobody has edited +since the upgrade is never re-parsed, so it keeps serving IDs derived under +rules no longer in force. `lash show` reports the stored ID; `lash lint` derives +a different one and refuses to resolve it; `lash check-index` compares hashes, +finds them equal, and calls the index in sync. Nothing points at the cause. + +### The version stamp + +`lash_types::task::ID_DERIVATION_VERSION` names the current rules. The index +records it under the `id_derivation_version` key in the `metadata` table. + +- **Match** — index incrementally, as before. +- **Mismatch or absent** — ignore the hash diff entirely and re-parse every + file, because the files are not what changed. + +The stamp is written only after a run that can vouch for the whole project: an +unscoped run with no parse errors. A scoped run (`lash index tasks/`) re-derives +part of the project, and a run with parse errors leaves those files' old rows in +place; stamping after either would claim a freshness the index does not have, +and the next run would skip the repair. + +**Bump the constant whenever `synthesize_task_id` changes what it returns for +any input.** An upgrade then repairs itself on the next `lash index`. + +### Capturing what moved + +Correcting the stored IDs is only half the repair. A `@depends-on` written +against an old ID is text in a file, and it stops resolving the moment the +stored IDs move — all of them at once, which makes the rebuild look like the +thing that caused the damage. + +The re-derive is the only moment both spellings of an ID exist: the stored rows +are still in place and the freshly parsed tasks are in hand. So that is where +the mapping is captured, into the `id_migrations` table, for `lash migrate-ids` +to consume. + +Old rows are matched to new tasks by **title plus structural position** +(`depth`, `order_index`) — none of which the ID rules touch. That pairing is +exact only because the file's content hash is unchanged, which the indexer +checks first; a file that was edited has had its structure shift underneath the +stored rows, and is re-indexed on its own hash anyway. Any key claimed by more +than one task on either side is dropped rather than guessed at: an ambiguous +pairing would produce a rename that `lash migrate-ids` writes into someone's +Markdown, so a missed rename — which surfaces as an unresolved reference the +author reads and fixes — is the better failure. + +### Verification + +`IndexVerifier` re-parses each file whose hash already matches and compares the +derived IDs against the stored ones. That is the expensive path for an +otherwise cheap check, and it is the only one that catches this: an unchanged +file is precisely the file whose IDs never get re-derived. It reports +`IssueKind::StaleTaskIds`, naming the stale IDs. Set +`VerifierConfig::with_task_id_check(false)` to skip it. diff --git a/docs/user-guide.md b/docs/user-guide.md index 32aa449..f7064d3 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -597,6 +597,48 @@ lash check-index --diff - `0` - Database is consistent - `1` - Inconsistencies found +Alongside the usual checks (stale records, missing files, hash mismatches), +`check-index` compares the stored task IDs against what Lash derives today. +A file whose content has not changed still drifts if the derivation rules +changed under it, and hash comparison alone cannot see that. + +#### `lash migrate-ids` + +Rewrite `@depends-on` references left dangling by a task-ID derivation change. + +```bash +# Show what changed and which references it affects +lash migrate-ids + +# Rewrite them, then re-index +lash migrate-ids --write + +# Discard the pending renames without rewriting anything +lash migrate-ids --forget +``` + +**Options:** +- `--write` - Apply the rewrites (without it, nothing is written) +- `--forget` - Discard the pending renames, for repairs done by hand +- `--json` - JSON output + +**Exit codes:** +- `0` - Nothing pending, or the rewrite succeeded +- `1` - Renames are pending and nothing has been written yet + +**Background:** a task with no explicit `@id:` gets its ID derived from its +title, so a release that changes the derivation rules moves every such ID. +`lash index` notices, re-derives the stored IDs, and records what each one used +to be — the only moment both spellings exist. `lash migrate-ids` consumes that +record. + +Only whole references on `@depends-on:` lines are rewritten. Prose that happens +to mention an old ID is left alone, and so is the unqualified `old-id` form, +since a bare token can name a file as readily as a task. Run `lash lint` after +migrating to catch anything left. + +To keep an ID stable across future changes, pin it with `@id:`. + ### Querying Tasks #### `lash list` From 64cb7ba3635368861cf5e8819128278cd8a92611 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 11 Aug 2026 15:54:50 -0600 Subject: [PATCH 2/3] fix(migrate-ids): preserve line endings and use the project file walker Rejoining split lines with \n rewrote every line ending in a CRLF file as a side effect of changing one reference. Discovery now goes through FileWalker so the rewritten set matches what the project indexes. --- crates/lash-cli/src/commands/migrate_ids.rs | 174 ++++++++++++++------ 1 file changed, 127 insertions(+), 47 deletions(-) diff --git a/crates/lash-cli/src/commands/migrate_ids.rs b/crates/lash-cli/src/commands/migrate_ids.rs index 3e51d78..abc16ee 100644 --- a/crates/lash-cli/src/commands/migrate_ids.rs +++ b/crates/lash-cli/src/commands/migrate_ids.rs @@ -18,7 +18,8 @@ use anyhow::{Context, Result}; use clap::Args; use lash::theme::CliTheme; use lash_db::{ - open_database, IdMigrationRepository, Indexer, IndexerConfig, TaskIdRename, TaskRepository, + open_database, FileWalker, FileWalkerConfig, IdMigrationRepository, Indexer, IndexerConfig, + TaskIdRename, TaskRepository, }; use lash_types::error::Diagnostic; use lash_types::LashConfig; @@ -209,49 +210,26 @@ fn depends_on_value(line: &str) -> Option<&str> { line.trim_start().strip_prefix("@depends-on:") } -/// Every Markdown file under the project root +/// Every Markdown file the project considers its own /// -/// Deliberately not restricted to files the indexer considers task files: a -/// file with no `## Tasks` section of its own can still carry a `@depends-on` -/// pointing at one that does. +/// Uses the same walker `lash index` does, so the set of files whose +/// references get rewritten is the set the project already treats as part of +/// itself — same excludes, same gitignore handling. It is deliberately *not* +/// narrowed to files the indexer accepts as task files: a file with no +/// `## Tasks` section of its own can still carry a `@depends-on` pointing at +/// one that does. fn markdown_files(project_root: &Path) -> Result> { - let mut files = Vec::new(); - collect_markdown(project_root, &mut files)?; + let walker = FileWalker::new(FileWalkerConfig::new(project_root.to_path_buf())); + let mut files: Vec = walker + .discover_files() + .context("Failed to scan the project for Markdown files")? + .into_iter() + .map(|meta| meta.absolute_path) + .collect(); files.sort(); Ok(files) } -/// Recursive half of [`markdown_files`] -fn collect_markdown(dir: &Path, out: &mut Vec) -> Result<()> { - let Ok(entries) = std::fs::read_dir(dir) else { - return Ok(()); - }; - - for entry in entries.flatten() { - let path = entry.path(); - let name = entry.file_name(); - let name = name.to_string_lossy(); - - // `.lash` holds the index, `.git` holds history, and neither contains - // task references. Other dot-directories are skipped for the same - // reason `lash index` ignores them. - if name.starts_with('.') { - continue; - } - if name == "target" || name == "node_modules" { - continue; - } - - if path.is_dir() { - collect_markdown(&path, out)?; - } else if path.extension().is_some_and(|ext| ext == "md") { - out.push(path); - } - } - - Ok(()) -} - /// Matches a written reference against the recorded renames struct RenameLookup<'a> { /// Keyed by `(file spelling, old local id)`, where the file spelling is @@ -350,6 +328,12 @@ fn normalize_spelling(spelling: &str) -> String { /// /// Each file is read, edited and written once. Only the exact reference tokens /// found by [`find_rewrites`] are replaced, on the lines they were found on. +/// +/// Lines are split with their terminators attached and put back untouched, so +/// a CRLF file stays CRLF and a file with no trailing newline keeps not having +/// one. Splitting on content and rejoining with `\n` would rewrite every line +/// ending in the file as a side effect of changing one reference — a diff the +/// author did not ask for, on Windows checkouts especially. fn apply_rewrites(project_root: &Path, rewrites: &[ReferenceRewrite]) -> Result<()> { let mut by_file: HashMap<&PathBuf, Vec<&ReferenceRewrite>> = HashMap::new(); for rewrite in rewrites { @@ -364,28 +348,41 @@ fn apply_rewrites(project_root: &Path, rewrites: &[ReferenceRewrite]) -> Result< let content = std::fs::read_to_string(&absolute_path) .with_context(|| format!("Failed to read {}", absolute_path.display()))?; - let ends_with_newline = content.ends_with('\n'); - let mut lines: Vec = content.lines().map(String::from).collect(); + let mut lines: Vec = content.split_inclusive('\n').map(String::from).collect(); for rewrite in file_rewrites { let Some(line) = lines.get_mut(rewrite.line_number - 1) else { continue; }; - *line = replace_reference(line, &rewrite.old_reference, &rewrite.new_reference); - } - - let mut updated = lines.join("\n"); - if ends_with_newline { - updated.push('\n'); + // Hold the terminator aside so the replacement cannot disturb it. + let (text, terminator) = split_terminator(line); + *line = format!( + "{}{terminator}", + replace_reference(text, &rewrite.old_reference, &rewrite.new_reference) + ); } - std::fs::write(&absolute_path, updated) + std::fs::write(&absolute_path, lines.concat()) .with_context(|| format!("Failed to write {}", absolute_path.display()))?; } Ok(()) } +/// Split a line into its text and its line terminator +/// +/// The terminator is `"\r\n"`, `"\n"`, or `""` for a final line with no +/// trailing newline. +fn split_terminator(line: &str) -> (&str, &str) { + if let Some(text) = line.strip_suffix("\r\n") { + (text, "\r\n") + } else if let Some(text) = line.strip_suffix('\n') { + (text, "\n") + } else { + (line, "") + } +} + /// Replace one whole reference on a `@depends-on:` line /// /// Splits on commas and swaps the matching token rather than doing a substring @@ -809,6 +806,89 @@ mod tests { ); } + #[test] + fn test_split_terminator_recognises_each_ending() { + assert_eq!(split_terminator("text\r\n"), ("text", "\r\n")); + assert_eq!(split_terminator("text\n"), ("text", "\n")); + // A final line with no trailing newline. + assert_eq!(split_terminator("text"), ("text", "")); + } + + #[test] + fn test_rewriting_preserves_crlf_line_endings() { + // Splitting on content and rejoining with \n would rewrite every line + // ending in the file as a side effect of changing one reference. + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("tasks.md"); + std::fs::write(&path, "# T\r\n\r\n@depends-on: tasks#old-id\r\n- [ ] A\r\n").unwrap(); + + apply_rewrites( + temp.path(), + &[ReferenceRewrite { + source_path: PathBuf::from("tasks.md"), + line_number: 3, + old_reference: "tasks#old-id".to_string(), + new_reference: "tasks#new-id".to_string(), + }], + ) + .unwrap(); + + let content = std::fs::read_to_string(&path).unwrap(); + assert_eq!( + content, "# T\r\n\r\n@depends-on: tasks#new-id\r\n- [ ] A\r\n", + "only the reference may change" + ); + } + + #[test] + fn test_rewriting_does_not_add_a_trailing_newline() { + // A file that did not end in a newline must not start doing so. + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("tasks.md"); + std::fs::write(&path, "@depends-on: tasks#old-id").unwrap(); + + apply_rewrites( + temp.path(), + &[ReferenceRewrite { + source_path: PathBuf::from("tasks.md"), + line_number: 1, + old_reference: "tasks#old-id".to_string(), + new_reference: "tasks#new-id".to_string(), + }], + ) + .unwrap(); + + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "@depends-on: tasks#new-id" + ); + } + + #[test] + fn test_find_and_apply_agree_on_line_numbers() { + // `find_rewrites` counts with `lines()` and `apply_rewrites` indexes + // into `split_inclusive('\n')`. They must stay in step or a rewrite + // lands on the wrong line. + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("tasks.md"); + std::fs::write( + &path, + "# T\n\n## Tasks\n\n- [ ] A\n- [ ] B\n @depends-on: tasks#old-id\n", + ) + .unwrap(); + + let renames = vec![rename("tasks.md", "tasks", "old-id", "new-id")]; + let found = find_rewrites(temp.path(), &renames).unwrap(); + + assert_eq!(found.len(), 1); + assert_eq!(found[0].line_number, 7); + + apply_rewrites(temp.path(), &found).unwrap(); + let content = std::fs::read_to_string(&path).unwrap(); + assert!(content.contains(" @depends-on: tasks#new-id")); + assert!(content.contains("- [ ] B\n")); + } + #[test] fn test_depends_on_value_requires_the_annotation() { assert_eq!(depends_on_value(" @depends-on: a, b"), Some(" a, b")); From 5f8f679cbf9bc358b79ed67adc1eb426f56c82c8 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Tue, 11 Aug 2026 16:10:34 -0600 Subject: [PATCH 3/3] fix(index): salvage the rename mapping across a --force rebuild --force wipes the database, which is the only place a task's previous ID survives. Pending renames are now read out (and undetected drift drawn out) before the wipe, and restored after. --- CHANGELOG.md | 5 +- crates/lash-cli/src/commands/index.rs | 79 ++++++++++++-- .../tests/id_derivation_drift_test.rs | 101 ++++++++++++++++++ 3 files changed, 177 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bc3791..a3fc23f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,10 @@ While the major version is 0, minor version bumps may contain breaking changes. version it was built under and re-derives every file when that does not match, so an upgrade repairs itself on the next `lash index`. The IDs that moved are reported, and recorded for `lash migrate-ids` — the re-derive is - the only moment both spellings exist. + the only moment both spellings exist. `lash index --force`, which wipes the + database, salvages the mapping before doing so — it was the workaround people + reached for, and it used to destroy the one record that could repair the + references it broke. - `lash check-index` compares stored task IDs against freshly derived ones instead of only comparing content hashes, which by construction cannot see a change in how IDs are derived from unchanged content. diff --git a/crates/lash-cli/src/commands/index.rs b/crates/lash-cli/src/commands/index.rs index 30b3075..6178764 100644 --- a/crates/lash-cli/src/commands/index.rs +++ b/crates/lash-cli/src/commands/index.rs @@ -68,6 +68,23 @@ pub fn execute(args: IndexArgs) -> Result { // Determine database path let db_path = get_database_path(&project_root)?; + // Load project configuration + let parser_config = LashConfig::from_root(&project_root).unwrap_or_else(|_| { + tracing::debug!("No project config found, using defaults"); + LashConfig::default() + }); + + // A force rebuild throws the old records away, and those records are the + // only place a task's previous ID survives. Salvage any pending renames + // first, so `--force` — which is what people reach for when the index + // looks wrong — does not destroy the one thing that can repair the + // references it is about to break (GitHub issue #54). + let preserved_renames = if args.force && db_path.exists() { + salvage_pending_renames(&db_path, &project_root, &parser_config) + } else { + Vec::new() + }; + // Initialize or open database let conn = if args.force || !db_path.exists() { // Force rebuild or DB doesn't exist - initialize fresh @@ -84,12 +101,6 @@ pub fn execute(args: IndexArgs) -> Result { // Run migrations to ensure schema is up to date run_migrations(&conn).context("Failed to run database migrations")?; - // Load project configuration - let parser_config = LashConfig::from_root(&project_root).unwrap_or_else(|_| { - tracing::debug!("No project config found, using defaults"); - LashConfig::default() - }); - // Configure indexer let mut indexer_config = IndexerConfig::new(project_root.clone()) .with_incremental(!args.force) @@ -160,7 +171,20 @@ pub fn execute(args: IndexArgs) -> Result { } // Execute indexing - let report = indexer.index_project().context("Failed to index project")?; + let mut report = indexer.index_project().context("Failed to index project")?; + + // Carry the salvaged renames across the rebuild that just wiped them. + if !preserved_renames.is_empty() { + lash_db::IdMigrationRepository::new(&conn) + .record_all(&preserved_renames) + .context("Failed to preserve pending ID renames across the rebuild")?; + for rename in preserved_renames { + if !report.id_renames.contains(&rename) { + report.id_renames.push(rename); + } + } + } + let report = report; // Convert parse errors to LashError types and report them // (Do this before clearing the progress bar so we can use it for suspended output) @@ -213,6 +237,47 @@ pub fn execute(args: IndexArgs) -> Result { } } +/// Pending ID renames worth carrying across a `--force` rebuild +/// +/// Two sources, both of which the rebuild is about to erase: +/// +/// - renames already recorded by an earlier run and not yet migrated; +/// - renames not yet detected, because the index has never been re-derived +/// since the rules changed. Those only exist while the old task rows do, so +/// an incremental pass runs first to draw them out. +/// +/// Best-effort throughout. A database too broken to read is exactly why +/// someone reached for `--force`, and refusing to rebuild because the salvage +/// failed would be the wrong trade. +fn salvage_pending_renames( + db_path: &Path, + project_root: &Path, + parser_config: &LashConfig, +) -> Vec { + let Ok(conn) = open_database(db_path) else { + return Vec::new(); + }; + if run_migrations(&conn).is_err() { + return Vec::new(); + } + + // Draw out any drift the index has not noticed yet. Doing the work twice + // costs one extra pass on the single run that finds something, and it is + // the only chance to see both spellings of an ID at once. + if lash_db::get_id_derivation_version(&conn).ok().flatten() + != Some(lash_types::task::ID_DERIVATION_VERSION) + { + let config = IndexerConfig::new(project_root.to_path_buf()) + .with_incremental(true) + .with_progress(false); + let _ = Indexer::new(&conn, config, parser_config).index_project(); + } + + lash_db::IdMigrationRepository::new(&conn) + .list_pending() + .unwrap_or_default() +} + /// Get the database path for a project fn get_database_path(project_root: &Path) -> Result { let lash_dir = project_root.join(".lash"); diff --git a/crates/lash-cli/tests/id_derivation_drift_test.rs b/crates/lash-cli/tests/id_derivation_drift_test.rs index 7c6719e..a5bc384 100644 --- a/crates/lash-cli/tests/id_derivation_drift_test.rs +++ b/crates/lash-cli/tests/id_derivation_drift_test.rs @@ -181,6 +181,107 @@ fn test_the_repair_does_not_repeat_itself() { assert_eq!(pending, 1, "the same rename must not accumulate"); } +#[test] +fn test_force_rebuild_does_not_destroy_the_rename_mapping() { + // `--force` was the workaround people reached for, and it wipes the + // database — the only place a task's previous ID survives. It used to + // correct the stored IDs and leave every reference dangling with nothing + // left to explain them. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .arg("--force") + .assert() + .success() + .stdout(predicate::str::contains("task ID changed")); + + let conn = db(&project); + let pending: i64 = conn + .query_row("SELECT COUNT(*) FROM id_migrations", [], |row| row.get(0)) + .unwrap(); + assert_eq!(pending, 1, "the mapping must survive the rebuild"); + assert!(stored_ids(&project).contains(&CURRENT_ID.to_string())); +} + +#[test] +fn test_force_rebuild_then_migrate_recovers_fully() { + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .arg("--force") + .assert() + .success(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .arg("--write") + .assert() + .success(); + + assert!(index_file(&project).contains(&format!("@depends-on: index#{CURRENT_ID}"))); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("lint") + .assert() + .success(); +} + +#[test] +fn test_force_rebuild_on_a_healthy_index_stays_quiet() { + // The salvage pass must not make an ordinary `--force` noisy. + let project = project_with_a_reference(); + index(&project); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .arg("--force") + .assert() + .success() + .stdout(predicate::str::contains("task ID changed").not()); +} + +#[test] +fn test_force_rebuild_carries_an_already_recorded_rename() { + // A rename detected by an earlier run, not yet migrated, when someone + // then runs --force. + let project = project_with_a_reference(); + index(&project); + simulate_pre_upgrade_index(&project); + index(&project); // records the rename + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("index") + .arg("--force") + .assert() + .success(); + + run_lash_command() + .arg("--root") + .arg(project.path()) + .arg("migrate-ids") + .assert() + .failure() + .stdout(predicate::str::contains("Would rewrite 1 reference")); +} + // --------------------------------------------------------------------- // check-index reports the drift instead of passing // ---------------------------------------------------------------------