diff --git a/CLAUDE.md b/CLAUDE.md index facb9d5..20e9b08 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ make release — release build ## Principles -**Fail fast.** No fallbacks. No swallowed errors. No "keep calm and carry on." CLI tools for agents serving two people — crash beats silent corruption. +**Fail fast.** No fallbacks. No swallowed errors. No "keep calm and carry on." Crash beats silent corruption. **Resist disabling lint checks.** We use Rust to force correct code. Fix clippy warnings, don't suppress them. diff --git a/Makefile b/Makefile index a4aab4c..41d12b5 100644 --- a/Makefile +++ b/Makefile @@ -28,6 +28,7 @@ release-musl: fmt: cargo fmt -- --check +# TODO(clippy-gate-skips-tests): no --all-targets, so test code is ungated. clippy: cargo clippy -- -D warnings diff --git a/TODO.md b/TODO.md index 33372dc..b1309c0 100644 --- a/TODO.md +++ b/TODO.md @@ -1,67 +1,68 @@ # TODOs -## `done-error-taxonomy-cleanup` - -`DoneError::Invalid(String)` and `DoneError::Infra(anyhow::Error)` (src/todo.rs) are two catch-all variants that every caller must special-case: the MCP layer routes `Infra` to `Err(e)` while emitting the rest as `Ok(envelope)`; CLI JSON mode does the same; CLI human mode stringifies everything. Consider splitting `Invalid` into a small set of named variants (path-not-md, replacing-with-no-stored, replacing-redundant, missing-anchor) so frontends can branch on named codes, and keeping `Infra` as the sole catch-all with a `DoneError::is_infra()` predicate so callers don't pattern-match on the specific variant. - -## `done-error-derive-serialize` - -`DoneError::to_json` (src/todo.rs) hand-rolls the `{error, reason, ...payload}` envelope by mutating a `serde_json::Value`. A `#[derive(Serialize)]` with `#[serde(tag = "error", rename_all = "snake_case")]` would replace the 80-line `code()` / `to_json()` pair with framework-driven serialization; the `reason` string can stay computed in a `reason()` accessor. Phase 3 added `ScheduleError` with the same hand-rolled idiom (code/reason/to_json/Display/Error/From) — the pattern is now load-bearing for two error types, with a likely third (`ReorderError`, `CancelError`) each time a mutator grows structured errors. Sweep both (all) enums together; the refactor is purely mechanical, no semantic change. - -## `done-anchored-double-rrule-parse` - -`done_anchored` in src/todo.rs parses and iterates the rrule twice on the stale-anchor branch — once via `compute_next_occurrence` for the naive next anchor, once via `compute_occurrence_after` for the skip-past target. A single helper that returns both values in one iteration would halve the parse+iterate work. Once per user action, so the impact is negligible; cleanup only. - -## `workspace-handle` - -Multiple callers carry `repo_dir` in their Options structs and independently call `open_and_sync`. Consider a higher-level `Workspace` handle that wraps `repo_dir` + `Cache`, so callers share a single entry point. See `db.rs`. - -## `cross-repo-ref-validation` - -`graf manifest check` validates manifest-level consistency (slugs unique, IDs match, paths exist) but does not validate cross-repo refs. A repo's `.graf/config.toml` `refs` section maps slugs to global IDs — `check` could verify those IDs exist in the application manifest and that the slugs don't conflict. Deferred per design doc: "Cross-repo ref validation (do slugs in refs resolve?) is deferred." - -## `sort-key-duplication` - -The SQL `ORDER BY` in `db.rs` and the Rust `todo_sort_cmp()` in `cli/mod.rs` encode the same sort logic independently. The Rust comparator is used for k-way merge of multi-repo results; the SQL ORDER BY handles single-repo. They must agree. The integration test `test_multi_repo_sort_matches_single_repo_sql_order` catches divergence. Consider generating the sort key from a shared definition to make divergence impossible. - -## `mcp-config-caching` - -`AppConfig::load_default()` is called on every MCP `graf_todo_query` tool call, re-reading and parsing the config file from disk each time. The MCP server is long-lived and config won't change mid-session. Load the config once at server startup and store it in `McpContext`, then reference it from tool `call()` methods without per-request I/O. - -## `xdg-path-duplication` - -`AppConfig::default_path()` and `AppManifest::default_path()` both implement the same XDG config directory logic (`XDG_CONFIG_HOME` → fallback `~/.config`). Extract a shared `graf_config_dir()` helper when a third caller appears. - -## `done-horizon` - -`--include-done --horizon N` exempts all terminal items from the horizon filter, returning every done/cancelled task regardless of when it was completed. For a user with many historical completed items this could produce a large result set. Consider adding a separate horizon for terminal items — e.g., only include items completed within the last N days — or filtering terminal items by `on_date`/`end_date` relative to the horizon cutoff. See the horizon exemption in `db.rs` `query_tasks()`. - -## `sql-terminal-status-strings` - -The SQL strings `status IN ('done', 'cancelled')` appear in the ORDER BY and horizon WHERE clauses of `db.rs`. These are hardcoded string literals that must stay in sync with `Status::Done` and `Status::Cancelled` serialization. If a new terminal status were added, these would need manual updates found only by text search. Consider building the SQL fragment from `Status::is_terminal()` variants or a shared constant. - -## `resolve-target-env-isolation` - -`resolve_target` unit tests call the real `AppManifest::load_default()`, so results depend on whether the user's `~/.config/graf/manifest.toml` exists and what slugs it contains. The function should accept a manifest (or manifest loader) as a parameter instead of reading the global config implicitly. This would also make `resolve_target` more testable in general. See `cli/mod.rs` `resolve_target()` and its tests. - -## `mcp-preflight-shared-error-type` - -`DoneError::PathNotFound`/`InvalidDate` (`src/todo.rs`) and `ScheduleError::PathNotFound`/`InvalidDate` (`src/todo.rs`) are structurally identical variants with identical `code()`, `reason()`, and `to_json()` arms. When `cancel-mcp-envelope` lands a third copy will appear. Extract a shared `McpPreflightError` enum (in `src/cli/mcp.rs` or a new `src/cli/mcp_errors.rs`) and embed it via a variant in each tool-specific error enum. Best tackled alongside `done-error-derive-serialize`. See `TODO(mcp-preflight-shared-error-type)` comment at `src/todo.rs`. - -## `recurring-phase-2-remove-flat-rrule` +## `recurring-phase-2-remove-flat-rrule` — BLOCKED as of 2026-07-20 `Frontmatter.rrule: Option` is retained as a legacy-shape compat field (`src/frontmatter.rs`). Reindex eagerly migrates old files into the nested `recurrence` block, and no write path emits flat `rrule`. Once we're confident no repos still carry legacy-shape files (or are willing to surface them as parse errors for manual fix-up), remove the field and the associated `Field::Rrule` variant, plus the legacy-compat branches in `maybe_migrate_legacy` and `effective_rrule`. -## `migration-rrule-syntax-validation` - -`maybe_migrate_legacy` in `src/migrate.rs` rewrites legacy frontmatter without validating that the `rrule` string is parseable. A file with a broken `rrule` (e.g. `FREQ=BOGUS`) migrates into the nested shape still broken; runtime breakage surfaces only when `compute_next_occurrence` is invoked. The intended behavior is that a `FREQ=BOGUS` rrule records a `FileError` and leaves the file untouched, but neither lint nor parse validates rrule syntax elsewhere, so migration isn't the natural place to add enforcement. Consider adding an rrule-syntax lint rule that applies uniformly; migration would then surface the issue via the post-rewrite lint pass. - -## `ephemeral-lint-errors` - -`TodoQueryResult.lint_errors` is populated from the current sync pass's `sync.errors`, which only contains errors for files the sync actually re-linted (step 5 working-tree changes, step 4 HEAD diff, step 2 reconcile-dirty). A file with a lint violation that was committed long ago and hasn't been touched since gets linted once when indexed; on subsequent queries its lint error is not re-emitted — the error message is never persisted in the cache. - -Null-`effective_date` rows are exempt from this silence because `todo_query` partitions them from `tasks` into `lint_errors` at the response-building layer on every call (see the invariant on `TodoQueryResult` and the test `test_null_effective_date_partitioned_across_queries`). But other lint rules (e.g. terminal item with `check_in_date` set, `end_date` without `start_date`) still exhibit the silence. - -`graf lint` runs a full-repo scan and is the authoritative way to surface all current violations. `graf todo`'s `lint_errors` is therefore a "what changed in this sync pass" channel plus the null-date partition, not a comprehensive "what's wrong in the repo" report. - -Fix options: (1) persist lint error messages in the cache (column on `docs` or a side table), refresh the set on each file re-lint; (2) re-run `check_rules` at query time against every cache row (likely too expensive for large repos); (3) document the current semantics and treat `graf lint` as the authoritative surface. Option (1) has the best cost/value ratio and would let `graf todo`'s `lint_errors` become trustworthy in steady state. +## `persist-parse-errors` + +Lint-rule violations are persisted per-doc in `docs.lint_errors`, so `graf todo` reports them on every query. Parse and validation failures are not: a file that fails to parse has its cache row deleted (`reconcile_dirty` and `apply_changed_files` in `src/db.rs`), so its error is reported only on the sync pass that touched it — the same ephemerality that persisted lint state fixed for lint rules. + +Persisting these means keeping rows for files with no valid frontmatter, which have no `tldr` and no dates: either a nullable-everything doc row or a separate errors table. That is a schema question of its own, and was out of scope for the lint-persistence work. Until then, `graf lint` remains the way to surface standing parse failures. + + +## `resolve-slug-arm-duplication` + +`resolve_repos`' slug arm (`src/cli/mod.rs`) and `resolve_target`'s +`DocRef::Qualified` arm independently perform the same sequence: +`find_repo_by_slug` → error on miss → `AppManifest::expand_path` → +`load_repo_config_required` → construct `ResolvedRepo`. The "unknown repo slug" +error wording has already diverged slightly between the two arms, and any future +per-repo metadata added to `ResolvedRepo` must be plumbed twice. Now that both +arms share a `manifest: Option<&AppManifest>` parameter, extract +`fn resolve_manifest_slug(manifest: &AppManifest, slug: &Slug) -> Result` +and call it from both. Deferred: the resolver-threading design deliberately +scoped to "plain data parameter, no restructuring," so this cleanup is a +separate change. + +## `xdg-empty-unset` + +`graf_config_dir_from()` (`src/config.rs`) uses an empty-but-set +`$XDG_CONFIG_HOME` verbatim, resolving config/manifest relative to the process +CWD. The XDG Base Directory spec requires an empty variable to be treated as +unset (fall back to `$HOME/.config`). The behavior is pre-existing (the old +`env::var` block did the same) and was deliberately preserved as a pure +extraction — the `config_dir_honors_empty_xdg_verbatim` test pins it so a future +fix is a deliberate change. Fixing it (`xdg_config_home.filter(|s| +!s.is_empty())`, invert the test) is a behavior change with CI/systemd/container +blast radius, so it is deferred to a design decision rather than folded into a +refactor. + +## `clippy-gate-skips-tests` + +The `make check` clippy gate (`Makefile`, `clippy:` target) runs `cargo clippy +-- -D warnings` without `--all-targets`, so test code is ungated. `cargo clippy +--all-targets` currently emits ~25 warnings in test modules (useless `vec!`, +redundant closures, etc.). CLAUDE.md's "clippy must pass with no warnings" thus +holds only for lib/bin code while test-code lint rot accumulates silently. The +warnings predate this work. Fixing means adding `--all-targets` to the clippy +target and burning down the existing test-code warnings — a project-wide +lint-gate policy change, separate from any feature work. + +## `frozen-series-effective-date` + +`EFF_DATE` in `src/db.rs` resolves an item's effective date as +`COALESCE(tentative_date, check_in_date, on_date, end_date, due_date)`, on the +premise that terminal items have their scheduling dates stripped and so sort +and report by completion date. A frozen series breaks that premise: anchored +exhaustion (`anchored_terminal_completion` in `src/todo.rs`) preserves +`check_in_date` frozen at the final occurrence, so an exhausted series reports +and sorts by that occurrence rather than by its `on_date` completion date — +off by however long completion lagged the last occurrence. + +Which date such an item should show is a product question against the phase-2 +freeze semantics (PRD-data-model §2), not a local bug fix: either the terminal +branch of the effective-date expression prefers `on_date`/`end_date` (a schema +bump plus an index review, since `EFF_DATE` must stay textually identical +across the schema index, SELECT, WHERE and ORDER BY), or the final-occurrence +ordering is declared intended and documented as such. diff --git a/src/bin/migrate_recurrence.rs b/src/bin/migrate_recurrence.rs index 5285c30..103dab5 100644 --- a/src/bin/migrate_recurrence.rs +++ b/src/bin/migrate_recurrence.rs @@ -225,10 +225,7 @@ fn validate( let mut done: Vec = Vec::new(); for inst in ctrl_instances.drain(..) { - let is_terminal = matches!( - inst.fm.status, - Some(frontmatter::Status::Done | frontmatter::Status::Cancelled) - ); + let is_terminal = inst.fm.status.as_ref().is_some_and(|s| s.is_terminal()); if is_terminal { done.push(inst); } else { diff --git a/src/cli/fix.rs b/src/cli/fix.rs index e877f51..3b6615c 100644 --- a/src/cli/fix.rs +++ b/src/cli/fix.rs @@ -2,6 +2,7 @@ use anyhow::Result; use clap::Args; use graf::lint::{self, LintOptions}; +use graf::manifest::AppManifest; #[derive(Args)] pub struct FixArgs { @@ -15,7 +16,8 @@ pub struct FixArgs { } pub fn run(args: FixArgs) -> Result<()> { - let resolved = super::resolve_single_repo(args.repo.as_deref())?; + let manifest = AppManifest::load_default()?; + let resolved = super::resolve_single_repo(args.repo.as_deref(), manifest.as_ref())?; let options = LintOptions { fix: true, repo_dir: resolved.path, diff --git a/src/cli/lint.rs b/src/cli/lint.rs index f2a7138..fe0494b 100644 --- a/src/cli/lint.rs +++ b/src/cli/lint.rs @@ -3,6 +3,7 @@ use clap::Args; use std::path::PathBuf; use graf::lint::{self, LintResult}; +use graf::manifest::AppManifest; #[derive(Args)] pub struct LintArgs { @@ -32,10 +33,14 @@ pub fn run(args: LintArgs) -> Result<()> { bail!("--changed and explicit file paths are mutually exclusive"); } + let manifest = AppManifest::load_default()?; let repos = if !args.files.is_empty() { - vec![super::resolve_single_repo(args.repo.as_deref())?] + vec![super::resolve_single_repo( + args.repo.as_deref(), + manifest.as_ref(), + )?] } else { - super::resolve_repos(args.repo.as_deref())? + super::resolve_repos(args.repo.as_deref(), manifest.as_ref())? }; let combined = super::multi_repo_lint( diff --git a/src/cli/manifest.rs b/src/cli/manifest.rs index ea3d8d6..55a89e9 100644 --- a/src/cli/manifest.rs +++ b/src/cli/manifest.rs @@ -222,24 +222,22 @@ fn run_check(args: CheckArgs) -> Result<()> { std::process::exit(1); } } else { - let mut errors = manifest.validate(); - errors.extend(manifest.validate_repos()); - let warnings = manifest.warnings(); - for warning in &warnings { + let report = manifest.check_report(); + for warning in &report.warnings { eprintln!("warning: {warning}"); } - if errors.is_empty() { + if report.errors.is_empty() { eprintln!( "Manifest OK ({} repos, {} domains)", manifest.repo.len(), manifest.domain.len() ); } else { - for error in &errors { + for error in &report.errors { eprintln!("error: {error}"); } - bail!("{} validation error(s)", errors.len()); + bail!("{} validation error(s)", report.errors.len()); } } diff --git a/src/cli/mcp.rs b/src/cli/mcp.rs index cea9a53..ffc1afd 100644 --- a/src/cli/mcp.rs +++ b/src/cli/mcp.rs @@ -6,6 +6,7 @@ use chrono::NaiveDate; use clap::Args; use serde_json::{Value, json}; +use graf::config::AppConfig; use graf::frontmatter::{Effort, Recurrence, Status}; use graf::lint::{self, LintOptions}; use graf::manifest::AppManifest; @@ -25,6 +26,8 @@ pub struct McpArgs { struct McpContext { /// Default repo from --repo flag at server startup. default_repo: Option, + /// App config, loaded once at server startup. + config: AppConfig, } impl McpContext { @@ -32,23 +35,34 @@ impl McpContext { /// if provided, otherwise falls back to the server-level default. fn resolve_single(&self, tool_repo: Option<&str>) -> Result { let repo_arg = tool_repo.or(self.default_repo.as_deref()); - super::resolve_single_repo(repo_arg) + let manifest = AppManifest::load_default()?; + super::resolve_single_repo(repo_arg, manifest.as_ref()) } /// Resolve repos for a query (may return multiple). fn resolve_all(&self, tool_repo: Option<&str>) -> Result> { let repo_arg = tool_repo.or(self.default_repo.as_deref()); - super::resolve_repos(repo_arg) + let manifest = AppManifest::load_default()?; + super::resolve_repos(repo_arg, manifest.as_ref()) } /// Resolve a target document path for mutation tools. /// Passes `tool_repo` for conflict detection, server default as fallback. + /// + /// The manifest is loaded per call (not cached) so tool calls always see + /// current manifest edits on this long-running server. fn resolve_target( &self, path_str: &str, tool_repo: Option<&str>, ) -> Result<(ResolvedRepo, PathBuf)> { - super::resolve_target(path_str, tool_repo, self.default_repo.as_deref()) + let manifest = AppManifest::load_default()?; + super::resolve_target( + path_str, + tool_repo, + self.default_repo.as_deref(), + manifest.as_ref(), + ) } } @@ -75,6 +89,7 @@ graf_ref_attachments, graf_ref_links, graf_ref_context_link."; pub fn run(args: McpArgs) -> Result<()> { let ctx = McpContext { default_repo: args.repo, + config: AppConfig::load_default()?, }; let server = Server::new("graf", env!("CARGO_PKG_VERSION")) @@ -449,7 +464,8 @@ impl Tool for TodoQueryTool { "repo": repo_property(), "horizon_days": { "type": "integer", - "description": "Only include items up to N days in the future" + "description": "Only include active items up to N days in the future \ + and done/cancelled items completed within the last N days" }, "include_done": { "type": "boolean", @@ -475,10 +491,8 @@ impl Tool for TodoQueryTool { fn call(&self, args: Value) -> Result { let repos = self.0.resolve_all(args["repo"].as_str())?; - // TODO(mcp-config-caching) - let config = graf::config::AppConfig::load_default()?; let explicit_limit = args["limit"].as_u64().map(|v| v as usize); - let limit = explicit_limit.or(config.todo.effective_limit()); + let limit = explicit_limit.or(self.0.config.todo.effective_limit()); let result = super::multi_repo_todo_query( &repos, @@ -1205,6 +1219,7 @@ mod tests { fn cancel_tool_ctx(dir: &TempDir) -> TodoCancelTool { TodoCancelTool(McpContext { default_repo: Some(dir.path().to_str().unwrap().to_string()), + config: AppConfig::default(), }) } @@ -1311,7 +1326,10 @@ mod tests { #[test] fn cancel_schema_requires_today() { - let tool = TodoCancelTool(McpContext { default_repo: None }); + let tool = TodoCancelTool(McpContext { + default_repo: None, + config: AppConfig::default(), + }); let schema = tool.input_schema(); let required = schema["required"].as_array().expect("required is array"); let required_strs: Vec<&str> = required.iter().filter_map(|v| v.as_str()).collect(); diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 34c76fe..373daf6 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -45,9 +45,14 @@ impl ResolvedRepo { /// When `--repo` is not given: /// - If a manifest exists: return all repos in the manifest. /// - Otherwise: return cwd as a single repo. -pub fn resolve_repos(repo_arg: Option<&str>) -> Result> { - let manifest = AppManifest::load_default()?; - +/// +/// The caller supplies the manifest (normally from `AppManifest::load_default()`). +/// `None` means no manifest is active, driving the cwd fallback and the +/// "no manifest found" slug errors. +pub fn resolve_repos( + repo_arg: Option<&str>, + manifest: Option<&AppManifest>, +) -> Result> { match repo_arg { Some(arg) => { if looks_like_path(arg) { @@ -57,6 +62,8 @@ pub fn resolve_repos(repo_arg: Option<&str>) -> Result> { } else { // Slug lookup let slug = Slug::new(arg).with_context(|| format!("invalid repo slug: {arg}"))?; + // TODO(resolve-slug-arm-duplication): shares logic with the + // resolve_target Qualified arm below. let manifest = manifest.ok_or_else(|| { anyhow::anyhow!( "no manifest found — cannot resolve slug {arg:?}. \ @@ -105,8 +112,13 @@ pub fn resolve_repos(repo_arg: Option<&str>) -> Result> { } /// Resolve repos, but require exactly one result. Used by mutation commands. -pub fn resolve_single_repo(repo_arg: Option<&str>) -> Result { - let repos = resolve_repos(repo_arg)?; +/// +/// The caller supplies the manifest (normally from `AppManifest::load_default()`). +pub fn resolve_single_repo( + repo_arg: Option<&str>, + manifest: Option<&AppManifest>, +) -> Result { + let repos = resolve_repos(repo_arg, manifest)?; if repos.len() > 1 { bail!("multiple repos found in manifest — specify --repo for this command"); } @@ -128,10 +140,13 @@ pub fn resolve_single_repo(repo_arg: Option<&str>) -> Result { /// /// `default_repo` is a fallback for bare paths when `repo_arg` is `None`. /// Used by MCP tools to pass the server-level `--repo` default. CLI callers pass `None`. +/// +/// The caller supplies the manifest (normally from `AppManifest::load_default()`). pub fn resolve_target( path_str: &str, repo_arg: Option<&str>, default_repo: Option<&str>, + manifest: Option<&AppManifest>, ) -> Result<(ResolvedRepo, PathBuf)> { let doc_ref = DocRef::parse(path_str).with_context(|| format!("invalid path: {path_str:?}"))?; @@ -143,7 +158,9 @@ pub fn resolve_target( but --repo was also specified" ); } - let manifest = AppManifest::load_default()?.ok_or_else(|| { + // TODO(resolve-slug-arm-duplication): shares logic with the + // resolve_repos slug arm above. + let manifest = manifest.ok_or_else(|| { anyhow::anyhow!("no manifest found — cannot resolve slug in path {path_str:?}") })?; let entry = manifest.find_repo_by_slug(&slug).ok_or_else(|| { @@ -162,7 +179,7 @@ pub fn resolve_target( } DocRef::Bare(path) => { let effective_repo = repo_arg.or(default_repo); - let repo = resolve_single_repo(effective_repo)?; + let repo = resolve_single_repo(effective_repo, manifest)?; Ok((repo, path)) } DocRef::Uri(_) => { @@ -356,13 +373,11 @@ pub fn manifest_list_json(manifest: &AppManifest) -> serde_json::Value { /// Validate an `AppManifest` and return the standard check JSON format. pub fn manifest_check_json(manifest: &AppManifest) -> serde_json::Value { - let mut errors = manifest.validate(); - errors.extend(manifest.validate_repos()); - let warnings = manifest.warnings(); + let report = manifest.check_report(); serde_json::json!({ - "ok": errors.is_empty(), - "errors": errors, - "warnings": warnings, + "ok": report.errors.is_empty(), + "errors": report.errors, + "warnings": report.warnings, "repos": manifest.repo.len(), "domains": manifest.domain.len(), }) @@ -393,13 +408,10 @@ fn todo_sort_cmp(a: &TodoItem, b: &TodoItem) -> std::cmp::Ordering { } /// Sort rank: `COALESCE(sort_order, priority, UNRANKED)`. -/// Matches the SQL `ORDER BY` in `db.rs`. +/// Matches the SQL `ORDER BY` in `db.rs`. `OrderedFloat` adapts the shared +/// `frontmatter::rank` formula to the `Ord`-based comparator chain. fn rank(item: &TodoItem) -> OrderedFloat { - OrderedFloat( - item.sort_order - .or(item.priority.map(|p| p as f64)) - .unwrap_or(graf::frontmatter::UNRANKED), - ) + OrderedFloat(graf::frontmatter::rank(item.sort_order, item.priority)) } /// K-way merge of pre-sorted task lists. Each input list must be sorted @@ -730,12 +742,44 @@ mod tests { } // ----------------------------------------------------------------------- - // resolve_target tests + // resolve_* tests // ----------------------------------------------------------------------- + use graf::manifest::ManifestRepo; + use tempfile::TempDir; + + /// Create a temp repo dir containing a minimal `.graf/config.toml`, and + /// return the dir guard plus a matching `ManifestRepo` entry. + fn test_repo(slug: &str, domain: Option<&str>) -> (TempDir, ManifestRepo) { + let dir = TempDir::new().unwrap(); + let slug = Slug::new(slug).unwrap(); + let id = GlobalId::new(&format!("example.com/{slug}")).unwrap(); + let config = RepoConfig { + id: id.clone(), + default_slug: slug.clone(), + domain: domain.map(|d| GlobalId::new(d).unwrap()), + refs: Default::default(), + }; + config.save(dir.path()).unwrap(); + let repo = ManifestRepo { + slug, + path: dir.path().to_string_lossy().into_owned(), + id, + }; + (dir, repo) + } + + /// Build an in-memory manifest from repo entries (no domains). + fn manifest_of(repos: Vec) -> AppManifest { + AppManifest { + repo: repos, + domain: Vec::new(), + } + } + #[test] fn resolve_target_conflict_slug_path_with_repo_arg() { - let err = resolve_target("life:todo/foo.md", Some("life"), None).unwrap_err(); + let err = resolve_target("life:todo/foo.md", Some("life"), None, None).unwrap_err(); let msg = err.to_string(); assert!( msg.contains("conflicting repo"), @@ -745,7 +789,7 @@ mod tests { #[test] fn resolve_target_conflict_different_slug_and_repo() { - let err = resolve_target("life:todo/foo.md", Some("work"), None).unwrap_err(); + let err = resolve_target("life:todo/foo.md", Some("work"), None, None).unwrap_err(); let msg = err.to_string(); assert!( msg.contains("conflicting repo"), @@ -755,7 +799,8 @@ mod tests { #[test] fn resolve_target_uri_rejected() { - let err = resolve_target("graf://example.com/life/todo/foo.md", None, None).unwrap_err(); + let err = + resolve_target("graf://example.com/life/todo/foo.md", None, None, None).unwrap_err(); let msg = err.to_string(); assert!( msg.contains("graf:// URIs not yet supported"), @@ -765,44 +810,148 @@ mod tests { #[test] fn resolve_target_qualified_no_conflict_with_default_repo() { - // A qualified path should not conflict with default_repo (only repo_arg). - // May succeed or fail depending on whether a manifest exists with this - // slug — either outcome is fine as long as there's no conflict error. - // TODO(resolve-target-env-isolation) - let result = resolve_target("life:todo/foo.md", None, Some("life")); - if let Err(e) = result { - let msg = e.to_string(); - assert!( - !msg.contains("conflicting repo"), - "default_repo should not trigger conflict, got: {msg}" - ); - } + // A qualified path resolves via its own slug; default_repo is ignored on + // that branch (only repo_arg conflicts). default_repo points at a + // *different* repo to prove it does not steer the result. + let (life_dir, life) = test_repo("life", None); + let (_other_dir, other) = test_repo("other", None); + let manifest = manifest_of(vec![life, other]); + let (resolved, path) = + resolve_target("life:todo/foo.md", None, Some("other"), Some(&manifest)).unwrap(); + assert_eq!(resolved.slug.as_ref().unwrap().to_string(), "life"); + assert_eq!(resolved.path, life_dir.path()); + assert_eq!(path, PathBuf::from("todo/foo.md")); + } + + #[test] + fn resolve_target_qualified_unknown_slug() { + let (_dir, repo) = test_repo("life", None); + let manifest = manifest_of(vec![repo]); + let err = resolve_target("work:todo/foo.md", None, None, Some(&manifest)).unwrap_err(); + assert!(err.to_string().contains("unknown repo slug"), "got: {err}"); + } + + #[test] + fn resolve_target_qualified_no_manifest() { + let err = resolve_target("life:todo/foo.md", None, None, None).unwrap_err(); + assert!(err.to_string().contains("no manifest found"), "got: {err}"); } #[test] fn resolve_target_bare_path_uses_default_repo() { - // With a bare path, repo_arg=None, and default_repo=Some("nonexistent"), - // resolve_target should attempt to resolve "nonexistent" as a slug. - // It will fail (no manifest), but the error should reference the slug — - // proving the default_repo was used rather than falling through to cwd. - let err = resolve_target("todo/foo.md", None, Some("nonexistent")).unwrap_err(); - let msg = err.to_string(); - // resolve_single_repo tries Slug::new("nonexistent") which succeeds, - // then looks up in manifest — the error proves default_repo was passed through. + // default_repo=Some("nonexistent") with a manifest lacking it → the slug + // lookup fails, proving default_repo was used (not a cwd fall-through). + let (_dir, repo) = test_repo("life", None); + let manifest = manifest_of(vec![repo]); + let err = + resolve_target("todo/foo.md", None, Some("nonexistent"), Some(&manifest)).unwrap_err(); + assert!(err.to_string().contains("unknown repo slug"), "got: {err}"); + } + + #[test] + fn resolve_target_bare_path_default_repo_positive() { + let (dir, repo) = test_repo("life", None); + let manifest = manifest_of(vec![repo]); + let (resolved, path) = + resolve_target("todo/foo.md", None, Some("life"), Some(&manifest)).unwrap(); + assert_eq!(resolved.slug.as_ref().unwrap().to_string(), "life"); + assert_eq!(resolved.path, dir.path()); + assert_eq!(path, PathBuf::from("todo/foo.md")); + } + + #[test] + fn resolve_target_bare_path_repo_arg_takes_precedence_over_default() { + // With both repo_arg and default_repo set, repo_arg wins: the resolved + // repo is `explicit`, not `default`. + let (explicit_dir, explicit) = test_repo("explicit", None); + let (_default_dir, default) = test_repo("default", None); + let manifest = manifest_of(vec![explicit, default]); + let (resolved, _path) = resolve_target( + "todo/foo.md", + Some("explicit"), + Some("default"), + Some(&manifest), + ) + .unwrap(); + assert_eq!(resolved.slug.as_ref().unwrap().to_string(), "explicit"); + assert_eq!(resolved.path, explicit_dir.path()); + } + + #[test] + fn resolve_repos_no_arg_returns_all_in_order() { + let (_a_dir, a) = test_repo("alpha", None); + let (_b_dir, b) = test_repo("beta", None); + let manifest = manifest_of(vec![a, b]); + let repos = resolve_repos(None, Some(&manifest)).unwrap(); + let slugs: Vec = repos + .iter() + .map(|r| r.slug.as_ref().unwrap().to_string()) + .collect(); + assert_eq!(slugs, ["alpha", "beta"]); + } + + #[test] + fn resolve_repos_slug_plumbs_domain_from_config() { + let (_dir, repo) = test_repo("life", Some("example.com/household")); + let manifest = manifest_of(vec![repo]); + let repos = resolve_repos(Some("life"), Some(&manifest)).unwrap(); + assert_eq!(repos.len(), 1); + assert_eq!(repos[0].slug.as_ref().unwrap().to_string(), "life"); + assert_eq!( + repos[0].domain.as_ref().map(|d| d.to_string()), + Some("example.com/household".to_string()) + ); + } + + #[test] + fn resolve_repos_slug_no_manifest() { + let err = resolve_repos(Some("life"), None).unwrap_err(); + assert!(err.to_string().contains("no manifest found"), "got: {err}"); + } + + #[test] + fn resolve_repos_no_arg_no_manifest_falls_back_to_cwd() { + let repos = resolve_repos(None, None).unwrap(); + assert_eq!(repos.len(), 1); + assert_eq!(repos[0].path, std::env::current_dir().unwrap()); + } + + #[test] + fn resolve_repos_no_arg_empty_manifest_falls_back_to_cwd() { + // A present-but-empty manifest with no --repo falls to cwd, same as no + // manifest at all. + let manifest = manifest_of(vec![]); + let repos = resolve_repos(None, Some(&manifest)).unwrap(); + assert_eq!(repos.len(), 1); + assert_eq!(repos[0].path, std::env::current_dir().unwrap()); + } + + #[test] + fn resolve_single_repo_multiple_repos_errors() { + let (_a_dir, a) = test_repo("alpha", None); + let (_b_dir, b) = test_repo("beta", None); + let manifest = manifest_of(vec![a, b]); + let err = resolve_single_repo(None, Some(&manifest)).unwrap_err(); assert!( - msg.contains("nonexistent"), - "error should reference the default_repo slug, got: {msg}" + err.to_string().contains("multiple repos found"), + "got: {err}" ); } #[test] - fn resolve_target_bare_path_repo_arg_takes_precedence_over_default() { - // When both repo_arg and default_repo are set, repo_arg wins for bare paths. - let err = resolve_target("todo/foo.md", Some("explicit"), Some("default")).unwrap_err(); - let msg = err.to_string(); + fn resolve_repos_slug_missing_config_errors() { + // Manifest entry pointing at a dir without `.graf/config.toml`. + let dir = TempDir::new().unwrap(); + let repo = ManifestRepo { + slug: Slug::new("life").unwrap(), + path: dir.path().to_string_lossy().into_owned(), + id: GlobalId::new("example.com/repo").unwrap(), + }; + let manifest = manifest_of(vec![repo]); + let err = resolve_repos(Some("life"), Some(&manifest)).unwrap_err(); assert!( - msg.contains("explicit"), - "error should reference repo_arg not default_repo, got: {msg}" + err.to_string().contains("no .graf/config.toml"), + "got: {err}" ); } } diff --git a/src/cli/reindex.rs b/src/cli/reindex.rs index f6e56fb..d0971a0 100644 --- a/src/cli/reindex.rs +++ b/src/cli/reindex.rs @@ -3,6 +3,7 @@ use std::io::IsTerminal; use anyhow::Result; use clap::Args; +use graf::manifest::AppManifest; use graf::todo; #[derive(Args)] @@ -17,7 +18,8 @@ pub struct ReindexArgs { } pub fn run(args: ReindexArgs) -> Result<()> { - let resolved = super::resolve_single_repo(args.repo.as_deref())?; + let manifest = AppManifest::load_default()?; + let resolved = super::resolve_single_repo(args.repo.as_deref(), manifest.as_ref())?; let result = todo::reindex(&resolved.path)?; let json = args.json || !std::io::stdout().is_terminal(); diff --git a/src/cli/todo.rs b/src/cli/todo.rs index 3165f8f..e389897 100644 --- a/src/cli/todo.rs +++ b/src/cli/todo.rs @@ -7,6 +7,7 @@ use clap::{Args, Subcommand}; use graf::clock::Clock; use graf::config::AppConfig; use graf::frontmatter::{Effort, Recurrence, Status}; +use graf::manifest::AppManifest; use graf::todo::{self, AddOptions, CancelOptions, DoneOptions, ReorderOptions, ScheduleOptions}; #[derive(Args)] @@ -14,7 +15,8 @@ pub struct TodoArgs { #[command(subcommand)] command: Option, - /// Only include items up to N days in the future + /// Only include active items up to N days in the future and + /// done/cancelled items completed within the last N days #[arg(long)] horizon: Option, @@ -112,7 +114,8 @@ pub fn run(args: TodoArgs) -> Result<()> { } fn run_query(args: TodoArgs) -> Result<()> { - let repos = super::resolve_repos(args.repo.as_deref())?; + let manifest = AppManifest::load_default()?; + let repos = super::resolve_repos(args.repo.as_deref(), manifest.as_ref())?; let statuses = parse_statuses(&args.status)?; let multi_repo = repos.len() > 1; @@ -147,7 +150,9 @@ fn run_query(args: TodoArgs) -> Result<()> { } fn run_add(args: AddArgs, json: bool) -> Result<()> { - let (resolved, path) = super::resolve_target(&args.path, args.repo.as_deref(), None)?; + let manifest = AppManifest::load_default()?; + let (resolved, path) = + super::resolve_target(&args.path, args.repo.as_deref(), None, manifest.as_ref())?; let recurrence = match (args.rrule.as_deref(), args.lead_days) { (Some(rrule), lead_days) => Some(Recurrence { @@ -275,7 +280,9 @@ struct ReorderArgs { } fn run_done(args: DoneArgs, json: bool) -> Result<()> { - let (resolved, path) = super::resolve_target(&args.path, args.repo.as_deref(), None)?; + let manifest = AppManifest::load_default()?; + let (resolved, path) = + super::resolve_target(&args.path, args.repo.as_deref(), None, manifest.as_ref())?; let call_result = todo::todo_done(&DoneOptions { repo_dir: resolved.path, path, @@ -364,7 +371,9 @@ fn run_done(args: DoneArgs, json: bool) -> Result<()> { } fn run_cancel(args: CancelArgs, json: bool) -> Result<()> { - let (resolved, path) = super::resolve_target(&args.path, args.repo.as_deref(), None)?; + let manifest = AppManifest::load_default()?; + let (resolved, path) = + super::resolve_target(&args.path, args.repo.as_deref(), None, manifest.as_ref())?; let result = todo::todo_cancel(&CancelOptions { repo_dir: resolved.path, path, @@ -389,7 +398,9 @@ fn run_cancel(args: CancelArgs, json: bool) -> Result<()> { } fn run_schedule(args: ScheduleArgs, json: bool) -> Result<()> { - let (resolved, path) = super::resolve_target(&args.path, args.repo.as_deref(), None)?; + let manifest = AppManifest::load_default()?; + let (resolved, path) = + super::resolve_target(&args.path, args.repo.as_deref(), None, manifest.as_ref())?; let call_result = todo::todo_schedule(&ScheduleOptions { repo_dir: resolved.path, path, @@ -430,8 +441,9 @@ fn run_schedule(args: ScheduleArgs, json: bool) -> Result<()> { } fn run_reorder(args: ReorderArgs, json: bool) -> Result<()> { - let (resolved, path) = super::resolve_target(&args.path, args.repo.as_deref(), None)?; - let manifest = graf::manifest::AppManifest::load_default()?; + let manifest = AppManifest::load_default()?; + let (resolved, path) = + super::resolve_target(&args.path, args.repo.as_deref(), None, manifest.as_ref())?; let target_sync = graf::db::open_and_sync(&resolved.path)?; let after = args diff --git a/src/config.rs b/src/config.rs index 464168b..1d92cbb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -5,19 +5,43 @@ use serde::Deserialize; const DEFAULT_TODO_LIMIT: usize = 100; +/// Resolve graf's config directory: `$XDG_CONFIG_HOME/graf`, falling back +/// to `$HOME/.config/graf`. Callers append their own filename. +pub fn graf_config_dir() -> Result { + graf_config_dir_from( + std::env::var("XDG_CONFIG_HOME").ok(), + std::env::var("HOME").ok(), + ) +} + +/// Pure core of [`graf_config_dir`]: `None` means the variable is unset. +/// Separated so unit tests need no env mutation. +fn graf_config_dir_from(xdg_config_home: Option, home: Option) -> Result { + let root = match xdg_config_home { + // TODO(xdg-empty-unset): an empty XDG_CONFIG_HOME is used verbatim; the + // XDG spec says empty means unset (fall back to $HOME/.config). + Some(dir) => PathBuf::from(dir), + None => { + let home = home.context("HOME environment variable not set")?; + PathBuf::from(home).join(".config") + } + }; + Ok(root.join("graf")) +} + /// Application-level config, loaded from `~/.config/graf/config.toml`. /// /// Unlike `AppManifest` (identity/location registry), this holds user /// preferences and behavioral defaults. Always returns a value — uses /// built-in defaults when no file exists. -#[derive(Debug, Default, Deserialize)] +#[derive(Debug, Clone, Default, Deserialize)] #[serde(default)] pub struct AppConfig { pub todo: TodoConfig, } /// Todo-related config. -#[derive(Debug, Deserialize)] +#[derive(Debug, Clone, Deserialize)] #[serde(default)] pub struct TodoConfig { /// Default limit for todo queries when no explicit `--limit` is given. @@ -48,19 +72,12 @@ impl AppConfig { /// Determine the config file path. /// /// Uses `GRAF_CONFIG` env var if set, otherwise `~/.config/graf/config.toml`. - // TODO(xdg-path-duplication) pub fn default_path() -> Result { if let Ok(p) = std::env::var("GRAF_CONFIG") { return Ok(PathBuf::from(p)); } - let config_dir = match std::env::var("XDG_CONFIG_HOME") { - Ok(dir) => PathBuf::from(dir), - Err(_) => { - let home = std::env::var("HOME").context("HOME environment variable not set")?; - PathBuf::from(home).join(".config") - } - }; - Ok(config_dir.join("graf/config.toml")) + let config_dir = graf_config_dir()?; + Ok(config_dir.join("config.toml")) } /// Load from the default path. Returns defaults if the file doesn't exist. @@ -127,4 +144,28 @@ mod tests { let config: AppConfig = toml::from_str("[todo]\n").unwrap(); assert_eq!(config.todo.default_limit, Some(100)); } + + #[test] + fn config_dir_uses_xdg_when_set() { + let dir = graf_config_dir_from(Some("/xdg/dir".to_string()), None).unwrap(); + assert_eq!(dir, PathBuf::from("/xdg/dir/graf")); + } + + #[test] + fn config_dir_falls_back_to_home_dot_config() { + let dir = graf_config_dir_from(None, Some("/home/u".to_string())).unwrap(); + assert_eq!(dir, PathBuf::from("/home/u/.config/graf")); + } + + #[test] + fn config_dir_errors_without_home() { + let err = graf_config_dir_from(None, None).unwrap_err(); + assert!(format!("{err:#}").contains("HOME")); + } + + #[test] + fn config_dir_honors_empty_xdg_verbatim() { + let dir = graf_config_dir_from(Some(String::new()), Some("/home/u".to_string())).unwrap(); + assert_eq!(dir, PathBuf::from("graf")); + } } diff --git a/src/db.rs b/src/db.rs index f43f241..4f918dd 100644 --- a/src/db.rs +++ b/src/db.rs @@ -2,7 +2,7 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; -use chrono::NaiveDate; +use chrono::{Datelike, NaiveDate}; use rusqlite::params; use rusqlite::types::ToSql; @@ -16,20 +16,33 @@ use crate::lint; // Schema // --------------------------------------------------------------------------- -const SCHEMA_VERSION: &str = "5"; +/// Bump on any schema change, and also on any change to `lint::check_rules` +/// (rules or message text): lint messages are persisted in `docs.lint_errors` +/// and deduped by exact text, so stale sets must be rebuilt. The +/// unparseable-rrule rule embeds the `rrule` crate's own error text in its +/// message, so a `rrule`-crate upgrade that rewords that error also changes +/// persisted message text and requires a bump, even with `check_rules` +/// untouched. +pub(crate) const SCHEMA_VERSION: &str = "8"; /// The COALESCE expression for effective date. Used in the schema index, /// SELECT, WHERE, and ORDER BY. Must be textually identical everywhere /// for SQLite's expression index to match. /// /// For active items the scheduling dates resolve first (lint requires at -/// least one of check_in_date / due_date). For terminal items those are -/// stripped and on_date/end_date is the fallback. -/// `on_date`/`end_date` must precede `due_date` so that terminal items -/// (whose scheduling dates are stripped) sort by completion date, not -/// their old deadline. +/// least one of check_in_date / due_date). Terminal items normally have +/// their scheduling dates stripped, so on_date/end_date is the fallback; +/// `on_date`/`end_date` precede `due_date` so those items sort by +/// completion date, not their old deadline. A frozen series (terminal, +/// recurrence retained) is the exception: it keeps `check_in_date` frozen +/// at the final occurrence, which therefore wins the COALESCE over its +/// `on_date`. TODO(frozen-series-effective-date) const EFF_DATE: &str = "COALESCE(tentative_date, check_in_date, on_date, end_date, due_date)"; +/// SQL tuple of terminal statuses. Must match Status::is_terminal() over +/// Status::ALL in declaration order — asserted by status_sql_literals_match_enum. +const TERMINAL_STATUSES_SQL: &str = "('done', 'cancelled')"; + const SCHEMA_DDL: &str = " CREATE TABLE IF NOT EXISTS meta ( key TEXT PRIMARY KEY, @@ -53,13 +66,14 @@ CREATE TABLE IF NOT EXISTS docs ( context_link TEXT, rrule TEXT, blocked_by TEXT, + lint_errors TEXT NOT NULL DEFAULT '[]', dirty INTEGER NOT NULL DEFAULT 0 ); CREATE INDEX IF NOT EXISTS idx_active_effective_date ON docs( COALESCE(tentative_date, check_in_date, on_date, end_date, due_date), - COALESCE(sort_order, priority, 6) -- 6 must match frontmatter::UNRANKED -) WHERE status IN ('todo', 'in_progress', 'reminder'); + COALESCE(sort_order, priority, 6) -- must match frontmatter::rank(); 6 = frontmatter::UNRANKED +) WHERE status IN ('todo', 'in_progress', 'reminder'); -- active statuses; asserted by status_sql_literals_match_enum CREATE INDEX IF NOT EXISTS idx_status ON docs(status, on_date, end_date); "; @@ -82,9 +96,6 @@ fn str_to_effort(s: &str) -> Result { // Public types // --------------------------------------------------------------------------- -// TODO(workspace-handle): Consider a higher-level Workspace handle that wraps -// repo_dir + Cache, so callers don't each carry repo_dir in their Options -// structs and independently call open_and_sync. pub struct Cache { conn: rusqlite::Connection, repo_dir: PathBuf, @@ -97,8 +108,11 @@ pub struct SyncResult { } pub struct TaskFilter { - /// Include items with effective_date <= reference_date + horizon_days. - /// None means no date ceiling: every item passes the date filter. + /// Symmetric date window: active items with + /// effective_date <= reference_date + horizon_days, and terminal + /// (done/cancelled) items with effective_date >= reference_date - + /// horizon_days, i.e. completed within the last `horizon_days` days. + /// None means no date filter at all: every item passes. pub horizon: Option, /// Statuses to include. Default: \[Todo, InProgress, Reminder\]. /// When `include_done` is true, Done and Cancelled are appended. @@ -166,16 +180,21 @@ pub fn db_path(repo_dir: &Path) -> PathBuf { /// handle ready for queries. pub fn open_and_sync(repo_dir: &Path) -> Result { let mut cache = open_db(repo_dir)?; - let is_new = cache - .conn - .query_row( - "SELECT 1 FROM meta WHERE key = 'last_indexed_commit'", - [], - |_| Ok(()), - ) - .is_err(); - - if is_new || schema_version_mismatch(&cache.conn) { + // A never-synced DB has the `meta` table (open_db just created it) but no + // `last_indexed_commit` row. Any other read failure is a real fault: + // reindexing on it would drop both tables, destroying the evidence and + // reporting the wrong cause. + let is_new = match cache.conn.query_row( + "SELECT 1 FROM meta WHERE key = 'last_indexed_commit'", + [], + |_| Ok(()), + ) { + Ok(()) => false, + Err(rusqlite::Error::QueryReturnedNoRows) => true, + Err(e) => return Err(e).context("failed to read last_indexed_commit from cache DB"), + }; + + if is_new || schema_version_mismatch(&cache.conn)? { let errors = do_reindex(&mut cache)?; return Ok(SyncResult { cache, errors }); } @@ -203,7 +222,7 @@ pub fn open_cache_no_sync(repo_dir: &Path) -> Result { ); } let cache = open_db(repo_dir)?; - if schema_version_mismatch(&cache.conn) { + if schema_version_mismatch(&cache.conn)? { bail!( "cache DB at {} has stale schema — query this repo to rebuild it", path.display() @@ -267,11 +286,11 @@ impl Cache { // Status filter let mut statuses: Vec = filter.statuses.iter().map(|s| s.to_string()).collect(); if filter.include_done { - if !statuses.iter().any(|s| s == "done") { - statuses.push("done".to_string()); - } - if !statuses.iter().any(|s| s == "cancelled") { - statuses.push("cancelled".to_string()); + for s in Status::ALL.iter().filter(|s| s.is_terminal()) { + let s = s.to_string(); + if !statuses.contains(&s) { + statuses.push(s); + } } } if statuses.is_empty() { @@ -289,24 +308,46 @@ impl Cache { qb.push_str(")"); } - // Date filter: when a horizon is set, only include active items with an - // effective date within the window. Active items without a date are - // excluded (they are invalid per lint rules but may exist in unchecked - // data). Terminal items (done/cancelled) are exempt from the horizon - // filter — they sort by on_date/end_date, not scheduling dates. - // TODO(done-horizon): this returns ALL terminal items regardless of age. + // Date filter: a horizon is a symmetric window around the reference + // date. Active items pass with an effective date at or before + // reference + days; active items without a date are excluded (they are + // invalid per lint rules but may exist in unchecked data). Terminal + // items (done/cancelled) pass with an effective date at or after + // reference - days: their scheduling dates are stripped on completion, + // so the effective date is the completion date. The terminal branch + // also admits a NULL effective date: such a row is a lint violation, + // and sync does not re-lint unchanged files, so letting it through is + // what surfaces it as a lint error rather than silently dropping it. if let Some(ref horizon) = filter.horizon { - let cutoff = horizon + let upper = horizon .reference_date .checked_add_signed(chrono::Duration::days(i64::from(horizon.days))) .context("date overflow computing horizon cutoff")?; - let cutoff_str = cutoff.to_string(); + let lower = horizon + .reference_date + .checked_sub_signed(chrono::Duration::days(i64::from(horizon.days))) + .context("date underflow computing terminal horizon lower bound")?; + + // Both bounds are compared against stored dates as text, so they + // must render in the same zero-padded four-digit-year form. + // Outside years 0000-9999 chrono emits a sign-prefixed extended + // year, which breaks the byte-wise ordering the predicate relies + // on; reject such a horizon rather than return wrong rows. + if upper.year() > 9999 || lower.year() < 0 { + bail!( + "horizon of {} days is out of range: window {lower}..{upper} \ + falls outside years 0000-9999", + horizon.days + ); + } - let p = qb.push_param(cutoff_str); - // TODO(sql-terminal-status-strings) + let p_upper = qb.push_param(upper.to_string()); + let p_lower = qb.push_param(lower.to_string()); qb.push_str(&format!( - " AND (status IN ('done', 'cancelled') \ - OR ({EFF_DATE} IS NOT NULL AND {EFF_DATE} <= {p}))" + " AND ((status IN {TERMINAL_STATUSES_SQL} \ + AND ({EFF_DATE} IS NULL OR {EFF_DATE} >= {p_lower})) \ + OR (status NOT IN {TERMINAL_STATUSES_SQL} \ + AND {EFF_DATE} IS NOT NULL AND {EFF_DATE} <= {p_upper}))" )); } @@ -318,11 +359,12 @@ impl Cache { )); } - // TODO(sort-key-duplication): this must match todo_sort_cmp() in cli/mod.rs - // TODO(sql-terminal-status-strings) + // This must match todo_sort_cmp() in cli/mod.rs; the rank term is the + // canonical formula frontmatter::rank(). Guarded by the multi-repo sort + // integration tests. // ORDER BY: active items first, then by effective date, rank, title. qb.push_str(&format!( - " ORDER BY status IN ('done', 'cancelled'), \ + " ORDER BY status IN {TERMINAL_STATUSES_SQL}, \ {EFF_DATE} IS NULL, {EFF_DATE}, \ COALESCE(sort_order, priority, {unranked}), tldr", unranked = frontmatter::UNRANKED as i64 @@ -346,6 +388,26 @@ impl Cache { Ok(tasks) } + /// All persisted lint errors for every cached doc, repo-wide and + /// independent of any task filter. A separate scan rather than a + /// `TaskRow` field: `query_tasks` filters by status and horizon, so + /// reading lint state off returned rows alone would keep terminal-item + /// and horizon-excluded violations invisible. + pub fn all_lint_errors(&self) -> Result> { + let mut stmt = self.conn.prepare( + "SELECT path, lint_errors FROM docs WHERE lint_errors != '[]' ORDER BY path", + )?; + let mut rows = stmt.query([])?; + let mut errors = Vec::new(); + while let Some(row) = rows.next()? { + let path = PathBuf::from(row.get::<_, String>(0)?); + for message in parse_json_vec(row.get::<_, Option>(1)?)? { + errors.push(FileError::new(&path, message)); + } + } + Ok(errors) + } + /// Look up a single document by path. Returns None if not in the cache. pub fn get_doc(&self, rel_path: &Path) -> Result> { let path_str = rel_path.to_str().context("path is not UTF-8")?; @@ -450,20 +512,26 @@ impl QueryBuilder { // Internal: schema version check // --------------------------------------------------------------------------- -/// Returns true if the DB has a schema version that doesn't match the current one. -fn schema_version_mismatch(conn: &rusqlite::Connection) -> bool { - let version: Option = conn - .query_row( - "SELECT value FROM meta WHERE key = 'schema_version'", - [], - |row| row.get(0), - ) - .ok(); - - match version.as_deref() { - None | Some(SCHEMA_VERSION) => false, - Some(_) => true, - } +/// Returns true if the DB has a schema version that doesn't match the current +/// one. A missing row counts as a mismatch: pre-versioning caches predate the +/// current column set and must be rebuilt rather than queried. Genuinely new +/// DBs are caught by the `is_new` check before this runs. +/// +/// Any other SQLite failure (corruption, I/O) propagates rather than being +/// read as staleness — a reindex would destroy the evidence and report the +/// wrong cause. +fn schema_version_mismatch(conn: &rusqlite::Connection) -> Result { + let version: Option = match conn.query_row( + "SELECT value FROM meta WHERE key = 'schema_version'", + [], + |row| row.get(0), + ) { + Ok(v) => Some(v), + Err(rusqlite::Error::QueryReturnedNoRows) => None, + Err(e) => return Err(e).context("failed to read schema_version from cache DB"), + }; + + Ok(version.as_deref() != Some(SCHEMA_VERSION)) } // --------------------------------------------------------------------------- @@ -642,6 +710,7 @@ fn reconcile_dirty( errors.append(&mut file_errors); } Err(e) => { + // TODO(persist-parse-errors) tx.execute("DELETE FROM docs WHERE path = ?1", params![path_str])?; errors.push(FileError::new(&path, format!("{e:#}"))); } @@ -679,6 +748,7 @@ fn apply_changed_files( errors.append(&mut file_errors); } Err(e) => { + // TODO(persist-parse-errors) let path_str = cf.path.to_str().context("path is not UTF-8")?; tx.execute("DELETE FROM docs WHERE path = ?1", params![path_str])?; errors.push(FileError::new(&cf.path, format!("{e:#}"))); @@ -746,21 +816,32 @@ fn parse_validate_and_migrate( /// Upsert a doc's frontmatter into the cache. Works on both `Connection` /// and `Transaction` (which derefs to `Connection`). Only caches the subset /// of frontmatter fields relevant to scheduling, filtering, and sorting. +/// +/// Lint state is computed here rather than passed in, so that stored lint +/// messages always match the stored frontmatter no matter which call site +/// wrote the row — the mutation paths in `todo.rs` have no lint plumbing of +/// their own and would otherwise clear the state. The sync call sites run +/// `check_rules` a second time as a result; it is a pure in-memory check +/// (field comparisons plus an rrule string parse for docs carrying one). fn execute_upsert( conn: &rusqlite::Connection, rel_path: &Path, fm: &Frontmatter, dirty: bool, ) -> Result<()> { + let lint_messages: Vec = lint::check_rules(rel_path, fm) + .into_iter() + .map(|e| e.message) + .collect(); conn.execute( "INSERT INTO docs ( path, tldr, status, priority, effort, tentative_date, check_in_date, due_date, on_date, end_date, sort_order, labels, assigned_to, context_link, rrule, - blocked_by, dirty + blocked_by, lint_errors, dirty ) VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, - ?16, ?17 + ?16, ?17, ?18 ) ON CONFLICT(path) DO UPDATE SET tldr=excluded.tldr, status=excluded.status, priority=excluded.priority, effort=excluded.effort, @@ -773,6 +854,7 @@ fn execute_upsert( assigned_to=excluded.assigned_to, context_link=excluded.context_link, rrule=excluded.rrule, blocked_by=excluded.blocked_by, + lint_errors=excluded.lint_errors, dirty=excluded.dirty", params![ rel_path.to_str().context("path is not UTF-8")?, @@ -791,6 +873,7 @@ fn execute_upsert( fm.context_link.as_deref(), fm.effective_rrule(), serde_json::to_string(&fm.blocked_by)?, + serde_json::to_string(&lint_messages)?, dirty, ], )?; @@ -808,6 +891,38 @@ mod tests { use std::process::Command; use tempfile::TempDir; + #[test] + fn status_sql_literals_match_enum() { + // Derive the terminal and active tuples from the enum, preserving + // Status::ALL declaration order. + let render = |statuses: &[String]| format!("('{}')", statuses.join("', '")); + let terminal: Vec = Status::ALL + .iter() + .filter(|s| s.is_terminal()) + .map(|s| s.to_string()) + .collect(); + let active: Vec = Status::ALL + .iter() + .filter(|s| !s.is_terminal()) + .map(|s| s.to_string()) + .collect(); + let derived_terminal = render(&terminal); + let derived_active = render(&active); + + // Covers the two runtime format! call sites that consume the constant. + assert_eq!(TERMINAL_STATUSES_SQL, derived_terminal); + // Covers the schema partial-index predicate. + assert!( + SCHEMA_DDL.contains(&format!("WHERE status IN {derived_active}")), + "SCHEMA_DDL active-status predicate does not match enum-derived {derived_active}" + ); + + // SQL-quoting safety of the generated tuples. + for s in Status::ALL.iter().map(|s| s.to_string()) { + assert!(!s.contains('\''), "status {s:?} contains a single quote"); + } + } + // -- Test helpers -- fn run_git(dir: &Path, args: &[&str]) { @@ -1239,8 +1354,25 @@ mod tests { assert_eq!(tasks3.len(), 4); // past + today + future + unscheduled } + /// Query with a horizon and the default active statuses plus done/cancelled. + fn horizon_filter(reference: (i32, u32, u32), days: u32) -> TaskFilter { + TaskFilter { + horizon: Some(HorizonFilter { + reference_date: NaiveDate::from_ymd_opt(reference.0, reference.1, reference.2) + .unwrap(), + days, + }), + include_done: true, + ..TaskFilter::default() + } + } + + fn tldrs(tasks: &[TaskRow]) -> Vec<&str> { + tasks.iter().map(|t| t.tldr.as_str()).collect() + } + #[test] - fn test_horizon_exempts_terminal_items() { + fn test_horizon_bounds_terminal_items() { let dir = create_test_repo(); // Active item within horizon write_md( @@ -1254,39 +1386,42 @@ mod tests { "future.md", "tldr: Future\nstatus: todo\ntentative_date: 2026-12-01", ); - // Done item — scheduling dates stripped, on_date set. - // Without the horizon exemption this would be filtered out because - // its EFF_DATE (scheduling COALESCE) is NULL. + // Done item — scheduling dates stripped, on_date set — completed + // inside the lookback window. write_md( dir.path(), "done.md", "tldr: Done\nstatus: done\non_date: 2026-04-10", ); - // Cancelled item — same situation. + // Cancelled item — terminated long before the lookback window. write_md( dir.path(), "cancelled.md", "tldr: Cancelled\nstatus: cancelled\non_date: 2026-03-01", ); + // Cancelled item inside the lookback window: the terminal branch + // admits both terminal statuses, not just done. + write_md( + dir.path(), + "cancelled_recent.md", + "tldr: CancelledRecent\nstatus: cancelled\non_date: 2026-04-09", + ); + // Anomalous future-dated completion: the terminal branch has a lower + // bound only, so it stays visible rather than being silently dropped. + write_md( + dir.path(), + "future_done.md", + "tldr: FutureDone\nstatus: done\non_date: 2026-05-20", + ); commit_all(dir.path()); let result = open_and_sync(dir.path()).unwrap(); - // Horizon = today only: active items within window + all terminal items - let filter = TaskFilter { - horizon: Some(HorizonFilter { - reference_date: NaiveDate::from_ymd_opt(2026, 4, 11).unwrap(), - days: 0, - }), - statuses: vec![Status::Todo, Status::InProgress, Status::Reminder], - include_done: true, - labels: Vec::new(), - limit: None, - }; + // Window 2026-04-06 .. 2026-04-16 + let filter = horizon_filter((2026, 4, 11), 5); let tasks = result.cache.query_tasks(&filter).unwrap(); - let tldrs: Vec<&str> = tasks.iter().map(|t| t.tldr.as_str()).collect(); + let tldrs = tldrs(&tasks); - // Active within horizon + both terminal items (exempt from horizon) assert!( tldrs.contains(&"Active"), "active within horizon: {tldrs:?}" @@ -1297,13 +1432,236 @@ mod tests { ); assert!( tldrs.contains(&"Done"), - "done item exempt from horizon: {tldrs:?}" + "terminal item completed within lookback: {tldrs:?}" ); assert!( - tldrs.contains(&"Cancelled"), - "cancelled item exempt from horizon: {tldrs:?}" + !tldrs.contains(&"Cancelled"), + "terminal item completed before lookback excluded: {tldrs:?}" ); - assert_eq!(tldrs.len(), 3); + assert!( + tldrs.contains(&"CancelledRecent"), + "cancelled item terminated within lookback: {tldrs:?}" + ); + assert!( + tldrs.contains(&"FutureDone"), + "future-dated completion stays visible: {tldrs:?}" + ); + assert_eq!(tldrs.len(), 4); + } + + #[test] + fn test_horizon_extreme_days_errors() { + let dir = create_test_repo(); + write_md( + dir.path(), + "active.md", + "tldr: Active\nstatus: todo\ntentative_date: 2026-04-11", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + // Past the representable date range: the upper bound overflows first. + let err = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), u32::MAX)) + .unwrap_err(); + let rendered = format!("{err:#}"); + assert!( + rendered.contains("date overflow computing horizon cutoff"), + "attributable overflow error: {rendered}" + ); + } + + #[test] + fn test_horizon_out_of_range_year_errors() { + let dir = create_test_repo(); + write_md( + dir.path(), + "active.md", + "tldr: Active\nstatus: todo\ntentative_date: 2026-04-11", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + // Representable, but the upper bound lands past year 9999, where the + // date no longer renders comparably against stored dates. Returning + // zero active items instead of all of them would be a silent wrong + // answer, so this must fail. + let err = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 3_000_000)) + .unwrap_err(); + let rendered = format!("{err:#}"); + assert!( + rendered.contains("out of range"), + "attributable range error: {rendered}" + ); + + // Mirror condition on the lower bound: a lookback past year 0. + let err = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 1_000_000)) + .unwrap_err(); + let rendered = format!("{err:#}"); + assert!( + rendered.contains("out of range"), + "attributable range error: {rendered}" + ); + } + + #[test] + fn test_horizon_terminal_lower_bound_is_inclusive() { + let dir = create_test_repo(); + write_md( + dir.path(), + "at_bound.md", + "tldr: AtBound\nstatus: done\non_date: 2026-04-06", + ); + write_md( + dir.path(), + "before_bound.md", + "tldr: BeforeBound\nstatus: done\non_date: 2026-04-05", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + // Window lower bound = 2026-04-11 - 5 days = 2026-04-06 + let tasks = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 5)) + .unwrap(); + assert_eq!(tldrs(&tasks), vec!["AtBound"]); + } + + #[test] + fn test_horizon_zero_days_terminal_window() { + let dir = create_test_repo(); + write_md( + dir.path(), + "today.md", + "tldr: Today\nstatus: done\non_date: 2026-04-11", + ); + write_md( + dir.path(), + "yesterday.md", + "tldr: Yesterday\nstatus: done\non_date: 2026-04-10", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + // days: 0 — the window collapses to the reference date itself. + let tasks = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 0)) + .unwrap(); + assert_eq!(tldrs(&tasks), vec!["Today"]); + } + + #[test] + fn test_horizon_windows_range_completion_by_end_date() { + let dir = create_test_repo(); + write_md( + dir.path(), + "recent.md", + "tldr: Recent\nstatus: done\nstart_date: 2026-04-01\nend_date: 2026-04-09", + ); + write_md( + dir.path(), + "old.md", + "tldr: Old\nstatus: done\nstart_date: 2026-02-01\nend_date: 2026-02-20", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + let tasks = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 5)) + .unwrap(); + assert_eq!(tldrs(&tasks), vec!["Recent"]); + } + + #[test] + fn test_horizon_applies_to_explicit_done_status() { + let dir = create_test_repo(); + write_md( + dir.path(), + "recent.md", + "tldr: Recent\nstatus: done\non_date: 2026-04-10", + ); + write_md( + dir.path(), + "old.md", + "tldr: Old\nstatus: done\non_date: 2026-01-10", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + // Explicit statuses, not include_done: the window still applies. + let filter = TaskFilter { + horizon: Some(HorizonFilter { + reference_date: NaiveDate::from_ymd_opt(2026, 4, 11).unwrap(), + days: 5, + }), + statuses: vec![Status::Done], + ..TaskFilter::default() + }; + let tasks = result.cache.query_tasks(&filter).unwrap(); + assert_eq!(tldrs(&tasks), vec!["Recent"]); + } + + #[test] + fn test_horizon_admits_null_date_terminal_row() { + let dir = create_test_repo(); + // Lint violation: terminal with no completion date. It must still + // pass the SQL filter so the layer above can report it. + write_md(dir.path(), "nodate.md", "tldr: NoDate\nstatus: done"); + write_md( + dir.path(), + "old.md", + "tldr: Old\nstatus: done\non_date: 2026-01-10", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + let tasks = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 5)) + .unwrap(); + assert_eq!(tldrs(&tasks), vec!["NoDate"]); + assert!(tasks[0].effective_date.is_none()); + } + + #[test] + fn test_horizon_windows_terminal_item_by_tentative_date() { + let dir = create_test_repo(); + // Lint tolerates a retained tentative_date on a terminal item; it wins + // the effective-date COALESCE, so it is what the window applies to. + write_md( + dir.path(), + "old_completion.md", + "tldr: OldCompletion\nstatus: done\non_date: 2026-01-10\ntentative_date: 2026-04-09", + ); + write_md( + dir.path(), + "recent_completion.md", + "tldr: RecentCompletion\nstatus: done\non_date: 2026-04-10\ntentative_date: 2026-01-09", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + + let tasks = result + .cache + .query_tasks(&horizon_filter((2026, 4, 11), 5)) + .unwrap(); + assert_eq!(tldrs(&tasks), vec!["OldCompletion"]); } #[test] @@ -1866,6 +2224,48 @@ did it early assert_eq!(row.rrule.as_deref(), Some("FREQ=WEEKLY")); } + #[test] + fn test_reindex_migrates_broken_flat_rrule_and_reports_it() { + // Migration is shape-only, so a legacy file with an unparseable rrule + // still migrates to the nested shape. The post-rewrite re-lint then + // reports the bad rule, which is where the user finds out. + let dir = create_test_repo(); + let file = "todo/recurring.md"; + let content = r#"--- +tldr: Legacy Broken Recurring +status: todo +check_in_date: 2026-04-18 +rrule: "FREQ=BOGUS" +--- + +Body text. +"#; + let abs = dir.path().join(file); + fs::create_dir_all(abs.parent().unwrap()).unwrap(); + fs::write(&abs, content).unwrap(); + commit_all(dir.path()); + + let result = reindex(dir.path()).unwrap(); + + // The doc still indexes — lint errors are data, not aborts. + assert_eq!(count_docs(&result.cache), 1); + + let rewritten = fs::read_to_string(&abs).unwrap(); + assert!( + rewritten.contains("recurrence:"), + "no recurrence: block:\n{rewritten}" + ); + assert!(!rewritten.contains("\nrrule:")); + + assert!( + result.errors.iter().any( + |e| e.message.contains("unparseable rrule") && e.message.contains("FREQ=BOGUS") + ), + "errors: {:?}", + result.errors + ); + } + #[test] fn test_reindex_noop_on_new_shape_file() { let dir = create_test_repo(); @@ -1970,4 +2370,299 @@ Body. assert!(!after.contains("\nrrule:")); assert!(!after.contains("completion_log:")); } + + // -- Persisted lint state -- + + /// Raw `lint_errors` column for a path, as stored. + fn lint_errors_col(cache: &Cache, path: &str) -> String { + cache + .conn + .query_row( + "SELECT lint_errors FROM docs WHERE path = ?1", + params![path], + |row| row.get(0), + ) + .unwrap() + } + + fn parse_fm(yaml: &str) -> Frontmatter { + frontmatter::parse_frontmatter(&format!("---\n{yaml}\n---\n")) + .unwrap() + .unwrap() + } + + #[test] + fn test_lint_errors_persist_across_quiescent_sync() { + let dir = create_test_repo(); + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + + // First sync: reindex lints the file and reports it. + let first = open_and_sync(dir.path()).unwrap(); + assert!( + first + .errors + .iter() + .any(|e| e.message.contains("check_in_date")) + ); + drop(first); + + // Second sync: HEAD unchanged, clean tree — sync re-lints nothing, + // but the persisted state still carries the violation. + let second = open_and_sync(dir.path()).unwrap(); + assert!( + second.errors.is_empty(), + "quiescent sync should re-lint nothing: {:?}", + second.errors + ); + let persisted = second.cache.all_lint_errors().unwrap(); + assert_eq!(persisted.len(), 1); + assert_eq!(persisted[0].path, PathBuf::from("bad.md")); + assert!(persisted[0].message.contains("check_in_date")); + } + + #[test] + fn test_lint_errors_cleared_when_file_fixed() { + let dir = create_test_repo(); + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + drop(open_and_sync(dir.path()).unwrap()); + + // Drop the offending field and commit the fix. + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\non_date: 2026-04-10", + ); + commit_all(dir.path()); + + let result = open_and_sync(dir.path()).unwrap(); + assert!(result.errors.is_empty()); + assert!(result.cache.all_lint_errors().unwrap().is_empty()); + assert_eq!(lint_errors_col(&result.cache, "bad.md"), "[]"); + } + + #[test] + fn test_upsert_doc_refreshes_lint_state() { + let dir = create_test_repo(); + write_md(dir.path(), "seed.md", "tldr: Seed"); + commit_all(dir.path()); + let cache = open_and_sync(dir.path()).unwrap().cache; + + let path = Path::new("task.md"); + let bad = + parse_fm("tldr: Task\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10"); + cache.upsert_doc(path, &bad, false).unwrap(); + let errors = cache.all_lint_errors().unwrap(); + assert_eq!(errors.len(), 1); + assert!(errors[0].message.contains("check_in_date")); + + // Upserting a clean version over it clears the stored state. + let good = parse_fm("tldr: Task\nstatus: done\non_date: 2026-04-10"); + cache.upsert_doc(path, &good, false).unwrap(); + assert!(cache.all_lint_errors().unwrap().is_empty()); + assert_eq!(lint_errors_col(&cache, "task.md"), "[]"); + } + + #[test] + fn test_all_lint_errors_fails_on_corrupt_json() { + let dir = create_test_repo(); + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + let cache = open_and_sync(dir.path()).unwrap().cache; + + cache + .conn + .execute( + "UPDATE docs SET lint_errors = '{not json' WHERE path = 'bad.md'", + [], + ) + .unwrap(); + + let err = cache.all_lint_errors().unwrap_err(); + let chain = format!("{err:#}"); + assert!( + chain.contains("corrupt JSON array in cache DB"), + "expected fail-fast on corrupt lint_errors, got: {chain}" + ); + } + + #[test] + fn test_schema_version_query_error_propagates() { + let dir = create_test_repo(); + write_md(dir.path(), "a.md", "tldr: A"); + commit_all(dir.path()); + let cache = open_and_sync(dir.path()).unwrap().cache; + + // A BLOB survives the column's TEXT affinity, so reading it as a + // String fails — a stand-in for a corrupt/unreadable meta table. + cache + .conn + .execute( + "INSERT OR REPLACE INTO meta (key, value) VALUES ('schema_version', x'00ff')", + [], + ) + .unwrap(); + + let err = schema_version_mismatch(&cache.conn).unwrap_err(); + assert!( + format!("{err:#}").contains("failed to read schema_version"), + "expected the DB error to propagate, got: {err:#}" + ); + } + + /// A failed `last_indexed_commit` probe must not read as "fresh DB": the + /// resulting reindex would drop both tables and hide the real fault. + #[test] + fn test_last_indexed_commit_query_error_propagates() { + let dir = create_test_repo(); + write_md(dir.path(), "a.md", "tldr: A"); + commit_all(dir.path()); + { + let cache = open_and_sync(dir.path()).unwrap().cache; + // Replace `meta` with a table of the same name but the wrong + // shape: `CREATE TABLE IF NOT EXISTS` leaves it alone and the + // probe fails with "no such column: key" — a fault, not a + // never-synced DB. + cache + .conn + .execute_batch( + "DROP TABLE meta; CREATE TABLE meta (wrong_column TEXT PRIMARY KEY);", + ) + .unwrap(); + } + + let Err(err) = open_and_sync(dir.path()) else { + panic!("expected the probe failure to propagate, got a successful sync"); + }; + assert!( + format!("{err:#}").contains("failed to read last_indexed_commit"), + "expected the DB error to propagate, got: {err:#}" + ); + } + + #[test] + fn test_all_lint_errors_expands_multiple_messages() { + let dir = create_test_repo(); + write_md(dir.path(), "seed.md", "tldr: Seed"); + commit_all(dir.path()); + let cache = open_and_sync(dir.path()).unwrap().cache; + + // Trips both the terminal-check_in_date rule and the terminal-rrule rule. + let fm = parse_fm( + "tldr: Task\nstatus: done\non_date: 2026-04-10\ncheck_in_date: 2026-05-01\nrecurrence:\n rrule: \"FREQ=WEEKLY\"", + ); + cache.upsert_doc(Path::new("task.md"), &fm, false).unwrap(); + + let errors = cache.all_lint_errors().unwrap(); + assert_eq!(errors.len(), 2, "got: {errors:?}"); + assert!(errors.iter().all(|e| e.path == PathBuf::from("task.md"))); + assert!(errors.iter().any(|e| e.message.contains("check_in_date"))); + assert!(errors.iter().any(|e| e.message.contains("rrule"))); + } + + #[test] + fn test_unparseable_file_drops_persisted_lint_state() { + let dir = create_test_repo(); + write_md( + dir.path(), + "task.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + + let first = open_and_sync(dir.path()).unwrap(); + assert_eq!(first.cache.all_lint_errors().unwrap().len(), 1); + drop(first); + + // Break the file (uncommitted): the row goes, and its lint state with it. + fs::write(dir.path().join("task.md"), "---\nbogus_field: oops\n---\n").unwrap(); + + let second = open_and_sync(dir.path()).unwrap(); + assert_eq!(count_docs(&second.cache), 0); + assert!(second.cache.all_lint_errors().unwrap().is_empty()); + // The parse failure itself is still reported for this pass. + assert_eq!(second.errors.len(), 1); + assert!(second.errors[0].path.to_str().unwrap().contains("task.md")); + } + + #[test] + fn test_stale_schema_version_reindex_populates_lint_state() { + let dir = create_test_repo(); + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + drop(open_and_sync(dir.path()).unwrap()); + + let conn = rusqlite::Connection::open(db_path(dir.path())).unwrap(); + conn.execute( + "UPDATE meta SET value = '99' WHERE key = 'schema_version'", + [], + ) + .unwrap(); + drop(conn); + + let result = open_and_sync(dir.path()).unwrap(); + assert_eq!(count_docs(&result.cache), 1); + let errors = result.cache.all_lint_errors().unwrap(); + assert_eq!(errors.len(), 1); + assert!(errors[0].message.contains("check_in_date")); + } + + #[test] + fn test_missing_schema_version_row_triggers_reindex() { + let dir = create_test_repo(); + write_md( + dir.path(), + "bad.md", + "tldr: Bad\nstatus: done\ncheck_in_date: 2026-05-01\non_date: 2026-04-10", + ); + commit_all(dir.path()); + drop(open_and_sync(dir.path()).unwrap()); + + // A pre-versioning cache: has last_indexed_commit but no schema_version. + let conn = rusqlite::Connection::open(db_path(dir.path())).unwrap(); + conn.execute("DELETE FROM meta WHERE key = 'schema_version'", []) + .unwrap(); + drop(conn); + + let result = open_and_sync(dir.path()).unwrap(); + assert_eq!(count_docs(&result.cache), 1); + let errors = result.cache.all_lint_errors().unwrap(); + assert_eq!(errors.len(), 1); + assert!(errors[0].message.contains("check_in_date")); + } + + #[test] + fn test_open_cache_no_sync_rejects_missing_schema_version() { + let dir = create_test_repo(); + write_md(dir.path(), "task.md", "tldr: Task"); + commit_all(dir.path()); + drop(open_and_sync(dir.path()).unwrap()); + + let conn = rusqlite::Connection::open(db_path(dir.path())).unwrap(); + conn.execute("DELETE FROM meta WHERE key = 'schema_version'", []) + .unwrap(); + drop(conn); + + let Err(err) = open_cache_no_sync(dir.path()) else { + panic!("expected open_cache_no_sync to reject a pre-versioning cache"); + }; + assert!(err.to_string().contains("stale schema"), "got: {err:#}"); + } } diff --git a/src/frontmatter.rs b/src/frontmatter.rs index 0d5e76f..4e24c7d 100644 --- a/src/frontmatter.rs +++ b/src/frontmatter.rs @@ -31,6 +31,15 @@ impl fmt::Display for Status { } impl Status { + /// All variants, in declaration order. + pub const ALL: [Status; 5] = [ + Status::Todo, + Status::InProgress, + Status::Done, + Status::Cancelled, + Status::Reminder, + ]; + /// Returns true for terminal statuses (Done, Cancelled). pub fn is_terminal(&self) -> bool { matches!(self, Status::Done | Status::Cancelled) @@ -169,6 +178,15 @@ pub struct Frontmatter { /// One past max priority (5), so unranked items sort after all prioritized ones. pub const UNRANKED: f64 = 6.0; +/// Sort rank: `COALESCE(sort_order, priority, UNRANKED)`. +/// The single Rust encoding of the rank formula. The SQL encodings in +/// `db.rs` (ORDER BY and the `idx_active_effective_date` expression index) +/// must agree with this; integration tests in +/// `tests/multi_repo_integration.rs` cross-check the two engines. +pub fn rank(sort_order: Option, priority: Option) -> f64 { + sort_order.or(priority.map(f64::from)).unwrap_or(UNRANKED) +} + impl Frontmatter { /// The effective rrule string, reading the canonical nested /// `recurrence.rrule` first, then falling back to the legacy flat @@ -557,6 +575,33 @@ pub fn parse_file(path: &Path) -> Result> { mod tests { use super::*; + #[test] + fn all_statuses_exhaustive() { + // Exhaustive match: adding a Status variant makes this non-exhaustive, + // a compile error prompting an update to Status::ALL. This is a nudge, + // not a proof of ALL's completeness. + for s in Status::ALL { + match s { + Status::Todo + | Status::InProgress + | Status::Done + | Status::Cancelled + | Status::Reminder => {} + } + } + + // Serializations must be pairwise distinct (duplicate-entry guard). + let serialized: Vec = Status::ALL.iter().map(|s| s.to_string()).collect(); + for i in 0..serialized.len() { + for j in (i + 1)..serialized.len() { + assert_ne!( + serialized[i], serialized[j], + "duplicate status serialization in Status::ALL" + ); + } + } + } + #[test] fn test_parse_basic_frontmatter() { let content = r#"--- @@ -677,16 +722,24 @@ summary: "This is a longer description that provides more context." #[test] fn test_all_status_values() { - for (yaml, expected) in [ - ("todo", Status::Todo), - ("in_progress", Status::InProgress), - ("done", Status::Done), - ("cancelled", Status::Cancelled), - ("reminder", Status::Reminder), - ] { - let content = format!("---\ntldr: test\nstatus: {yaml}\n---\n"); + // Frozen on-disk spellings: the wire format written to markdown files. + // Pinned independently of Display so a coordinated rename across + // Display/FromStr/serde still trips here rather than silently changing + // the format under existing files. + assert_eq!( + Status::ALL.map(|s| s.to_string()), + ["todo", "in_progress", "done", "cancelled", "reminder"] + ); + + for s in Status::ALL { + // Display / FromStr roundtrip. + let rendered = s.to_string(); + assert_eq!(rendered.parse::(), Ok(s.clone())); + // serde (YAML) roundtrip: the DB stores Display strings, so YAML + // parsing must map each Display spelling back to its variant. + let content = format!("---\ntldr: test\nstatus: {rendered}\n---\n"); let fm = parse_frontmatter(&content).unwrap().unwrap(); - assert_eq!(fm.status, Some(expected)); + assert_eq!(fm.status, Some(s)); } } @@ -1477,4 +1530,24 @@ assigned_to: test "Field variants without Frontmatter fields: {extra_in_enum:?}" ); } + + #[test] + fn rank_sort_order_wins_over_priority() { + assert_eq!(rank(Some(2.5), Some(1)), 2.5); + } + + #[test] + fn rank_falls_back_to_priority() { + assert_eq!(rank(None, Some(3)), 3.0); + } + + #[test] + fn rank_falls_back_to_unranked() { + assert_eq!(rank(None, None), UNRANKED); + } + + #[test] + fn rank_fractional_sort_order_below_priority_one() { + assert!(rank(Some(0.5), None) < rank(None, Some(1))); + } } diff --git a/src/identity.rs b/src/identity.rs index 777e50b..b18279c 100644 --- a/src/identity.rs +++ b/src/identity.rs @@ -41,7 +41,7 @@ impl GlobalId { } if domain .chars() - .any(|c| c.is_whitespace() || c == ':' || c == '#') + .any(|c| c.is_control() || c.is_whitespace() || c == ':' || c == '#') { bail!("global ID domain contains invalid characters: {s:?}"); } @@ -55,6 +55,9 @@ impl GlobalId { if path.ends_with('/') { bail!("global ID path must not end with '/': {s:?}"); } + if path.chars().any(|c| c.is_control() || c.is_whitespace()) { + bail!("global ID path contains invalid characters: {s:?}"); + } Ok(()) } @@ -362,6 +365,20 @@ mod tests { assert!(GlobalId::new("example.com/life/").is_err()); } + #[test] + fn global_id_invalid_control_char_in_path() { + assert!(GlobalId::new("example.com/a\x1b[31mred").is_err()); + assert!(GlobalId::new("example.com/a\nb").is_err()); + assert!(GlobalId::new("example.com/a b").is_err()); + assert!(GlobalId::new("example.com/a\tb").is_err()); + } + + #[test] + fn global_id_invalid_control_char_in_domain() { + assert!(GlobalId::new("e\x1b[31mvil.com/x").is_err()); + assert!(GlobalId::new("evil\n.com/x").is_err()); + } + #[test] fn global_id_invalid_uppercase() { assert!(GlobalId::new("Example.com/life").is_err()); diff --git a/src/lint.rs b/src/lint.rs index cc7a87e..21d99b2 100644 --- a/src/lint.rs +++ b/src/lint.rs @@ -1,5 +1,5 @@ use anyhow::{Context, Result}; -use chrono::{DateTime, Utc}; +use chrono::{DateTime, NaiveDate, Utc}; use std::path::{Path, PathBuf}; use std::time::SystemTime; @@ -45,52 +45,108 @@ pub fn lint_result_to_json(result: &LintResult) -> Value { }) } +const MSG_ON_AND_START: &str = + "on_date and start_date cannot both be set (use start_date + end_date for ranges)"; +const MSG_ON_AND_END: &str = + "on_date and end_date cannot both be set (use start_date + end_date for ranges)"; +const MSG_END_NEEDS_START: &str = "end_date requires start_date"; +const MSG_TERMINAL_NO_DATE: &str = "done/cancelled item missing on_date or end_date"; +const MSG_TERMINAL_CHECK_IN: &str = "done/cancelled item has check_in_date set"; +const MSG_TERMINAL_RRULE: &str = + "done/cancelled item has rrule set (remove rrule to stop recurrence)"; +const MSG_ACTIVE_NO_DATE: &str = "active task missing check_in_date or due_date"; + +/// Roster of every *fixed-text* message `check_rules` can emit. Those rules +/// reference the `MSG_*` constants directly and emit through `emit`, which +/// asserts membership here, so an unregistered message fails in every +/// debug/test run regardless of fixture coverage. The message-snapshot test +/// checks its fixtures against this array in the other direction: a message +/// listed here that no fixture trips fails the test rather than passing +/// unnoticed. +/// +/// The unparseable-rrule rule is deliberately outside this scheme: its text +/// embeds the offending rrule string and the parser's error, so it has no +/// fixed form to register or dedupe fixtures against. +const RULE_MESSAGES: [&str; 7] = [ + MSG_ON_AND_START, + MSG_ON_AND_END, + MSG_END_NEEDS_START, + MSG_TERMINAL_NO_DATE, + MSG_TERMINAL_CHECK_IN, + MSG_TERMINAL_RRULE, + MSG_ACTIVE_NO_DATE, +]; + +/// DTSTART the rrule-syntax rule parses against. The rrule crate validates +/// at parse time and that validation is DTSTART-relative: a rule whose `UNTIL` +/// precedes DTSTART is rejected as `UntilBeforeStart`. Lint has no real +/// anchor date, so it anchors at the epoch — otherwise a perfectly valid rule +/// with an `UNTIL` in the past would lint as broken. Pre-1970 `UNTIL` values +/// are not plausible in this system. +fn lint_rrule_dtstart() -> NaiveDate { + NaiveDate::from_ymd_opt(1970, 1, 1).expect("epoch date is valid") +} + +/// Single emit point for lint violations. Every message must be declared in +/// `RULE_MESSAGES`; the assertion makes an undeclared one fail loudly in +/// debug and test builds instead of depending on a fixture happening to trip +/// it. +fn emit(errors: &mut Vec, path: &Path, msg: &'static str) { + debug_assert!( + RULE_MESSAGES.contains(&msg), + "lint message not declared in RULE_MESSAGES: {msg:?}" + ); + errors.push(FileError::new(path, msg)); +} + /// Check lint rules on a parsed Frontmatter. Returns errors (if any) /// without modifying anything. Used by both `lint_md_file` and the /// sync protocol. +/// +/// Any change to these rules or their message text requires bumping +/// `SCHEMA_VERSION` in `db.rs`. Messages are persisted per-doc in the cache +/// and deduped against the current pass by exact text: without a bump, an +/// added rule stays silent for every doc not re-upserted since, and reworded +/// ones double-report. New messages belong in `RULE_MESSAGES`. The +/// unparseable-rrule rule embeds the `rrule` crate's error text, so a +/// `rrule`-crate upgrade that rewords that error is also a message-text change +/// and requires the same bump. +/// +/// Exemption: a *frozen series* — a terminal item carrying a recurrence with a +/// non-empty completion log — keeps its `check_in_date`, `due_date` and +/// recurrence block frozen at the final occurrence, so the two +/// terminal-scheduling rules do not apply to it. Terminal items with a +/// recurrence but no completion history, and terminal items with leftover +/// `check_in_date` and no recurrence, are still flagged. pub fn check_rules(path: &Path, fm: &Frontmatter) -> Vec { let mut errors = Vec::new(); + // A completed series freezes recurrence / check_in_date / due_date at the + // final occurrence. That shape is a valid record, not leftover scheduling + // state. Both conjuncts matter: non-recurring completions also append to + // the log, and a recurrence with an empty log was never completed here. + let frozen_series = fm.effective_rrule().is_some() && !fm.completion_log.is_empty(); + // Date mutual exclusivity rules (apply to all documents, not just tasks) if fm.on_date.is_some() && fm.start_date.is_some() { - errors.push(FileError::new( - path, - "on_date and start_date cannot both be set (use start_date + end_date for ranges)" - .to_string(), - )); + emit(&mut errors, path, MSG_ON_AND_START); } if fm.on_date.is_some() && fm.end_date.is_some() { - errors.push(FileError::new( - path, - "on_date and end_date cannot both be set (use start_date + end_date for ranges)" - .to_string(), - )); + emit(&mut errors, path, MSG_ON_AND_END); } if fm.end_date.is_some() && fm.start_date.is_none() { - errors.push(FileError::new( - path, - "end_date requires start_date".to_string(), - )); + emit(&mut errors, path, MSG_END_NEEDS_START); } if matches!(fm.status, Some(Status::Done | Status::Cancelled)) { if fm.on_date.is_none() && fm.end_date.is_none() { - errors.push(FileError::new( - path, - "done/cancelled item missing on_date or end_date".to_string(), - )); + emit(&mut errors, path, MSG_TERMINAL_NO_DATE); } - if fm.check_in_date.is_some() { - errors.push(FileError::new( - path, - "done/cancelled item has check_in_date set".to_string(), - )); + if fm.check_in_date.is_some() && !frozen_series { + emit(&mut errors, path, MSG_TERMINAL_CHECK_IN); } - if fm.effective_rrule().is_some() { - errors.push(FileError::new( - path, - "done/cancelled item has rrule set (remove rrule to stop recurrence)".to_string(), - )); + if fm.effective_rrule().is_some() && !frozen_series { + emit(&mut errors, path, MSG_TERMINAL_RRULE); } } @@ -102,9 +158,24 @@ pub fn check_rules(path: &Path, fm: &Frontmatter) -> Vec { ) && fm.check_in_date.is_none() && fm.due_date.is_none() { + emit(&mut errors, path, MSG_ACTIVE_NO_DATE); + } + + // Recurrence rules must parse. Checked for every document carrying an + // effective rrule regardless of status: an unparseable rule is corrupt + // data whether or not the item is still active, and a terminal item can + // legitimately collect both this error and MSG_TERMINAL_RRULE. The + // message embeds the offending string, so it carries no `MSG_*` constant + // and does not go through `emit`. + if let Some(rrule) = fm.effective_rrule() + && let Err(e) = crate::todo::parse_rrule_set(rrule, lint_rrule_dtstart()) + { + // `root_cause()` is the parser's own error; the intermediate + // `with_context("parsing rrule: …")` from `parse_rrule_set` repeats the + // rrule string this message already names, so skip the full chain. errors.push(FileError::new( path, - "active task missing check_in_date or due_date".to_string(), + format!("unparseable rrule '{rrule}': {}", e.root_cause()), )); } @@ -325,6 +396,192 @@ mod tests { use std::fs; use tempfile::TempDir; + fn bare_fm() -> Frontmatter { + Frontmatter { + tldr: "test".to_string(), + labels: vec![], + created: None, + updated: None, + summary: None, + on_date: None, + start_date: None, + end_date: None, + due_date: None, + check_in_date: None, + verified_date: None, + context_link: None, + links: vec![], + attachments: vec![], + status: None, + priority: None, + effort: None, + blocked_by: vec![], + rrule: None, + tentative_date: None, + sort_order: None, + assigned_to: None, + recurrence: None, + completion_log: vec![], + } + } + + fn log_entry(day: u32) -> crate::frontmatter::CompletionLogEntry { + crate::frontmatter::CompletionLogEntry { + completed: NaiveDate::from_ymd_opt(2026, 4, day).unwrap(), + occurrence: Some(NaiveDate::from_ymd_opt(2026, 4, day).unwrap()), + comment: None, + } + } + + /// The frozen-series shape written on anchored exhaustion is exempt from + /// the two terminal-scheduling rules. The exemption is status-symmetric: + /// a hand-abandoned series with preserved history is the same valid + /// record, so `cancelled` is exempt on the same terms as `done`. + #[test] + fn test_frozen_series_exempt_from_terminal_rules() { + let d = |y, m, day| NaiveDate::from_ymd_opt(y, m, day).unwrap(); + for status in [Status::Done, Status::Cancelled] { + let label = status.to_string(); + let fm = Frontmatter { + status: Some(status), + check_in_date: Some(d(2026, 4, 10)), + due_date: Some(d(2026, 4, 10)), + on_date: Some(d(2026, 4, 10)), + rrule: Some("FREQ=WEEKLY;COUNT=2".to_string()), + completion_log: vec![log_entry(10)], + ..bare_fm() + }; + let errors = check_rules(Path::new("test.md"), &fm); + assert!( + errors.is_empty(), + "frozen series ({label}) must be clean; got: {:?}", + errors.iter().map(|e| &e.message).collect::>() + ); + } + } + + /// A completion log without any recurrence exempts nothing: non-recurring + /// completions append log entries too, so leftover `check_in_date` on a + /// plain done item is still a violation. + #[test] + fn test_completion_log_alone_does_not_exempt() { + let d = |y, m, day| NaiveDate::from_ymd_opt(y, m, day).unwrap(); + let fm = Frontmatter { + status: Some(Status::Done), + check_in_date: Some(d(2026, 4, 10)), + on_date: Some(d(2026, 4, 10)), + completion_log: vec![log_entry(10)], + ..bare_fm() + }; + let messages: Vec = check_rules(Path::new("test.md"), &fm) + .into_iter() + .map(|e| e.message) + .collect(); + assert_eq!(messages, vec![MSG_TERMINAL_CHECK_IN]); + } + + /// A recurrence with an empty completion log is a series never completed + /// through graf: both terminal rules still fire. + #[test] + fn test_empty_completion_log_still_flags_terminal_rules() { + let d = |y, m, day| NaiveDate::from_ymd_opt(y, m, day).unwrap(); + let fm = Frontmatter { + status: Some(Status::Done), + check_in_date: Some(d(2026, 4, 10)), + on_date: Some(d(2026, 4, 10)), + rrule: Some("FREQ=WEEKLY".to_string()), + ..bare_fm() + }; + let messages: Vec = check_rules(Path::new("test.md"), &fm) + .into_iter() + .map(|e| e.message) + .collect(); + assert_eq!(messages, vec![MSG_TERMINAL_CHECK_IN, MSG_TERMINAL_RRULE,]); + } + + /// A frozen series with no completion date still trips the date rule: + /// graf's own exhaustion always writes `on_date`. + #[test] + fn test_frozen_series_still_needs_completion_date() { + let d = |y, m, day| NaiveDate::from_ymd_opt(y, m, day).unwrap(); + let fm = Frontmatter { + status: Some(Status::Done), + check_in_date: Some(d(2026, 4, 10)), + rrule: Some("FREQ=WEEKLY".to_string()), + completion_log: vec![log_entry(10)], + ..bare_fm() + }; + let messages: Vec = check_rules(Path::new("test.md"), &fm) + .into_iter() + .map(|e| e.message) + .collect(); + assert_eq!(messages, vec![MSG_TERMINAL_NO_DATE]); + } + + /// Canary: asserts the fixtures below collectively emit exactly the + /// messages declared in `RULE_MESSAGES`, and pins the schema version + /// those messages are persisted under. Lint messages live in + /// `docs.lint_errors` and are deduped against the current pass by exact + /// text, so stale persisted sets must be rebuilt whenever the rules + /// change. Comparing against the declaration rather than a hand-copied + /// list means a rule added to `RULE_MESSAGES` but tripped by no fixture + /// also fails here, instead of passing unnoticed. + #[test] + fn test_check_rules_message_snapshot() { + let path = Path::new("test.md"); + let d = |y, m, day| NaiveDate::from_ymd_opt(y, m, day).unwrap(); + + let mut fixtures = Vec::new(); + + // Date mutual-exclusivity rules. + fixtures.push(Frontmatter { + on_date: Some(d(2026, 4, 10)), + start_date: Some(d(2026, 4, 1)), + end_date: Some(d(2026, 4, 15)), + ..bare_fm() + }); + fixtures.push(Frontmatter { + end_date: Some(d(2026, 4, 15)), + ..bare_fm() + }); + + // Terminal-item rules. + fixtures.push(Frontmatter { + status: Some(Status::Done), + check_in_date: Some(d(2026, 4, 10)), + rrule: Some("FREQ=WEEKLY".to_string()), + ..bare_fm() + }); + + // Active-task rule. + fixtures.push(Frontmatter { + status: Some(Status::Todo), + ..bare_fm() + }); + + let mut messages: Vec = fixtures + .iter() + .flat_map(|fm| check_rules(path, fm)) + .map(|e| e.message) + .collect(); + messages.sort(); + messages.dedup(); + + let mut expected: Vec = RULE_MESSAGES.iter().map(|m| m.to_string()).collect(); + expected.sort(); + + assert_eq!( + messages, expected, + "check_rules output no longer matches RULE_MESSAGES — bump SCHEMA_VERSION in db.rs, \ + and add a fixture here for any newly declared message" + ); + assert_eq!( + crate::db::SCHEMA_VERSION, + "8", + "SCHEMA_VERSION changed — update this canary alongside it" + ); + } + #[test] fn test_format_timestamp() { let dt = DateTime::parse_from_rfc3339("2025-05-20T20:35:36.732Z") @@ -1033,30 +1290,9 @@ mod tests { fn test_on_date_and_start_date_invalid() { let path = Path::new("test.md"); let fm = Frontmatter { - tldr: "test".to_string(), - labels: vec![], - created: None, - updated: None, - summary: None, on_date: Some(NaiveDate::from_ymd_opt(2026, 4, 10).unwrap()), start_date: Some(NaiveDate::from_ymd_opt(2026, 4, 1).unwrap()), - end_date: None, - due_date: None, - check_in_date: None, - verified_date: None, - context_link: None, - links: vec![], - attachments: vec![], - status: None, - priority: None, - effort: None, - blocked_by: vec![], - rrule: None, - tentative_date: None, - sort_order: None, - assigned_to: None, - recurrence: None, - completion_log: vec![], + ..bare_fm() }; let errors = check_rules(path, &fm); assert!(errors.iter().any(|e| { @@ -1069,30 +1305,9 @@ mod tests { fn test_on_date_and_end_date_invalid() { let path = Path::new("test.md"); let fm = Frontmatter { - tldr: "test".to_string(), - labels: vec![], - created: None, - updated: None, - summary: None, on_date: Some(NaiveDate::from_ymd_opt(2026, 4, 10).unwrap()), - start_date: None, end_date: Some(NaiveDate::from_ymd_opt(2026, 4, 15).unwrap()), - due_date: None, - check_in_date: None, - verified_date: None, - context_link: None, - links: vec![], - attachments: vec![], - status: None, - priority: None, - effort: None, - blocked_by: vec![], - rrule: None, - tentative_date: None, - sort_order: None, - assigned_to: None, - recurrence: None, - completion_log: vec![], + ..bare_fm() }; let errors = check_rules(path, &fm); assert!(errors.iter().any(|e| { @@ -1105,30 +1320,8 @@ mod tests { fn test_end_date_without_start_date_invalid() { let path = Path::new("test.md"); let fm = Frontmatter { - tldr: "test".to_string(), - labels: vec![], - created: None, - updated: None, - summary: None, - on_date: None, - start_date: None, end_date: Some(NaiveDate::from_ymd_opt(2026, 4, 15).unwrap()), - due_date: None, - check_in_date: None, - verified_date: None, - context_link: None, - links: vec![], - attachments: vec![], - status: None, - priority: None, - effort: None, - blocked_by: vec![], - rrule: None, - tentative_date: None, - sort_order: None, - assigned_to: None, - recurrence: None, - completion_log: vec![], + ..bare_fm() }; let errors = check_rules(path, &fm); assert!( @@ -1142,30 +1335,8 @@ mod tests { fn test_start_date_alone_valid() { let path = Path::new("test.md"); let fm = Frontmatter { - tldr: "test".to_string(), - labels: vec![], - created: None, - updated: None, - summary: None, - on_date: None, start_date: Some(NaiveDate::from_ymd_opt(2026, 4, 1).unwrap()), - end_date: None, - due_date: None, - check_in_date: None, - verified_date: None, - context_link: None, - links: vec![], - attachments: vec![], - status: None, - priority: None, - effort: None, - blocked_by: vec![], - rrule: None, - tentative_date: None, - sort_order: None, - assigned_to: None, - recurrence: None, - completion_log: vec![], + ..bare_fm() }; let errors = check_rules(path, &fm); assert!(errors.is_empty()); @@ -1175,35 +1346,127 @@ mod tests { fn test_start_date_plus_end_date_valid() { let path = Path::new("test.md"); let fm = Frontmatter { - tldr: "test".to_string(), - labels: vec![], - created: None, - updated: None, - summary: None, - on_date: None, start_date: Some(NaiveDate::from_ymd_opt(2026, 4, 1).unwrap()), end_date: Some(NaiveDate::from_ymd_opt(2026, 4, 10).unwrap()), - due_date: None, - check_in_date: None, - verified_date: None, - context_link: None, - links: vec![], - attachments: vec![], - status: None, - priority: None, - effort: None, - blocked_by: vec![], - rrule: None, - tentative_date: None, - sort_order: None, - assigned_to: None, - recurrence: None, - completion_log: vec![], + ..bare_fm() }; let errors = check_rules(path, &fm); assert!(errors.is_empty()); } + // ----------------------------------------------------------------------- + // Unparseable-rrule rule + // ----------------------------------------------------------------------- + + fn lint_content(content: &str) -> LintResult { + let dir = TempDir::new().unwrap(); + let file = dir.path().join("test.md"); + fs::write(&file, content).unwrap(); + let mut result = LintResult::default(); + lint_md_file(&file, None, false, &mut result).unwrap(); + result + } + + fn rrule_errors(result: &LintResult) -> Vec<&str> { + result + .errors + .iter() + .map(|e| e.message.as_str()) + .filter(|m| m.starts_with("unparseable rrule")) + .collect() + } + + #[test] + fn test_broken_nested_rrule_is_error() { + let result = lint_content( + "---\ntldr: test\nstatus: todo\ncheck_in_date: '2026-05-01'\nrecurrence:\n rrule: \"FREQ=BOGUS\"\n---\n", + ); + assert_eq!(result.errors.len(), 1, "got: {:?}", result.errors); + assert!(result.errors[0].message.contains("FREQ=BOGUS")); + } + + #[test] + fn test_broken_flat_rrule_is_error() { + // Uniformity: the rule reads `effective_rrule()`, so the legacy flat + // shape is checked on the same terms as the nested one. + let result = lint_content( + "---\ntldr: test\nstatus: todo\ncheck_in_date: '2026-05-01'\nrrule: \"FREQ=BOGUS\"\n---\n", + ); + assert_eq!(result.errors.len(), 1, "got: {:?}", result.errors); + assert!(result.errors[0].message.contains("FREQ=BOGUS")); + } + + #[test] + fn test_broken_rrule_on_statusless_doc_is_error() { + // Status-independent: a knowledge-base entry carrying a broken rrule + // is still corrupt data. + let result = lint_content("---\ntldr: test\nrrule: \"FREQ=BOGUS\"\n---\n"); + assert_eq!(result.errors.len(), 1, "got: {:?}", result.errors); + assert!(result.errors[0].message.contains("FREQ=BOGUS")); + } + + #[test] + fn test_valid_rrule_with_past_until_is_ok() { + // The rrule crate rejects UNTIL < DTSTART at parse time, so lint + // anchors at the epoch. A valid rule whose UNTIL has already passed + // must not lint as broken. + let result = lint_content( + "---\ntldr: test\nstatus: todo\ncheck_in_date: '2026-05-01'\nrecurrence:\n rrule: \"FREQ=DAILY;UNTIL=20200101T120000Z\"\n---\n", + ); + assert!(result.errors.is_empty(), "got: {:?}", result.errors); + } + + #[test] + fn test_empty_rrule_is_error() { + let result = lint_content( + "---\ntldr: test\nstatus: todo\ncheck_in_date: '2026-05-01'\nrrule: \"\"\n---\n", + ); + assert_eq!(rrule_errors(&result).len(), 1, "got: {:?}", result.errors); + } + + #[test] + fn test_done_with_broken_rrule_reports_both_errors() { + // Lint errors are additive: the terminal-rrule rule and the + // unparseable-rrule rule are independently true here. + let result = lint_content( + "---\ntldr: test\nstatus: done\non_date: '2026-04-10'\nrrule: \"FREQ=BOGUS\"\n---\n", + ); + let messages: Vec<&str> = result.errors.iter().map(|e| e.message.as_str()).collect(); + assert!( + messages.iter().any(|m| *m == MSG_TERMINAL_RRULE), + "got: {messages:?}" + ); + assert_eq!(rrule_errors(&result).len(), 1, "got: {messages:?}"); + } + + #[test] + fn test_newline_injected_rrule_is_error() { + // A double-quoted YAML scalar carries a literal newline into the rrule + // string, which would otherwise inject a second ICS content line. The + // rule must flag it rather than bless it as clean. + let result = lint_content( + "---\ntldr: test\nstatus: todo\ncheck_in_date: '2026-05-01'\nrrule: \"FREQ=DAILY\\nRRULE:FREQ=MINUTELY\"\n---\n", + ); + assert_eq!(rrule_errors(&result).len(), 1, "got: {:?}", result.errors); + assert!(result.errors[0].message.contains("newline")); + } + + #[test] + fn test_unparseable_rrule_message_is_stable() { + // Canary pinning the exact persisted message text for a fixed bad rule. + // The tail is the `rrule` crate's own parse-error wording, which is + // persisted verbatim in `docs.lint_errors`. If this fails after an + // `rrule`-crate upgrade that reworded the error, bump SCHEMA_VERSION in + // db.rs and update this snapshot. + let result = lint_content("---\ntldr: test\nrrule: \"FREQ=BOGUS\"\n---\n"); + let msgs = rrule_errors(&result); + assert_eq!(msgs.len(), 1, "got: {:?}", result.errors); + assert_eq!( + msgs[0], + "unparseable rrule 'FREQ=BOGUS': `BOGUS` is not a valid frequency.", + ); + } + #[test] fn test_lint_result_to_json_format() { let result = LintResult { diff --git a/src/manifest.rs b/src/manifest.rs index cb97327..8609763 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -116,6 +116,15 @@ pub struct ManifestDomain { pub id: GlobalId, } +/// Outcome of validating the manifest against repo configs on disk. +/// +/// Errors fail `manifest check`; warnings are surfaced but do not. +#[derive(Debug, Default)] +pub struct RepoCheckReport { + pub errors: Vec, + pub warnings: Vec, +} + impl AppManifest { /// Determine the manifest file path. /// @@ -124,14 +133,8 @@ impl AppManifest { if let Ok(p) = std::env::var("GRAF_MANIFEST") { return Ok(PathBuf::from(p)); } - let config_dir = match std::env::var("XDG_CONFIG_HOME") { - Ok(dir) => PathBuf::from(dir), - Err(_) => { - let home = std::env::var("HOME").context("HOME environment variable not set")?; - PathBuf::from(home).join(".config") - } - }; - Ok(config_dir.join("graf/manifest.toml")) + let config_dir = crate::config::graf_config_dir()?; + Ok(config_dir.join("manifest.toml")) } /// Load from the default path. Returns `None` if the file doesn't exist. @@ -191,6 +194,11 @@ impl AppManifest { self.domain.iter().find(|d| d.slug == *slug) } + /// Look up a domain by global ID. + pub fn find_domain_by_id(&self, id: &GlobalId) -> Option<&ManifestDomain> { + self.domain.iter().find(|d| d.id == *id) + } + /// Get all repo global IDs (useful for URI resolution). pub fn repo_ids(&self) -> impl Iterator { self.repo.iter().map(|r| &r.id) @@ -205,60 +213,63 @@ impl AppManifest { let mut errors = Vec::new(); // Duplicate repo slugs - let mut seen: HashMap<&str, &GlobalId> = HashMap::new(); - for repo in &self.repo { - if let Some(prev_id) = seen.insert(repo.slug.as_str(), &repo.id) { - errors.push(format!( - "duplicate repo slug {:?} (IDs: {}, {})", - repo.slug.as_str(), - prev_id, - repo.id - )); - } + for (slug, prev_id, id) in + Self::duplicate_pairs(self.repo.iter().map(|r| (r.slug.as_str(), r.id.as_str()))) + { + errors.push(format!( + "duplicate repo slug {slug:?} (IDs: {prev_id}, {id})" + )); } // Duplicate domain slugs - let mut seen: HashMap<&str, &GlobalId> = HashMap::new(); - for domain in &self.domain { - if let Some(prev_id) = seen.insert(domain.slug.as_str(), &domain.id) { - errors.push(format!( - "duplicate domain slug {:?} (IDs: {}, {})", - domain.slug.as_str(), - prev_id, - domain.id - )); - } + for (slug, prev_id, id) in + Self::duplicate_pairs(self.domain.iter().map(|d| (d.slug.as_str(), d.id.as_str()))) + { + errors.push(format!( + "duplicate domain slug {slug:?} (IDs: {prev_id}, {id})" + )); } // Duplicate repo IDs - let mut seen: HashMap<&str, &str> = HashMap::new(); - for repo in &self.repo { - if let Some(prev_slug) = seen.insert(repo.id.as_str(), repo.slug.as_str()) { - errors.push(format!( - "duplicate repo ID {:?} (slugs: {}, {})", - repo.id.as_str(), - prev_slug, - repo.slug.as_str() - )); - } + for (id, prev_slug, slug) in + Self::duplicate_pairs(self.repo.iter().map(|r| (r.id.as_str(), r.slug.as_str()))) + { + errors.push(format!( + "duplicate repo ID {id:?} (slugs: {prev_slug}, {slug})" + )); } // Duplicate domain IDs - let mut seen: HashMap<&str, &str> = HashMap::new(); - for domain in &self.domain { - if let Some(prev_slug) = seen.insert(domain.id.as_str(), domain.slug.as_str()) { - errors.push(format!( - "duplicate domain ID {:?} (slugs: {}, {})", - domain.id.as_str(), - prev_slug, - domain.slug.as_str() - )); - } + for (id, prev_slug, slug) in + Self::duplicate_pairs(self.domain.iter().map(|d| (d.id.as_str(), d.slug.as_str()))) + { + errors.push(format!( + "duplicate domain ID {id:?} (slugs: {prev_slug}, {slug})" + )); } errors } + /// Find duplicate keys in a sequence of `(key, value)` pairs. + /// + /// Returns `(key, previous_value, value)` for each duplicate occurrence, + /// in encounter order of the later occurrence. For a key seen 3+ times, + /// `previous_value` is the immediately preceding occurrence's value, not + /// the first. + fn duplicate_pairs<'a>( + pairs: impl Iterator, + ) -> Vec<(&'a str, &'a str, &'a str)> { + let mut seen: HashMap<&str, &str> = HashMap::new(); + let mut dups = Vec::new(); + for (key, value) in pairs { + if let Some(prev) = seen.insert(key, value) { + dups.push((key, prev, value)); + } + } + dups + } + /// Check for non-fatal issues (prefix-extension relationships between repo IDs). /// /// These are not errors — prefix relationships resolve correctly via longest-match — @@ -281,16 +292,16 @@ impl AppManifest { /// Validate the manifest against actual repo configs on disk. /// /// Checks: paths exist as directories, `.graf/config.toml` exists, - /// manifest ID matches repo config ID. - // TODO(cross-repo-ref-validation) - pub fn validate_repos(&self) -> Vec { - let mut errors = Vec::new(); + /// manifest ID matches repo config ID, and each repo's `refs` section + /// resolves against the manifest via `validate_refs`. + pub fn validate_repos(&self) -> RepoCheckReport { + let mut report = RepoCheckReport::default(); for repo in &self.repo { let path = match Self::expand_path(&repo.path) { Ok(p) => p, Err(e) => { - errors.push(format!( + report.errors.push(format!( "repo {:?}: cannot expand path {:?}: {e}", repo.slug.as_str(), repo.path @@ -300,7 +311,7 @@ impl AppManifest { }; if !path.is_dir() { - errors.push(format!( + report.errors.push(format!( "repo {:?}: path does not exist or is not a directory: {}", repo.slug.as_str(), path.display() @@ -311,23 +322,26 @@ impl AppManifest { match RepoConfig::load(&path) { Ok(Some(config)) => { if config.id != repo.id { - errors.push(format!( + report.errors.push(format!( "repo {:?}: manifest ID ({}) does not match repo config ID ({})", repo.slug.as_str(), repo.id, config.id )); } + let refs_report = self.validate_refs(&repo.slug, &config.refs); + report.errors.extend(refs_report.errors); + report.warnings.extend(refs_report.warnings); } Ok(None) => { - errors.push(format!( + report.errors.push(format!( "repo {:?}: no .graf/config.toml found at {}", repo.slug.as_str(), path.display() )); } Err(e) => { - errors.push(format!( + report.errors.push(format!( "repo {:?}: error reading config: {e}", repo.slug.as_str() )); @@ -335,7 +349,112 @@ impl AppManifest { } } - errors + report + } + + /// Full `manifest check` report: in-memory manifest validation + /// (`validate`), on-disk repo/refs validation (`validate_repos`), and + /// non-fatal `warnings`, merged into one `{errors, warnings}`. Single + /// source of truth for both the text and JSON check paths. + pub fn check_report(&self) -> RepoCheckReport { + let repo_report = self.validate_repos(); + let mut errors = self.validate(); + errors.extend(repo_report.errors); + let mut warnings = self.warnings(); + warnings.extend(repo_report.warnings); + RepoCheckReport { errors, warnings } + } + + /// Validate a repo's `refs` section against this manifest. + /// + /// Errors: a ref repo/domain ID that resolves to no manifest entry; + /// duplicate slug or duplicate ID within `refs.repo` or within + /// `refs.domain`. Warning: a ref slug equal to an app-manifest slug that + /// is bound to a *different* global ID (installation-local collision). + fn validate_refs(&self, repo_slug: &Slug, refs: &RepoRefs) -> RepoCheckReport { + let mut report = RepoCheckReport::default(); + let rs = repo_slug.as_str(); + + // Unknown / duplicate repo refs. + for entry in &refs.repo { + if self.find_repo_by_id(&entry.id).is_none() { + report.errors.push(format!( + "repo {rs:?}: ref repo slug {:?} points to unknown ID {}", + entry.slug.as_str(), + entry.id + )); + } + } + for (slug, prev_id, id) in + Self::duplicate_pairs(refs.repo.iter().map(|e| (e.slug.as_str(), e.id.as_str()))) + { + report.errors.push(format!( + "repo {rs:?}: duplicate ref repo slug {slug:?} (IDs: {prev_id}, {id})" + )); + } + for (id, prev_slug, slug) in + Self::duplicate_pairs(refs.repo.iter().map(|e| (e.id.as_str(), e.slug.as_str()))) + { + report.errors.push(format!( + "repo {rs:?}: duplicate ref repo ID {id:?} (slugs: {prev_slug}, {slug})" + )); + } + + // Unknown / duplicate domain refs. + for entry in &refs.domain { + if self.find_domain_by_id(&entry.id).is_none() { + report.errors.push(format!( + "repo {rs:?}: ref domain slug {:?} points to unknown ID {}", + entry.slug.as_str(), + entry.id + )); + } + } + for (slug, prev_id, id) in + Self::duplicate_pairs(refs.domain.iter().map(|e| (e.slug.as_str(), e.id.as_str()))) + { + report.errors.push(format!( + "repo {rs:?}: duplicate ref domain slug {slug:?} (IDs: {prev_id}, {id})" + )); + } + for (id, prev_slug, slug) in + Self::duplicate_pairs(refs.domain.iter().map(|e| (e.id.as_str(), e.slug.as_str()))) + { + report.errors.push(format!( + "repo {rs:?}: duplicate ref domain ID {id:?} (slugs: {prev_slug}, {slug})" + )); + } + + // Manifest-slug shadowing with a different ID (warning only). Each ref + // kind is checked only against its own manifest namespace — repos and + // domains are separate namespaces, so a domain ref reusing a repo slug + // (or vice versa) is not a shadow. + for entry in &refs.repo { + if let Some(mrepo) = self.find_repo_by_slug(&entry.slug) + && mrepo.id != entry.id + { + report.warnings.push(format!( + "repo {rs:?}: ref slug {:?} shadows manifest repo slug bound to different ID (manifest: {}, ref: {})", + entry.slug.as_str(), + mrepo.id, + entry.id + )); + } + } + for entry in &refs.domain { + if let Some(mdom) = self.find_domain_by_slug(&entry.slug) + && mdom.id != entry.id + { + report.warnings.push(format!( + "repo {rs:?}: ref slug {:?} shadows manifest domain slug bound to different ID (manifest: {}, ref: {})", + entry.slug.as_str(), + mdom.id, + entry.id + )); + } + } + + report } } @@ -711,7 +830,7 @@ mod tests { }], domain: vec![], }; - let errors = manifest.validate_repos(); + let errors = manifest.validate_repos().errors; assert_eq!(errors.len(), 1); assert!(errors[0].contains("does not exist")); } @@ -727,7 +846,7 @@ mod tests { }], domain: vec![], }; - let errors = manifest.validate_repos(); + let errors = manifest.validate_repos().errors; assert_eq!(errors.len(), 1); assert!(errors[0].contains("no .graf/config.toml")); } @@ -751,7 +870,7 @@ mod tests { }], domain: vec![], }; - let errors = manifest.validate_repos(); + let errors = manifest.validate_repos().errors; assert_eq!(errors.len(), 1); assert!(errors[0].contains("does not match")); } @@ -775,7 +894,599 @@ mod tests { }], domain: vec![], }; - assert!(manifest.validate_repos().is_empty()); + let report = manifest.validate_repos(); + assert!(report.errors.is_empty()); + assert!(report.warnings.is_empty()); + } + + // ---- Refs validation ---- + + fn ref_entry(slug: &str, id: &str) -> RefEntry { + RefEntry { + slug: Slug::new(slug).unwrap(), + id: GlobalId::new(id).unwrap(), + } + } + + fn m_repo(slug: &str, path: &str, id: &str) -> ManifestRepo { + ManifestRepo { + slug: Slug::new(slug).unwrap(), + path: path.to_string(), + id: GlobalId::new(id).unwrap(), + } + } + + fn m_domain(slug: &str, id: &str) -> ManifestDomain { + ManifestDomain { + slug: Slug::new(slug).unwrap(), + id: GlobalId::new(id).unwrap(), + } + } + + /// Write a repo config with the given ID and refs into a fresh tempdir. + fn repo_on_disk(id: &str, refs: RepoRefs) -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + RepoConfig { + id: GlobalId::new(id).unwrap(), + default_slug: Slug::new("s").unwrap(), + domain: None, + refs, + } + .save(dir.path()) + .unwrap(); + dir + } + + #[test] + fn find_domain_by_id_found() { + let manifest = AppManifest { + repo: vec![], + domain: vec![m_domain("household", "example.com/household")], + }; + assert!( + manifest + .find_domain_by_id(&GlobalId::new("example.com/household").unwrap()) + .is_some() + ); + } + + #[test] + fn find_domain_by_id_not_found() { + let manifest = AppManifest { + repo: vec![], + domain: vec![m_domain("household", "example.com/household")], + }; + assert!( + manifest + .find_domain_by_id(&GlobalId::new("example.com/other").unwrap()) + .is_none() + ); + } + + #[test] + fn validate_refs_all_valid() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("shared", "example.com/shared")], + domain: vec![ref_entry("house", "example.com/house")], + }, + ); + let shared = repo_on_disk("example.com/shared", RepoRefs::default()); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo( + "shared", + shared.path().to_str().unwrap(), + "example.com/shared", + ), + ], + domain: vec![m_domain("house", "example.com/house")], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_unknown_repo_id() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("x", "example.com/missing")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("unknown ID")); + assert!(errors[0].contains("example.com/missing")); + assert!(errors[0].contains("\"x\"")); + } + + #[test] + fn validate_refs_unknown_domain_id() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![], + domain: vec![ref_entry("d", "example.com/missingdom")], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("ref domain slug")); + assert!(errors[0].contains("example.com/missingdom")); + } + + #[test] + fn validate_refs_duplicate_repo_slug() { + let a = repo_on_disk("example.com/a", RepoRefs::default()); + let b = repo_on_disk("example.com/b", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ + ref_entry("dup", "example.com/a"), + ref_entry("dup", "example.com/b"), + ], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo("a", a.path().to_str().unwrap(), "example.com/a"), + m_repo("b", b.path().to_str().unwrap(), "example.com/b"), + ], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("duplicate ref repo slug")); + assert!(errors[0].contains("\"dup\"")); + } + + #[test] + fn validate_refs_duplicate_domain_slug() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![], + domain: vec![ + ref_entry("dup", "example.com/d1"), + ref_entry("dup", "example.com/d2"), + ], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![ + m_domain("d1", "example.com/d1"), + m_domain("d2", "example.com/d2"), + ], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("duplicate ref domain slug")); + } + + #[test] + fn validate_refs_duplicate_repo_id() { + let a = repo_on_disk("example.com/a", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ + ref_entry("one", "example.com/a"), + ref_entry("two", "example.com/a"), + ], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo("a", a.path().to_str().unwrap(), "example.com/a"), + ], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("duplicate ref repo ID")); + assert!(errors[0].contains("example.com/a")); + } + + #[test] + fn validate_refs_same_slug_repo_and_domain_ok() { + let shared = repo_on_disk("example.com/shared", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("both", "example.com/shared")], + domain: vec![ref_entry("both", "example.com/house")], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo( + "shared", + shared.path().to_str().unwrap(), + "example.com/shared", + ), + ], + domain: vec![m_domain("house", "example.com/house")], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_shadow_same_id_clean() { + // Ref slug equals a manifest repo slug bound to the SAME ID: agreement. + let shared = repo_on_disk("example.com/shared", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("shared", "example.com/shared")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo( + "shared", + shared.path().to_str().unwrap(), + "example.com/shared", + ), + ], + domain: vec![], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_shadow_different_id_warns() { + // Ref slug "shared" resolves by ID to manifest repo "real", but the + // manifest also has a repo *slug* "shared" bound to a different ID. + let real = repo_on_disk("example.com/real", RepoRefs::default()); + let other = repo_on_disk("example.com/other", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("shared", "example.com/real")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo("real", real.path().to_str().unwrap(), "example.com/real"), + // Manifest repo slug "shared" bound to a different ID than the ref. + m_repo( + "shared", + other.path().to_str().unwrap(), + "example.com/other", + ), + ], + domain: vec![], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert_eq!(report.warnings.len(), 1, "warnings: {:?}", report.warnings); + assert!(report.warnings[0].contains("shadows manifest repo slug")); + } + + #[test] + fn validate_refs_duplicate_domain_id() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![], + domain: vec![ + ref_entry("one", "example.com/d"), + ref_entry("two", "example.com/d"), + ], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![m_domain("d", "example.com/d")], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 1, "errors: {errors:?}"); + assert!(errors[0].contains("duplicate ref domain ID")); + assert!(errors[0].contains("example.com/d")); + } + + #[test] + fn validate_refs_shadow_domain_different_id_warns() { + // Ref domain slug "shared" resolves by ID to manifest domain "real", + // but the manifest also has a domain *slug* "shared" bound to a + // different ID. + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![], + domain: vec![ref_entry("shared", "example.com/real")], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![ + m_domain("real", "example.com/real"), + m_domain("shared", "example.com/other"), + ], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert_eq!(report.warnings.len(), 1, "warnings: {:?}", report.warnings); + assert!(report.warnings[0].contains("shadows manifest domain slug")); + } + + #[test] + fn validate_refs_domain_ref_reusing_repo_slug_ok() { + // A domain ref whose slug equals a manifest *repo* slug is not a shadow: + // repos and domains are separate namespaces. + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![], + domain: vec![ref_entry("life", "example.com/house")], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![m_domain("house", "example.com/house")], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_repo_ref_reusing_domain_slug_ok() { + // A repo ref whose slug equals a manifest *domain* slug is not a shadow. + let shared = repo_on_disk("example.com/shared", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("house", "example.com/shared")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo( + "shared", + shared.path().to_str().unwrap(), + "example.com/shared", + ), + ], + domain: vec![m_domain("house", "example.com/house")], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_self_reference_ok() { + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ref_entry("me", "example.com/life")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + life.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![], + }; + let report = manifest.validate_repos(); + assert!(report.errors.is_empty(), "errors: {:?}", report.errors); + assert!( + report.warnings.is_empty(), + "warnings: {:?}", + report.warnings + ); + } + + #[test] + fn validate_refs_bad_refs_and_id_mismatch() { + // Config ID differs from manifest ID AND refs point nowhere: both fire. + let dir = repo_on_disk( + "example.com/actual", + RepoRefs { + repo: vec![ref_entry("x", "example.com/missing")], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![m_repo( + "life", + dir.path().to_str().unwrap(), + "example.com/life", + )], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + assert_eq!(errors.len(), 2, "errors: {errors:?}"); + assert!(errors.iter().any(|e| e.contains("does not match"))); + assert!(errors.iter().any(|e| e.contains("unknown ID"))); + } + + #[test] + fn validate_refs_triple_duplicate_repo_slug() { + // Three ref entries share a slug (distinct IDs): N=3 occurrences yield + // exactly N-1=2 duplicate-slug reports. + let a = repo_on_disk("example.com/a", RepoRefs::default()); + let b = repo_on_disk("example.com/b", RepoRefs::default()); + let c = repo_on_disk("example.com/c", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ + ref_entry("dup", "example.com/a"), + ref_entry("dup", "example.com/b"), + ref_entry("dup", "example.com/c"), + ], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo("a", a.path().to_str().unwrap(), "example.com/a"), + m_repo("b", b.path().to_str().unwrap(), "example.com/b"), + m_repo("c", c.path().to_str().unwrap(), "example.com/c"), + ], + domain: vec![], + }; + let errors = manifest.validate_repos().errors; + let dups: Vec<_> = errors + .iter() + .filter(|e| e.contains("duplicate ref repo slug")) + .collect(); + assert_eq!(dups.len(), 2, "errors: {errors:?}"); + } + + #[test] + fn check_report_merges_all_streams() { + // One manifest that simultaneously produces a `validate()` error + // (duplicate manifest repo slug), a `validate_repos` refs error + // (unknown ref ID), a repo-level shadow warning, and a `warnings()` + // prefix-relationship warning — all four streams must reach + // `check_report`. + let real = repo_on_disk("example.com/real", RepoRefs::default()); + let other = repo_on_disk("example.com/other", RepoRefs::default()); + let dup1 = repo_on_disk("example.com/dup1", RepoRefs::default()); + let dup2 = repo_on_disk("example.com/dup2", RepoRefs::default()); + let sub = repo_on_disk("example.com/life/sub", RepoRefs::default()); + let life = repo_on_disk( + "example.com/life", + RepoRefs { + repo: vec![ + // Unknown ID -> validate_repos error. + ref_entry("x", "example.com/missing"), + // Resolves to "real", but manifest slug "shared" is bound to + // a different ID -> shadow warning. + ref_entry("shared", "example.com/real"), + ], + domain: vec![], + }, + ); + let manifest = AppManifest { + repo: vec![ + m_repo("life", life.path().to_str().unwrap(), "example.com/life"), + m_repo("real", real.path().to_str().unwrap(), "example.com/real"), + m_repo( + "shared", + other.path().to_str().unwrap(), + "example.com/other", + ), + // Duplicate manifest repo slug "dup" -> validate() error. + m_repo("dup", dup1.path().to_str().unwrap(), "example.com/dup1"), + m_repo("dup", dup2.path().to_str().unwrap(), "example.com/dup2"), + // ID "example.com/life/sub" is a /-boundary extension of + // "example.com/life" -> `warnings()` prefix warning. + m_repo("sub", sub.path().to_str().unwrap(), "example.com/life/sub"), + ], + domain: vec![], + }; + let report = manifest.check_report(); + assert!( + report + .errors + .iter() + .any(|e| e.contains("duplicate repo slug")), + "missing validate() error: {:?}", + report.errors + ); + assert!( + report.errors.iter().any(|e| e.contains("unknown ID")), + "missing validate_repos error: {:?}", + report.errors + ); + assert!( + report + .warnings + .iter() + .any(|w| w.contains("shadows manifest repo slug")), + "missing repo-level warning: {:?}", + report.warnings + ); + assert!( + report + .warnings + .iter() + .any(|w| w.contains("prefix relationship")), + "missing warnings() prefix warning: {:?}", + report.warnings + ); } #[test] diff --git a/src/todo.rs b/src/todo.rs index 4d60db1..089b212 100644 --- a/src/todo.rs +++ b/src/todo.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::fmt; use std::fs; use std::io::Write; @@ -20,22 +21,24 @@ use crate::identity::{GlobalId, Slug}; // Query // --------------------------------------------------------------------------- -/// Result of a todo query: tasks + any lint errors found during sync. +/// Result of a todo query: tasks + the repo's lint errors. +/// +/// `lint_errors` is comprehensive, not a "what changed this pass" channel: +/// it is seeded from the lint state persisted on every cached doc +/// (`db::Cache::all_lint_errors`), so violations committed long ago are +/// reported on every query, under any status filter or horizon, and for +/// docs that are not tasks at all. Parse and validation failures from the +/// current sync pass are merged in on top; duplicates across the two +/// sources collapse. +/// +/// A horizon windows terminal items by completion date; a terminal item with +/// no completion date still passes the SQL filter and surfaces here as a lint +/// error. /// /// Invariant: every item in `tasks` has a non-null `effective_date`. Rows /// whose `effective_date` resolves to NULL in the cache (indicating a /// scheduling-date lint violation) are partitioned into `lint_errors` -/// instead of `tasks`. This holds regardless of whether the current -/// sync pass re-linted the violating file. -/// -/// Horizon note: when `horizon_days` is set, the SQL WHERE clause in -/// `db::Cache::query_tasks` filters out active items with NULL -/// `effective_date` before they reach the partition (terminal items -/// bypass the horizon filter). The partition only observes null-date -/// rows that the DB layer actually returned. Brenn's todo query does -/// not set a horizon so the invariant holds end-to-end there; callers -/// that set a horizon should understand that null-date active items -/// are silent under horizon-bounded queries by design. +/// instead of `tasks`. #[derive(Debug, Serialize)] pub struct TodoQueryResult { /// Sharing domains that contributed results (for contamination tracking). @@ -105,7 +108,8 @@ pub struct QueryOptions { pub repo_slug: Option, /// Sharing domain to tag onto results (when operating with manifest). pub repo_domain: Option, - /// When set, only include items with effective_date <= today + N days. + /// When set, only include active items with effective_date <= today + N + /// days and done/cancelled items completed within the last N days. /// None means no date filtering (the default). pub horizon_days: Option, pub include_done: bool, @@ -142,25 +146,51 @@ pub fn todo_query(opts: &QueryOptions) -> Result { let slug = &opts.repo_slug; let domain = &opts.repo_domain; + // lint_errors is built from three sources against one dedup set: the + // persisted per-doc lint state (comprehensive), this pass's sync errors + // (parse/validation failures, plus lint messages that duplicate the ones + // just persisted), and the null-effective_date partition below. + let mut lint_errors: Vec = Vec::new(); + let mut seen: HashSet<(String, String)> = HashSet::new(); + let mut paths_with_lint: HashSet = HashSet::new(); + + // Single emission point for every source, so the dedup key can never + // diverge between them. + let mut emit = |path: String, message: String| { + if seen.insert((path.clone(), message.clone())) { + lint_errors.push(TodoError { + path, + repo: slug.clone(), + message, + }); + } + }; + + for e in sync.cache.all_lint_errors()? { + let path = e.path.display().to_string(); + paths_with_lint.insert(path.clone()); + emit(path, e.message); + } + + for e in sync.errors { + emit(e.path.display().to_string(), e.message); + } + // Partition rows: those with a resolved effective_date become TodoItems; // those with null effective_date (a scheduling-date lint violation in - // the stored data) become synthesized lint_errors. See - // TodoQueryResult's invariant doc. + // the stored data) stay out of `tasks`. Such a row always carries a + // persisted lint message naming the specific rule it broke, so the + // generic synthesized message is only emitted if that state is somehow + // absent — reporting something beats reporting nothing. let mut tasks = Vec::with_capacity(rows.len()); - let mut lint_errors: Vec = sync - .errors - .into_iter() - .map(|e| TodoError { - path: e.path.display().to_string(), - repo: slug.clone(), - message: e.message, - }) - .collect(); - for row in rows { match task_row_to_item(row, slug.clone(), domain.clone()) { Ok(item) => tasks.push(item), - Err(err) => lint_errors.push(err), + Err(err) => { + if !paths_with_lint.contains(&err.path) { + emit(err.path, err.message); + } + } } } @@ -632,19 +662,11 @@ pub enum DoneError { /// The file's frontmatter or state is not eligible for `done` /// (lint errors, malformed input, etc.). Returned as plain text — /// not one of the design-named structured codes. - // TODO(done-error-taxonomy-cleanup): split into named variants so - // frontends can branch on specific causes. Invalid(String), // --- pre-flight / MCP layer errors (not business logic) --- - // TODO(mcp-preflight-shared-error-type): PathNotFound and InvalidDate are - // structurally identical to ScheduleError::PathNotFound/InvalidDate. Extract - // a shared McpPreflightError type (cli/mcp.rs or cli/mcp_errors.rs) so all - // tool error enums embed it rather than duplicating. Deferred until - // cancel-mcp-envelope lands (third copy forces the issue) or - // done-error-derive-serialize sweeps both enums. // TODO(done-error-grouping-comment): add doc-comment grouping markers // separating pre-flight/MCP-layer, business-logic, and infra variants as - // DoneError grows with done-error-taxonomy-cleanup variants. + // DoneError grows. /// Path argument does not resolve in the repo (file not found, /// malformed slug, lint errors, etc.). Returned as a structured /// envelope — the LLM should check the path and retry. @@ -730,12 +752,6 @@ impl DoneError { } /// Render the `{error, reason, ...payload}` JSON envelope. - // TODO(done-error-derive-serialize): replace hand-rolled mutation - // with `#[derive(Serialize)] #[serde(tag = "error", rename_all = "snake_case")]`. - // Phase 3 added `ScheduleError` with the same hand-rolled idiom - // (see below in this file), so this pattern is now load-bearing for - // two error enums — the derive refactor gets more valuable, not - // less, and should sweep both enums in one pass. pub fn to_json(&self) -> serde_json::Value { use serde_json::json; let mut obj = json!({ @@ -1012,10 +1028,6 @@ fn done_anchored( )) })?; - // TODO(done-anchored-double-rrule-parse): this computes `naive_next` from - // `previous_anchor`, then may re-parse/iterate the same rrule below for - // `compute_occurrence_after`. A unified helper returning both in one pass - // would halve the work on the stale branch. let naive_next = compute_next_occurrence(rrule_str, previous_anchor).map_err(DoneError::Infra)?; @@ -1118,7 +1130,9 @@ fn anchored_terminal_completion( FieldMutation::Remove { field: Field::TentativeDate, }, - // Preserve: CheckInDate, DueDate, Recurrence (+ legacy Rrule). + // Preserve: CheckInDate, DueDate, Recurrence (+ legacy Rrule) — frozen + // at the final occurrence. `lint::check_rules` exempts this shape + // (recurrence + non-empty completion log) from its terminal rules. // Clear sort_order: terminal tasks rank by on_date in the query // layer, and the only writer of sort_order is `todo_reorder`. FieldMutation::Remove { @@ -1494,11 +1508,9 @@ pub struct ReorderAnchor { impl ReorderAnchor { /// Effective rank: `COALESCE(sort_order, priority, UNRANKED)`. - /// Matches the SQL `ORDER BY` and `cli::rank()`. + /// Delegates to the shared `frontmatter::rank` formula. fn rank(&self) -> f64 { - self.sort_order - .or(self.priority.map(|p| p as f64)) - .unwrap_or(crate::frontmatter::UNRANKED) + crate::frontmatter::rank(self.sort_order, self.priority) } } @@ -1606,14 +1618,26 @@ pub fn todo_reorder(opts: &ReorderOptions) -> Result { // Recurrence helpers // --------------------------------------------------------------------------- -/// Compute the next occurrence date from an RFC 5545 RRULE string. +/// Parse an RFC 5545 RRULE body into an `RRuleSet` anchored at noon-UTC on +/// `dtstart_date`. /// -/// Uses `dtstart_date` as the DTSTART (the current occurrence's scheduled date) -/// and returns the first occurrence strictly after DTSTART, or None if the rule -/// is exhausted. -fn compute_next_occurrence(rrule_str: &str, dtstart_date: NaiveDate) -> Result> { +/// The rrule crate cannot parse a bare RRULE body, so this synthesizes the +/// `DTSTART:` line the parser requires. Noon-UTC is deliberate: it sidesteps +/// DST seams in the crate. The crate validates at parse time and validation +/// is DTSTART-relative — notably a rule whose `UNTIL` precedes `dtstart_date` +/// is rejected outright. +pub(crate) fn parse_rrule_set(rrule_str: &str, dtstart_date: NaiveDate) -> Result { + // An embedded newline lets the body inject additional ICS content lines + // (a second RRULE, or EXRULE/RDATE/EXDATE) into the DTSTART block built + // below. The rrule crate accepts multi-line input, so such a line would + // silently change occurrence computation while parsing cleanly. Reject it + // so both lint and the done-time callers refuse the injection. + ensure!( + !rrule_str.contains(['\n', '\r']), + "rrule contains an embedded newline" + ); + // The rrule crate needs DateTime, not NaiveDate. - // noon-UTC is deliberate: sidesteps DST seams in the rrule crate. let noon = NaiveTime::from_hms_opt(12, 0, 0).unwrap(); let dtstart = Utc .from_utc_datetime(&dtstart_date.and_time(noon)) @@ -1623,9 +1647,18 @@ fn compute_next_occurrence(rrule_str: &str, dtstart_date: NaiveDate) -> Result Result> { + let rrset = parse_rrule_set(rrule_str, dtstart_date)?; // The rrule crate includes dtstart itself as the first occurrence, // so we need at most 2 to find the one strictly after dtstart. @@ -1661,17 +1694,7 @@ fn compute_occurrence_after( dtstart_date: NaiveDate, threshold: NaiveDate, ) -> Result> { - let noon = NaiveTime::from_hms_opt(12, 0, 0).unwrap(); - let dtstart = Utc - .from_utc_datetime(&dtstart_date.and_time(noon)) - .with_timezone(&rrule::Tz::UTC); - let full_str = format!( - "DTSTART:{}\nRRULE:{rrule_str}", - dtstart.format("%Y%m%dT%H%M%SZ") - ); - let rrset: RRuleSet = full_str - .parse() - .with_context(|| format!("parsing rrule: {rrule_str}"))?; + let rrset = parse_rrule_set(rrule_str, dtstart_date)?; // Safety bound: stale-anchor windows realistically produce tens to low // hundreds; MAX_RRULE_ITERATIONS protects against pathological rrule @@ -2267,6 +2290,70 @@ mod tests { ); } + #[test] + fn parse_rrule_set_accepts_a_valid_rule() { + assert!(parse_rrule_set("FREQ=WEEKLY;BYDAY=SA", d(2026, 4, 18)).is_ok()); + } + + #[test] + fn parse_rrule_set_error_names_the_rule() { + let err = parse_rrule_set("FREQ=BOGUS", d(2026, 4, 18)).unwrap_err(); + assert!( + format!("{err:#}").contains("FREQ=BOGUS"), + "error should name the offending rule: {err:#}" + ); + } + + #[test] + fn parse_rrule_set_rejects_embedded_newline() { + // A newline would inject additional ICS content lines into the parse. + for injected in [ + "FREQ=DAILY\nRRULE:FREQ=MINUTELY", + "FREQ=DAILY\rRRULE:FREQ=MINUTELY", + ] { + let err = parse_rrule_set(injected, d(2026, 4, 18)).unwrap_err(); + assert!( + format!("{err:#}").contains("newline"), + "expected newline rejection, got: {err:#}" + ); + } + } + + #[test] + fn todo_cancel_refuses_a_task_with_an_unparseable_rrule() { + // The lint-gate in `read_and_validate` blocks every mutation of a + // broken-rrule task, cancel included — even though cancel would strip + // the recurrence and so incidentally remove the bad rrule. Mutating + // around corrupt data hides it; the remedy is to fix the rrule in the + // file first. + let dir = setup_repo(); + let source = "---\ntldr: test\nstatus: todo\ncheck_in_date: 2026-04-18\nrecurrence:\n rrule: \"FREQ=BOGUS\"\n---\nBody.\n"; + let path = write_task(&dir, "todo/task.md", source); + + let outcome = todo_cancel(&CancelOptions { + repo_dir: dir.path().to_path_buf(), + path: path.clone(), + date: None, + replacing: None, + today: Some(d(2026, 4, 18)), + }); + let Err(err) = outcome else { + panic!("cancel must be blocked by the lint gate"); + }; + + let msg = format!("{err:#}"); + assert!( + msg.contains("has lint errors, fix before mutating"), + "{msg}" + ); + assert!(msg.contains("FREQ=BOGUS"), "{msg}"); + assert_eq!( + fs::read_to_string(dir.path().join(&path)).unwrap(), + source, + "file must be left untouched" + ); + } + #[test] fn todo_add_with_recurrence_roundtrips() { let dir = setup_repo(); @@ -2573,6 +2660,55 @@ mod tests { assert_eq!(fm.blocked_by, vec!["todo/dep: thing.md"]); } + #[test] + fn todo_query_horizon_reports_null_date_terminal_row_as_lint_error() { + let dir = setup_repo(); + let today = Clock::from_env().today(); + let recent = today - chrono::Duration::days(1); + let old = today - chrono::Duration::days(300); + + write_task( + &dir, + "todo/nodate.md", + "---\ntldr: NoDate\nstatus: done\n---\n", + ); + write_task( + &dir, + "todo/recent.md", + &format!("---\ntldr: Recent\nstatus: done\non_date: {recent}\n---\n"), + ); + write_task( + &dir, + "todo/old.md", + &format!("---\ntldr: Old\nstatus: done\non_date: {old}\n---\n"), + ); + run_git(dir.path(), &["add", "-A"]); + run_git(dir.path(), &["commit", "-m", "tasks"]); + + let result = todo_query(&QueryOptions { + repo_dir: dir.path().to_path_buf(), + repo_slug: None, + repo_domain: None, + horizon_days: Some(5), + include_done: false, + statuses: vec![Status::Done], + labels: vec![], + limit: None, + }) + .unwrap(); + + let tldrs: Vec<&str> = result.tasks.iter().map(|t| t.tldr.as_str()).collect(); + assert_eq!(tldrs, vec!["Recent"], "window keeps only recent completion"); + assert!( + result + .lint_errors + .iter() + .any(|e| e.path.ends_with("nodate.md")), + "null-date terminal row still surfaces as a lint error: {:?}", + result.lint_errors + ); + } + #[test] fn todo_query_on_legacy_file_returns_data_and_leaves_file_unchanged() { // Once the cache is primed (schema matches), routine tool diff --git a/tests/multi_repo_integration.rs b/tests/multi_repo_integration.rs index a10088d..5282432 100644 --- a/tests/multi_repo_integration.rs +++ b/tests/multi_repo_integration.rs @@ -214,18 +214,18 @@ fn test_null_effective_date_partitioned_multi_repo() { assert_eq!(tasks[0]["tldr"], "Good"); assert_eq!(tasks[0]["repo"], "work"); - // combined.lint_errors: contains the partition error for the broken - // task, tagged with repo: "life". + // combined.lint_errors: the broken task is reported via its persisted + // lint state, tagged with repo: "life". The message names the rule it + // broke rather than the generic no-effective_date partition text. let lint_errors = result["lint_errors"].as_array().unwrap(); - let partition_msg = "item has no effective_date (missing scheduling or completed date)"; let found = lint_errors.iter().any(|e| { e["repo"].as_str() == Some("life") - && e["message"].as_str() == Some(partition_msg) + && e["message"].as_str() == Some("active task missing check_in_date or due_date") && e["path"].as_str() == Some("todo/broken.md") }); assert!( found, - "expected partition lint_error for repo 'life' with path 'todo/broken.md'; got: {lint_errors:?}" + "expected lint_error for repo 'life' with path 'todo/broken.md'; got: {lint_errors:?}" ); } @@ -616,151 +616,236 @@ fn test_reorder_bare_path_with_colon_falls_through() { ); } -#[test] -fn test_multi_repo_sort_matches_single_repo_sql_order() { - // Verify the Rust sort key produces the same ordering as the SQL ORDER BY. - // Put several items with different sort attributes in one repo, query it - // both as single-repo (SQL) and multi-repo (Rust sort), compare order. - let tmp = tempfile::tempdir().unwrap(); - let manifest = tmp.path().join("manifest.toml"); - - let repo1 = - setup_repo_with_config(tmp.path(), "life", "test.org/life", "life", None, &manifest); - let repo2 = - setup_repo_with_config(tmp.path(), "work", "test.org/work", "work", None, &manifest); - - // Work repo needs at least one commit for graf to sync it - add_task( - &repo2, - "todo/placeholder.md", - "---\ntldr: Placeholder\nstatus: done\non_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - - // Items with varied dates, priorities, and sort_orders - add_task( - &repo1, - "todo/jan2-pri5.md", - "---\ntldr: Jan2 pri5\nstatus: todo\npriority: 5\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - add_task( - &repo1, - "todo/jan2-pri1.md", - "---\ntldr: Jan2 pri1\nstatus: todo\npriority: 1\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - add_task( - &repo1, - "todo/jan1.md", - "---\ntldr: Jan1\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - add_task( - &repo1, - "todo/jan2-so.md", - "---\ntldr: Jan2 so0.5\nstatus: todo\npriority: 5\nsort_order: 0.5\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - - // Single-repo query (SQL sort) - let out = graf(&["todo", "--json", "--repo", "life"], &manifest); +/// Extract the ordered `tldr` sequence from a `graf todo --json` run. +fn query_tldrs(args: &[&str], manifest: &Path) -> Vec { + let out = graf(args, manifest); assert!( out.status.success(), - "single-repo query failed: {}", + "query {args:?} failed: {}", String::from_utf8_lossy(&out.stderr) ); - let single: Value = serde_json::from_str(&String::from_utf8(out.stdout).unwrap()).unwrap(); - let single_tldrs: Vec<&str> = single["tasks"] + let v: Value = serde_json::from_str(&String::from_utf8(out.stdout).unwrap()).unwrap(); + v["tasks"] .as_array() .unwrap() .iter() - .map(|t| t["tldr"].as_str().unwrap()) - .collect(); + .map(|t| t["tldr"].as_str().unwrap().to_string()) + .collect() +} - // Multi-repo query (Rust sort) — includes empty work repo - let out = graf(&["todo", "--json"], &manifest); +/// Cross-check the SQL `ORDER BY` against the Rust merge comparator for a set +/// of items, and pin the absolute expected order. +/// +/// `items` is `(filename, frontmatter)`. `repo_of` assigns each item to repo 0 +/// (`life`) or 1 (`work`) for the multi-repo path, where each repo is SQL-sorted +/// and the merged order comes from `todo_sort_cmp`. The SQL oracle places every +/// item in a single repo, so its order is pure SQL `ORDER BY`. Both orderings +/// must equal `expected` — the test fails even if both engines drifted in +/// tandem. Distribute competing items across both repos so the comparator, not +/// SQL, decides their relative order. +fn assert_sql_matches_merge( + query_args: &[&str], + items: &[(&str, &str)], + repo_of: &[usize], + expected: &[&str], +) { + assert_eq!(items.len(), repo_of.len()); assert!( - out.status.success(), - "multi-repo query failed: {}", - String::from_utf8_lossy(&out.stderr) + repo_of.contains(&0) && repo_of.contains(&1), + "comparator is only exercised cross-repo; each repo also needs >=1 commit to sync" ); - let multi: Value = serde_json::from_str(&String::from_utf8(out.stdout).unwrap()).unwrap(); - let multi_tldrs: Vec<&str> = multi["tasks"] - .as_array() - .unwrap() - .iter() - .map(|t| t["tldr"].as_str().unwrap()) - .collect(); + let tmp = tempfile::tempdir().unwrap(); + + // SQL oracle: all items in one repo, pure SQL ORDER BY. + let oracle_manifest = tmp.path().join("oracle.toml"); + let oracle = setup_repo_with_config( + tmp.path(), + "oracle", + "test.org/oracle", + "oracle", + None, + &oracle_manifest, + ); + for (name, fm) in items { + add_task(&oracle, name, fm); + } + let sql_order = query_tldrs(query_args, &oracle_manifest); + + // Rust merge: items split across two repos so the comparator decides + // cross-repo order. + let split_manifest = tmp.path().join("split.toml"); + let life = setup_repo_with_config( + tmp.path(), + "life", + "test.org/life", + "life", + None, + &split_manifest, + ); + let work = setup_repo_with_config( + tmp.path(), + "work", + "test.org/work", + "work", + None, + &split_manifest, + ); + let repos = [&life, &work]; + for ((name, fm), &r) in items.iter().zip(repo_of) { + add_task(repos[r], name, fm); + } + let merge_order = query_tldrs(query_args, &split_manifest); + let sql: Vec<&str> = sql_order.iter().map(String::as_str).collect(); + let merge: Vec<&str> = merge_order.iter().map(String::as_str).collect(); + assert_eq!(sql.as_slice(), expected, "SQL ORDER BY must match expected"); assert_eq!( - single_tldrs, multi_tldrs, - "Rust sort (multi-repo) must match SQL sort (single-repo)" + merge.as_slice(), + expected, + "Rust merge comparator must match expected" ); } #[test] -fn test_multi_repo_sort_unranked_matches_sql() { - // Verify SQL and Rust agree on where unranked items (no sort_order, - // no priority) sort — they should use UNRANKED (6.0), landing after - // all prioritized items. - let tmp = tempfile::tempdir().unwrap(); - let manifest = tmp.path().join("manifest.toml"); - - let repo1 = - setup_repo_with_config(tmp.path(), "life", "test.org/life", "life", None, &manifest); - let repo2 = - setup_repo_with_config(tmp.path(), "work", "test.org/work", "work", None, &manifest); - - add_task( - &repo2, - "todo/placeholder.md", - "---\ntldr: Placeholder\nstatus: done\non_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", +fn test_multi_repo_sort_matches_single_repo_sql_order() { + // Verify the Rust merge comparator produces the same ordering as the SQL + // ORDER BY across effective date, priority rank, and sort_order override. + // Competing jan2 items live in different repos so the comparator, not SQL, + // decides their order. + assert_sql_matches_merge( + &["todo", "--json"], + &[ + ( + "todo/jan1.md", + "---\ntldr: Jan1\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/jan2-so.md", + "---\ntldr: Jan2 so0.5\nstatus: todo\npriority: 5\nsort_order: 0.5\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/jan2-pri1.md", + "---\ntldr: Jan2 pri1\nstatus: todo\npriority: 1\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/jan2-pri5.md", + "---\ntldr: Jan2 pri5\nstatus: todo\npriority: 5\ncheck_in_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ], + // Jan1→life, so0.5→work, pri1→life, pri5→work: every adjacent pair in + // the expected order spans repos. + &[0, 1, 0, 1], + &["Jan1", "Jan2 so0.5", "Jan2 pri1", "Jan2 pri5"], ); +} - // Same date, different rank scenarios - add_task( - &repo1, - "todo/pri1.md", - "---\ntldr: Pri1\nstatus: todo\npriority: 1\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - add_task( - &repo1, - "todo/pri5.md", - "---\ntldr: Pri5\nstatus: todo\npriority: 5\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", - ); - add_task( - &repo1, - "todo/unranked.md", - "---\ntldr: Unranked\nstatus: todo\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", +#[test] +fn test_multi_repo_sort_unranked_matches_sql() { + // Verify SQL and Rust agree on where unranked items (no sort_order, + // no priority) sort — they use UNRANKED (6.0), landing after all + // prioritized items. Same date, so rank alone orders them; competitors + // split across repos so the comparator decides. The unranked item's tldr + // ("A unranked") is byte-order-first, so byte order contradicts rank + // order: only correct rank handling (UNRANKED sorts last) yields the + // expected sequence — dropping the comparator's rank term reorders it. + let expected = ["Pri1", "Pri5", "A unranked"]; + assert_sql_matches_merge( + &["todo", "--json"], + &[ + ( + "todo/pri1.md", + "---\ntldr: Pri1\nstatus: todo\npriority: 1\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/pri5.md", + "---\ntldr: Pri5\nstatus: todo\npriority: 5\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/unranked.md", + "---\ntldr: A unranked\nstatus: todo\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ], + // Pri1→life, Pri5→work, A unranked→life: the Pri5/unranked decision is + // cross-repo. + &[0, 1, 0], + &expected, ); + // Unranked sorts last (rank 6.0 > priority 5) despite its byte-first tldr; + // enforced by the expected order inside assert_sql_matches_merge. +} - // Single-repo (SQL) - let out = graf(&["todo", "--json", "--repo", "life"], &manifest); - assert!(out.status.success()); - let single: Value = serde_json::from_str(&String::from_utf8(out.stdout).unwrap()).unwrap(); - let single_tldrs: Vec<&str> = single["tasks"] - .as_array() - .unwrap() - .iter() - .map(|t| t["tldr"].as_str().unwrap()) - .collect(); - - // Multi-repo (Rust sort) - let out = graf(&["todo", "--json"], &manifest); - assert!(out.status.success()); - let multi: Value = serde_json::from_str(&String::from_utf8(out.stdout).unwrap()).unwrap(); - let multi_tldrs: Vec<&str> = multi["tasks"] - .as_array() - .unwrap() - .iter() - .map(|t| t["tldr"].as_str().unwrap()) - .collect(); +#[test] +fn test_multi_repo_sort_terminal_matches_sql() { + // Terminal items (done/cancelled) must sort after all active items even + // when their effective dates are earlier. SQL uses `status IN + // ('done','cancelled')`; Rust uses `is_terminal()`. Both active and + // terminal items live in both repos, so the comparator decides the + // active/terminal boundary cross-repo. Query with --include-done. + let expected = ["Active life", "Active work", "Done life", "Cancelled work"]; + assert_sql_matches_merge( + &["todo", "--json", "--include-done"], + &[ + ( + "todo/active-life.md", + "---\ntldr: Active life\nstatus: todo\npriority: 3\ncheck_in_date: '2020-06-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/active-work.md", + "---\ntldr: Active work\nstatus: todo\npriority: 3\ncheck_in_date: '2020-06-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + // Terminal items carry on_date (their post-completion effective + // date), set earlier than every active item's date. + ( + "todo/done-life.md", + "---\ntldr: Done life\nstatus: done\non_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/cancelled-work.md", + "---\ntldr: Cancelled work\nstatus: cancelled\non_date: '2020-01-02'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ], + // life holds Active life + Done life; work holds Active work + + // Cancelled work. Placing Active work before Done life despite Done + // life's earlier date exercises terminal-last handling cross-repo. + &[0, 1, 0, 1], + &expected, + ); + // Terminal items sort last, in effective-date order among themselves; + // enforced by the expected order inside assert_sql_matches_merge. +} - assert_eq!( - single_tldrs, multi_tldrs, - "Rust sort must match SQL sort for unranked items" - ); - // Unranked should be last (rank 6.0 > priority 5) - assert_eq!( - *single_tldrs.last().unwrap(), - "Unranked", - "unranked item should sort last" +#[test] +fn test_multi_repo_sort_title_tiebreak_matches_sql() { + // Items identical on status, effective date, and rank, differing only in + // tldr, split across repos and created in non-alphabetical order. The + // uppercase/lowercase pair (`B` / `a`) pins the shared byte-order + // semantics: SQLite's default BINARY collation is memcmp over UTF-8, and + // Rust str Ord is byte-wise, so 'B' (0x42) < 'a' (0x61) on both sides. + assert_sql_matches_merge( + &["todo", "--json"], + &[ + // Added in non-alphabetical order; SQL/Rust must sort by tldr. + ( + "todo/a.md", + "---\ntldr: a task\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/d.md", + "---\ntldr: D task\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/b.md", + "---\ntldr: B task\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ( + "todo/c.md", + "---\ntldr: C task\nstatus: todo\npriority: 3\ncheck_in_date: '2020-01-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ), + ], + // B→life, C→work, D→life, a→work: C-before-D and D-before-`a` (the + // case corner) are both cross-repo decisions. + &[1, 0, 0, 1], + &["B task", "C task", "D task", "a task"], ); } diff --git a/tests/todo_integration.rs b/tests/todo_integration.rs index 5140571..45ee4aa 100644 --- a/tests/todo_integration.rs +++ b/tests/todo_integration.rs @@ -237,9 +237,11 @@ fn test_todo_query_default_no_horizon() { } /// A task missing both `check_in_date` and `due_date` has null -/// effective_date in the cache. `todo_query` must partition such rows -/// into `lint_errors` with the synthesized message, never into `tasks` -/// — see the invariant on `TodoQueryResult`. +/// effective_date in the cache. Such rows stay out of `tasks` (the +/// `TodoQueryResult` invariant) and are reported in `lint_errors` via +/// their persisted lint state, which names the rule they broke. The +/// generic partition message is a last resort and must not appear when +/// persisted state exists. #[test] fn test_null_effective_date_partitioned_to_lint_errors() { let tmp = tempfile::tempdir().unwrap(); @@ -269,23 +271,27 @@ fn test_null_effective_date_partitioned_to_lint_errors() { "null-effective_date row must not appear in tasks: {tasks:?}" ); - // It must appear in lint_errors with the synthesized partition message. + // It must appear in lint_errors exactly once, naming the rule it broke. let lint_errors = result["lint_errors"].as_array().unwrap(); - let partition_msg = "item has no effective_date (missing scheduling or completed date)"; - let found = lint_errors + let broken: Vec<&Value> = lint_errors .iter() - .any(|e| e["path"] == "todo/no-dates.md" && e["message"].as_str() == Some(partition_msg)); - assert!( - found, - "expected partition lint_error with message {partition_msg:?} for todo/no-dates.md; got: {lint_errors:?}" + .filter(|e| e["path"] == "todo/no-dates.md") + .collect(); + assert_eq!( + broken.len(), + 1, + "expected exactly one lint_error for todo/no-dates.md; got: {broken:?}" + ); + assert_eq!( + broken[0]["message"].as_str(), + Some("active task missing check_in_date or due_date") ); } -/// Partition applies identically to terminal (done/cancelled) items -/// that fail their own scheduling-date lint rule ("done/cancelled item -/// missing on_date or end_date"). The `task_row_to_item` conversion is -/// status-agnostic — any null-`effective_date` row is partitioned — so -/// both classes of lint violation surface through the same channel. +/// Terminal (done/cancelled) items that fail their own scheduling-date +/// lint rule are reported the same way. The persisted set is scanned +/// independently of the query's status filter, so the violation surfaces +/// even though the item is out of query scope without `--include-done`. #[test] fn test_null_effective_date_partition_for_terminal_status() { let tmp = tempfile::tempdir().unwrap(); @@ -300,15 +306,9 @@ fn test_null_effective_date_partition_for_terminal_status() { "add broken done", ); - // --include-done so the done item enters the query scope. + // Default status filter: the done item is NOT in query scope. let output = graf_cmd() - .args([ - "todo", - "--json", - "--include-done", - "--repo", - repo.to_str().unwrap(), - ]) + .args(["todo", "--json", "--repo", repo.to_str().unwrap()]) .output() .unwrap(); @@ -319,37 +319,31 @@ fn test_null_effective_date_partition_for_terminal_status() { let tasks = result["tasks"].as_array().unwrap(); assert!( tasks.iter().all(|t| t["path"] != "todo/broken-done.md"), - "null-effective_date terminal row must not appear in tasks: {tasks:?}" + "terminal row must not appear in tasks under the default filter: {tasks:?}" ); - // It must appear in lint_errors with the same synthesized message as - // for active items (the partition message is status-agnostic). + // It is nonetheless reported, with the rule's own message. let lint_errors = result["lint_errors"].as_array().unwrap(); - let partition_msg = "item has no effective_date (missing scheduling or completed date)"; let found = lint_errors.iter().any(|e| { - e["path"] == "todo/broken-done.md" && e["message"].as_str() == Some(partition_msg) + e["path"] == "todo/broken-done.md" + && e["message"].as_str() == Some("done/cancelled item missing on_date or end_date") }); assert!( found, - "expected partition lint_error for todo/broken-done.md; got: {lint_errors:?}" + "expected lint_error for todo/broken-done.md; got: {lint_errors:?}" ); } -/// Running the query twice without changing files still partitions the -/// null-date row, and on the second pass the partition is the ONLY -/// source of the error — sync's incremental pass does not re-lint -/// already-committed clean files. This guards the invariant "every -/// null-effective_date row in the cache produces a `TodoError` on every -/// call, regardless of whether the current sync pass re-linted that file." +/// Running the query twice without changing files reports the same +/// rule-specific error both times, exactly once per pass. /// /// Mechanics: the graf cache persists at `/.graf.db` between CLI -/// invocations. Pass 1 cold-starts with no cache, triggering `do_reindex` -/// which walks every file and produces lint errors in `sync.errors`. Pass 2 -/// finds the cache populated and `last_indexed_commit == HEAD`, so the -/// incremental sync skips both reconcile-dirty (no dirty rows) and the -/// commit-diff step — `sync.errors` is empty. If the partition at -/// `todo_query`'s response-building layer didn't exist, pass 2 would -/// emit no error for the broken file at all. +/// invocations. Pass 1 cold-starts with no cache, triggering `do_reindex`, +/// which lints every file and persists the messages. Pass 2 finds the cache +/// populated and `last_indexed_commit == HEAD`, so the incremental sync +/// skips both reconcile-dirty (no dirty rows) and the commit-diff step — +/// `sync.errors` is empty and the persisted set is the only source. This is +/// the regression the persisted lint state exists to prevent. #[test] fn test_null_effective_date_partitioned_across_queries() { let tmp = tempfile::tempdir().unwrap(); @@ -362,7 +356,8 @@ fn test_null_effective_date_partitioned_across_queries() { "add lint-dirty", ); - let partition_msg = "item has no effective_date (missing scheduling or completed date)"; + let rule_msg = "active task missing check_in_date or due_date"; + let generic_msg = "item has no effective_date (missing scheduling or completed date)"; for pass in 1..=2 { let output = graf_cmd() @@ -385,33 +380,24 @@ fn test_null_effective_date_partitioned_across_queries() { .filter(|e| e["path"] == "todo/no-dates.md") .collect(); - let has_partition = broken_errors - .iter() - .any(|e| e["message"].as_str() == Some(partition_msg)); + // Exactly one error, naming the rule, on both passes. Pass 1 has it + // from sync and the persisted set at once — dedup collapses them. + assert_eq!( + broken_errors.len(), + 1, + "pass {pass}: expected exactly one lint_error for todo/no-dates.md; got: {broken_errors:?}" + ); + assert_eq!( + broken_errors[0]["message"].as_str(), + Some(rule_msg), + "pass {pass}: wrong message" + ); assert!( - has_partition, - "pass {pass}: expected partition lint_error for todo/no-dates.md; got: {lint_errors:?}" + !lint_errors + .iter() + .any(|e| e["message"].as_str() == Some(generic_msg)), + "pass {pass}: generic message must not appear when persisted state exists" ); - - if pass == 2 { - // Invariant test: on the second pass, sync does NOT re-lint - // already-committed files, so the only error source for the - // broken file is the partition at the response-building - // layer. The original lint rule message "active task missing - // check_in_date or due_date" must NOT appear — if it did, it - // would mean sync re-linted (and the partition would be - // redundant rather than load-bearing). - let sync_lint_leaked = broken_errors.iter().any(|e| { - e["message"] - .as_str() - .is_some_and(|m| m.contains("missing check_in_date")) - }); - assert!( - !sync_lint_leaked, - "pass 2: sync should not re-lint clean committed files; \ - partition should be the ONLY source of the error. Got: {broken_errors:?}" - ); - } } } @@ -990,6 +976,13 @@ fn test_todo_done_recurring_advances() { "---\ntldr: Recurring Task\nstatus: todo\npriority: 3\ncheck_in_date: '2026-04-06'\nrrule: \"FREQ=WEEKLY\"\ntentative_date: '2026-04-06'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n\nSome instructions.\n", ).unwrap(); + // Positive control for the lint-freshness assertion below: a file that + // must appear under the same path filter. + std::fs::write( + repo.join("todo/known-bad.md"), + "---\ntldr: Known Bad\nstatus: done\npriority: 3\ncheck_in_date: '2026-04-01'\non_date: '2026-04-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + ).unwrap(); + let output = graf_cmd() .args([ "todo", @@ -1024,6 +1017,40 @@ fn test_todo_done_recurring_advances() { assert!(content.contains("recurrence:")); assert!(content.contains("completion_log:")); assert!(content.contains("Some instructions.")); // body preserved + + // The advanced shape is lint-clean: persisted lint state after the + // mutation carries no entry for the file. + let output = graf_cmd() + .args([ + "todo", + "--json", + "--include-done", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!(output.status.success()); + let result: Value = serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + let messages_for = |path: &str| -> Vec<&str> { + result["lint_errors"] + .as_array() + .unwrap() + .iter() + .filter(|e| e["path"] == path) + .filter_map(|e| e["message"].as_str()) + .collect() + }; + let control = messages_for("todo/known-bad.md"); + assert!( + control.contains(&"done/cancelled item has check_in_date set"), + "positive control missing — the path filter no longer matches: {control:?}" + ); + let messages = messages_for("todo/recurring.md"); + assert!( + messages.is_empty(), + "recurring advance left persisted lint errors: {messages:?}" + ); } #[test] @@ -3679,3 +3706,360 @@ fn test_todo_cancel_with_today_override() { "cancel on_date has buggy UTC date:\n{content}", ); } + +/// A violating file dirty in the working tree is linted by the current sync +/// pass AND carried in the persisted set. The two must collapse to a single +/// reported error rather than double-reporting. +#[test] +fn test_lint_error_deduped_when_file_dirty() { + let tmp = tempfile::tempdir().unwrap(); + let repo = setup_repo(tmp.path()); + + write_and_commit( + &repo, + "todo/bad.md", + "---\ntldr: Bad\nstatus: done\non_date: '2026-04-10'\ncheck_in_date: '2026-05-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + "add violating file", + ); + + // Prime the cache so the violation is persisted. + let primed = graf_cmd() + .args(["todo", "--json", "--repo", repo.to_str().unwrap()]) + .output() + .unwrap(); + assert!(primed.status.success()); + + // Now dirty the file in the working tree, keeping the same violation. + std::fs::write( + repo.join("todo/bad.md"), + "---\ntldr: Bad Edited\nstatus: done\non_date: '2026-04-10'\ncheck_in_date: '2026-05-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-02T00:00:00Z'\n---\n", + ) + .unwrap(); + + let output = graf_cmd() + .args(["todo", "--json", "--repo", repo.to_str().unwrap()]) + .output() + .unwrap(); + assert!(output.status.success()); + let result: Value = serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + + let lint_errors = result["lint_errors"].as_array().unwrap(); + let occurrences = lint_errors + .iter() + .filter(|e| { + e["path"] == "todo/bad.md" + && e["message"].as_str() == Some("done/cancelled item has check_in_date set") + }) + .count(); + assert_eq!( + occurrences, 1, + "persisted and current-pass errors must collapse; got: {lint_errors:?}" + ); +} + +/// A horizon-bounded query excludes the violating row from `tasks`, but the +/// persisted set is scanned independently of the filter, so the violation is +/// still reported. +#[test] +fn test_horizon_does_not_silence_lint_errors() { + let tmp = tempfile::tempdir().unwrap(); + let repo = setup_repo(tmp.path()); + + write_and_commit( + &repo, + "todo/no-dates.md", + "---\ntldr: No Dates Task\nstatus: todo\npriority: 3\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + "add lint-dirty", + ); + + // Two passes: the second is quiescent, with no sync errors to fall back on. + for pass in 1..=2 { + let output = graf_cmd() + .args([ + "todo", + "--json", + "--horizon", + "7", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!(output.status.success(), "pass {pass} failed"); + let result: Value = + serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + + let tasks = result["tasks"].as_array().unwrap(); + assert!( + tasks.iter().all(|t| t["path"] != "todo/no-dates.md"), + "pass {pass}: null-date row must not appear in tasks" + ); + + let lint_errors = result["lint_errors"].as_array().unwrap(); + let found = lint_errors.iter().any(|e| { + e["path"] == "todo/no-dates.md" + && e["message"].as_str() == Some("active task missing check_in_date or due_date") + }); + assert!( + found, + "pass {pass}: horizon must not silence the violation; got: {lint_errors:?}" + ); + } +} + +/// Statusless knowledge-base docs are never returned by `query_tasks`, but +/// their date-exclusivity violations still reach `lint_errors` through the +/// persisted set. +#[test] +fn test_non_task_doc_lint_surfaces_in_query() { + let tmp = tempfile::tempdir().unwrap(); + let repo = setup_repo(tmp.path()); + + write_and_commit( + &repo, + "kb-entry.md", + "---\ntldr: KB Entry\nend_date: '2026-04-15'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\nBody.\n", + "add kb entry", + ); + + let output = graf_cmd() + .args(["todo", "--json", "--repo", repo.to_str().unwrap()]) + .output() + .unwrap(); + assert!(output.status.success()); + let result: Value = serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + + let tasks = result["tasks"].as_array().unwrap(); + assert!( + tasks.iter().all(|t| t["path"] != "kb-entry.md"), + "statusless doc must not appear in tasks; got: {tasks:?}" + ); + + let lint_errors = result["lint_errors"].as_array().unwrap(); + let found = lint_errors.iter().any(|e| { + e["path"] == "kb-entry.md" && e["message"].as_str() == Some("end_date requires start_date") + }); + assert!( + found, + "statusless doc's violation must surface; got: {lint_errors:?}" + ); +} + +/// The mutation paths (`add`, `schedule`, `done`, `cancel`) write frontmatter +/// through `upsert_doc`, which recomputes persisted lint state. A mutation +/// that wrote a lint-tripping shape would leave a violation reported on every +/// later query, so each family asserts a clean path after mutating. +/// +/// A deliberately violating file provides the positive control: the same +/// filter must match something, otherwise the clean-path assertions are +/// vacuously true. +#[test] +fn test_mutation_paths_leave_no_persisted_lint_errors() { + let tmp = tempfile::tempdir().unwrap(); + let repo = setup_repo(tmp.path()); + + write_and_commit( + &repo, + "todo/known-bad.md", + "---\ntldr: Known Bad\nstatus: done\npriority: 3\ncheck_in_date: '2026-04-01'\non_date: '2026-04-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + "add known-bad", + ); + + let lint_errors_for = |path: &str| -> Vec { + let output = graf_cmd() + .args([ + "todo", + "--json", + "--include-done", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let result: Value = + serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + result["lint_errors"] + .as_array() + .unwrap() + .iter() + .filter(|e| e["path"] == path) + .cloned() + .collect() + }; + + let run = |args: &[&str]| { + let mut full = vec!["todo"]; + full.extend_from_slice(args); + full.extend_from_slice(&["--repo", repo.to_str().unwrap()]); + let output = graf_cmd().args(&full).output().unwrap(); + assert!( + output.status.success(), + "{args:?} failed: {} {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + }; + + // Positive control: the filter and the path format it matches on both + // work, so the emptiness assertions below mean something. + let control: Vec = lint_errors_for("todo/known-bad.md") + .iter() + .filter_map(|e| e["message"].as_str().map(str::to_string)) + .collect(); + assert!( + control + .iter() + .any(|m| m == "done/cancelled item has check_in_date set"), + "positive control missing — the path filter no longer matches: {control:?}" + ); + + run(&[ + "add", + "todo/fresh.md", + "Fresh Task", + "--priority", + "2", + "--check-in", + "2026-05-01", + ]); + let after_add = lint_errors_for("todo/fresh.md"); + assert!( + after_add.is_empty(), + "add wrote a lint-tripping shape: {after_add:?}" + ); + + run(&["schedule", "todo/fresh.md", "2026-06-01"]); + let after_schedule = lint_errors_for("todo/fresh.md"); + assert!( + after_schedule.is_empty(), + "schedule wrote a lint-tripping shape: {after_schedule:?}" + ); + + run(&["done", "todo/fresh.md", "--completion-date", "2026-06-01"]); + let after_done = lint_errors_for("todo/fresh.md"); + assert!( + after_done.is_empty(), + "done wrote a lint-tripping shape: {after_done:?}" + ); + + run(&[ + "add", + "todo/cancel-me.md", + "Cancel Me", + "--priority", + "2", + "--check-in", + "2026-05-01", + ]); + run(&["cancel", "todo/cancel-me.md"]); + let after_cancel = lint_errors_for("todo/cancel-me.md"); + assert!( + after_cancel.is_empty(), + "cancel wrote a lint-tripping shape: {after_cancel:?}" + ); +} + +/// Anchored terminal completion preserves `check_in_date` and the recurrence +/// block, frozen at the final occurrence. `check_rules` exempts that shape, +/// so a real drive to exhaustion leaves no persisted lint errors. +#[test] +fn test_anchored_exhaustion_leaves_no_persisted_lint_errors() { + let tmp = tempfile::tempdir().unwrap(); + let repo = setup_repo(tmp.path()); + + write_and_commit( + &repo, + "todo/anchored-finite.md", + "---\ntldr: Anchored Finite\nstatus: todo\npriority: 3\ncheck_in_date: '2026-01-03'\nrrule: \"FREQ=WEEKLY;BYDAY=SA;UNTIL=20260228T000000Z\"\ntentative_date: '2026-01-03'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + "add anchored finite", + ); + + // Positive control for the lint assertion below: a file that must appear + // under the same path filter. + write_and_commit( + &repo, + "todo/known-bad.md", + "---\ntldr: Known Bad\nstatus: done\npriority: 3\ncheck_in_date: '2026-04-01'\non_date: '2026-04-01'\ncreated: '2025-01-01T00:00:00Z'\nupdated: '2025-01-01T00:00:00Z'\n---\n", + "add known-bad", + ); + + let output = graf_cmd() + .args([ + "todo", + "--json", + "done", + "todo/anchored-finite.md", + "--completion-date", + "2026-05-01", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = graf_cmd() + .args([ + "todo", + "--json", + "--include-done", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!(output.status.success()); + let result: Value = serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + let messages_for = |path: &str| -> Vec<&str> { + result["lint_errors"] + .as_array() + .unwrap() + .iter() + .filter(|e| e["path"] == path) + .filter_map(|e| e["message"].as_str()) + .collect() + }; + let control = messages_for("todo/known-bad.md"); + assert!( + control.contains(&"done/cancelled item has check_in_date set"), + "positive control missing — the path filter no longer matches: {control:?}" + ); + let messages = messages_for("todo/anchored-finite.md"); + + assert!( + messages.is_empty(), + "frozen series must be lint-clean; got: {messages:?}" + ); + + // The lint gate no longer blocks mutations on the frozen file: `done` + // reaches its own terminal-status rejection. + let output = graf_cmd() + .args([ + "todo", + "--json", + "done", + "todo/anchored-finite.md", + "--completion-date", + "2026-05-02", + "--repo", + repo.to_str().unwrap(), + ]) + .output() + .unwrap(); + let stdout = String::from_utf8(output.stdout).unwrap(); + let resp: Value = serde_json::from_str(&stdout).unwrap(); + let reason = resp["reason"].as_str().unwrap(); + assert!( + reason.contains("already has terminal status"), + "expected the terminal-status rejection, not the lint gate; got: {reason}" + ); +}