From 262ff6395c4d24b4bb7064ad06c5d71472d7547d Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 14:53:18 -0400 Subject: [PATCH 1/9] chore(todo): mark flat-rrule removal BLOCKED, delete 5 won't-do TODOs Per TODO burndown triage 2026-07-20. - Mark recurring-phase-2-remove-flat-rrule BLOCKED as of 2026-07-20 - Delete from TODO.md and remove in-code markers for: done-error-taxonomy-cleanup, done-error-derive-serialize, mcp-preflight-shared-error-type, done-anchored-double-rrule-parse, workspace-handle - Reword done-error-grouping-comment to drop the dangling slug reference Claude-Session: https://claude.ai/code/session_011gDGijCtMBrvxhotRnSxf7 --- TODO.md | 22 +--------------------- src/db.rs | 3 --- src/todo.rs | 20 +------------------- 3 files changed, 2 insertions(+), 43 deletions(-) diff --git a/TODO.md b/TODO.md index 33372dc..eb801d3 100644 --- a/TODO.md +++ b/TODO.md @@ -1,21 +1,5 @@ # 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." @@ -44,11 +28,7 @@ The SQL strings `status IN ('done', 'cancelled')` appear in the ORDER BY and hor `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`. diff --git a/src/db.rs b/src/db.rs index f43f241..d774a54 100644 --- a/src/db.rs +++ b/src/db.rs @@ -82,9 +82,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, diff --git a/src/todo.rs b/src/todo.rs index 4d60db1..d6e863a 100644 --- a/src/todo.rs +++ b/src/todo.rs @@ -632,19 +632,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 +722,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 +998,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)?; From 06471e6bb80afd53a6db83c0908e143869aace58 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 16:56:34 -0400 Subject: [PATCH 2/9] fix(lint): persist lint errors in the cache DB Lint errors were computed at query time and discarded, so callers that read the cache saw a partial picture. Persist them alongside the indexed entries, dedup at the read path, and bump SCHEMA_VERSION. Frozen recurring series are exempted from terminal-status rules: the phase-2 recurrence design forecloses stripping their rrule, so the terminal rules would otherwise emit permanent false positives. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 25 +- src/db.rs | 413 +++++++++++++++++++++++-- src/lint.rs | 402 +++++++++++++++--------- src/todo.rs | 79 +++-- tests/multi_repo_integration.rs | 10 +- tests/todo_integration.rs | 520 +++++++++++++++++++++++++++----- 6 files changed, 1168 insertions(+), 281 deletions(-) diff --git a/TODO.md b/TODO.md index eb801d3..aa43b80 100644 --- a/TODO.md +++ b/TODO.md @@ -36,12 +36,27 @@ The SQL strings `status IN ('done', 'cancelled')` appear in the ORDER BY and hor `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` +## `persist-parse-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. +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. -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. +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. -`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. +## `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/db.rs b/src/db.rs index d774a54..be0fd53 100644 --- a/src/db.rs +++ b/src/db.rs @@ -16,18 +16,23 @@ 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. +pub(crate) const SCHEMA_VERSION: &str = "7"; /// 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)"; const SCHEMA_DDL: &str = " @@ -53,6 +58,7 @@ 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 ); @@ -163,16 +169,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 }); } @@ -200,7 +211,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() @@ -343,6 +354,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")?; @@ -447,20 +478,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)) } // --------------------------------------------------------------------------- @@ -639,6 +676,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:#}"))); } @@ -676,6 +714,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:#}"))); @@ -743,21 +782,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 over +/// a handful of `Option`s. 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, @@ -770,6 +820,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")?, @@ -788,6 +839,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, ], )?; @@ -1967,4 +2019,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/lint.rs b/src/lint.rs index cc7a87e..36af4af 100644 --- a/src/lint.rs +++ b/src/lint.rs @@ -45,52 +45,90 @@ 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 message `check_rules` can emit. The 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. +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, +]; + +/// 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`. +/// +/// 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,10 +140,7 @@ pub fn check_rules(path: &Path, fm: &Frontmatter) -> Vec { ) && fm.check_in_date.is_none() && fm.due_date.is_none() { - errors.push(FileError::new( - path, - "active task missing check_in_date or due_date".to_string(), - )); + emit(&mut errors, path, MSG_ACTIVE_NO_DATE); } errors @@ -325,6 +360,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, + "7", + "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 +1254,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 +1269,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 +1284,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 +1299,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,30 +1310,9 @@ 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()); diff --git a/src/todo.rs b/src/todo.rs index d6e863a..b6dc688 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,20 @@ 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. /// /// 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). @@ -142,25 +141,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); + } + } } } @@ -1100,7 +1125,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 { diff --git a/tests/multi_repo_integration.rs b/tests/multi_repo_integration.rs index a10088d..4c162ee 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:?}" ); } 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}" + ); +} From 5378113d10ba2909ba9bc33d0382cbdd1608c975 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 17:13:59 -0400 Subject: [PATCH 3/9] feat(query): window terminal items by completion date under --horizon Terminal items were windowed by their effective date, so a task completed long ago could still surface (or a recently finished one drop out) depending on scheduling fields that no longer mean anything once the item is done. Window them by completion date instead. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 4 - src/cli/mcp.rs | 3 +- src/cli/todo.rs | 3 +- src/db.rs | 341 +++++++++++++++++++++++++++++++++++++++++++----- src/todo.rs | 56 +++++++- 5 files changed, 364 insertions(+), 43 deletions(-) diff --git a/TODO.md b/TODO.md index aa43b80..2b3b3fe 100644 --- a/TODO.md +++ b/TODO.md @@ -16,10 +16,6 @@ The SQL `ORDER BY` in `db.rs` and the Rust `todo_sort_cmp()` in `cli/mod.rs` enc `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. diff --git a/src/cli/mcp.rs b/src/cli/mcp.rs index cea9a53..da3f2ef 100644 --- a/src/cli/mcp.rs +++ b/src/cli/mcp.rs @@ -449,7 +449,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", diff --git a/src/cli/todo.rs b/src/cli/todo.rs index 3165f8f..37990f8 100644 --- a/src/cli/todo.rs +++ b/src/cli/todo.rs @@ -14,7 +14,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, diff --git a/src/db.rs b/src/db.rs index be0fd53..2b9cd77 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; @@ -100,8 +100,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. @@ -297,24 +300,47 @@ 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); + let p_upper = qb.push_param(upper.to_string()); + let p_lower = qb.push_param(lower.to_string()); // TODO(sql-terminal-status-strings) qb.push_str(&format!( - " AND (status IN ('done', 'cancelled') \ - OR ({EFF_DATE} IS NOT NULL AND {EFF_DATE} <= {p}))" + " AND ((status IN ('done', 'cancelled') \ + AND ({EFF_DATE} IS NULL OR {EFF_DATE} >= {p_lower})) \ + OR (status NOT IN ('done', 'cancelled') \ + AND {EFF_DATE} IS NOT NULL AND {EFF_DATE} <= {p_upper}))" )); } @@ -1288,8 +1314,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( @@ -1303,39 +1346,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:?}" @@ -1346,13 +1392,236 @@ mod tests { ); assert!( tldrs.contains(&"Done"), - "done item exempt from horizon: {tldrs:?}" + "terminal item completed within lookback: {tldrs:?}" + ); + assert!( + !tldrs.contains(&"Cancelled"), + "terminal item completed before lookback excluded: {tldrs:?}" ); assert!( - tldrs.contains(&"Cancelled"), - "cancelled item exempt from horizon: {tldrs:?}" + tldrs.contains(&"CancelledRecent"), + "cancelled item terminated within lookback: {tldrs:?}" ); - assert_eq!(tldrs.len(), 3); + 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] diff --git a/src/todo.rs b/src/todo.rs index b6dc688..ed812cd 100644 --- a/src/todo.rs +++ b/src/todo.rs @@ -31,6 +31,10 @@ use crate::identity::{GlobalId, Slug}; /// 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` @@ -104,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, @@ -2582,6 +2587,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 From e91a8ef5a228164e0c625a989ed6118560706799 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 18:05:41 -0400 Subject: [PATCH 4/9] feat(lint): flag unparseable rrules The migration/reindex path accepted rrule strings that the rrule crate cannot parse, so malformed recurrences silently produced no occurrences. Lint now flags unparseable rrules, rejects newline-injected values that could smuggle extra ICS properties, and pins the crate's error text with a canary so a crate upgrade that changes wording trips the SCHEMA_VERSION gate. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 4 -- src/db.rs | 54 +++++++++++++++-- src/lint.rs | 167 +++++++++++++++++++++++++++++++++++++++++++++++++--- src/todo.rs | 113 +++++++++++++++++++++++++++++------ 4 files changed, 302 insertions(+), 36 deletions(-) diff --git a/TODO.md b/TODO.md index 2b3b3fe..83a8b3d 100644 --- a/TODO.md +++ b/TODO.md @@ -28,10 +28,6 @@ The SQL strings `status IN ('done', 'cancelled')` appear in the ORDER BY and hor `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. - ## `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. diff --git a/src/db.rs b/src/db.rs index 2b9cd77..75a4512 100644 --- a/src/db.rs +++ b/src/db.rs @@ -18,8 +18,12 @@ use crate::lint; /// 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. -pub(crate) const SCHEMA_VERSION: &str = "7"; +/// 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 @@ -813,8 +817,8 @@ fn parse_validate_and_migrate( /// 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 over -/// a handful of `Option`s. +/// `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, @@ -2184,6 +2188,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(); diff --git a/src/lint.rs b/src/lint.rs index 36af4af..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; @@ -56,12 +56,17 @@ 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 message `check_rules` can emit. The 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. +/// 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, @@ -72,6 +77,16 @@ const RULE_MESSAGES: [&str; 7] = [ 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 @@ -92,7 +107,10 @@ fn emit(errors: &mut Vec, path: &Path, msg: &'static str) { /// `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`. +/// 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 @@ -143,6 +161,24 @@ pub fn check_rules(path: &Path, fm: &Frontmatter) -> Vec { 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, + format!("unparseable rrule '{rrule}': {}", e.root_cause()), + )); + } + errors } @@ -541,7 +577,7 @@ mod tests { ); assert_eq!( crate::db::SCHEMA_VERSION, - "7", + "8", "SCHEMA_VERSION changed — update this canary alongside it" ); } @@ -1318,6 +1354,119 @@ mod tests { 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/todo.rs b/src/todo.rs index ed812cd..cd73b80 100644 --- a/src/todo.rs +++ b/src/todo.rs @@ -1620,14 +1620,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)) @@ -1637,9 +1649,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. @@ -1675,17 +1696,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 @@ -2281,6 +2292,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(); From c2366251722fe1ba1524256d473a7c56dae6e2b0 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 18:22:24 -0400 Subject: [PATCH 5/9] refactor(cli): thread manifest through resolver family for test isolation The resolver family read the manifest from ambient process environment, so tests could not exercise it in isolation and risked leaking each other's state. Thread an explicit manifest through the resolver call chain; tests now inject in-memory manifests over TempDir instead of mutating the environment. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 18 +++- src/cli/fix.rs | 4 +- src/cli/lint.rs | 9 +- src/cli/mcp.rs | 17 +++- src/cli/mod.rs | 232 +++++++++++++++++++++++++++++++++++++-------- src/cli/reindex.rs | 4 +- src/cli/todo.rs | 25 +++-- 7 files changed, 252 insertions(+), 57 deletions(-) diff --git a/TODO.md b/TODO.md index 83a8b3d..c7a91b0 100644 --- a/TODO.md +++ b/TODO.md @@ -20,10 +20,6 @@ The SQL `ORDER BY` in `db.rs` and the Rust `todo_sort_cmp()` in `cli/mod.rs` enc 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. - ## `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`. @@ -35,6 +31,20 @@ Lint-rule violations are persisted per-doc in `docs.lint_errors`, so `graf todo` 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. + ## `frozen-series-effective-date` `EFF_DATE` in `src/db.rs` resolves an item's effective date as 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/mcp.rs b/src/cli/mcp.rs index da3f2ef..8f93388 100644 --- a/src/cli/mcp.rs +++ b/src/cli/mcp.rs @@ -32,23 +32,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(), + ) } } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 34c76fe..96cd665 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(_) => { @@ -730,12 +747,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 +794,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 +804,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 +815,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 37990f8..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)] @@ -113,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; @@ -148,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 { @@ -276,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, @@ -365,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, @@ -390,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, @@ -431,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 From faeb38ffe3f87672bf40853b4ce8cacff74410d1 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 18:50:58 -0400 Subject: [PATCH 6/9] feat(manifest): validate cross-repo refs at check time Cross-repo refs were never validated, so a manifest could point at a global ID that no repo actually defines (or that two repos both claim) and nothing flagged it until a downstream lookup silently failed. Validate refs at check time: report dangling refs, and treat a duplicate global ID within a repo's own refs as an error. Ref IDs are sanitized before they reach check output so untrusted values can't inject control characters. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 4 - src/cli/manifest.rs | 12 +- src/cli/mod.rs | 10 +- src/identity.rs | 19 +- src/manifest.rs | 825 +++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 798 insertions(+), 72 deletions(-) diff --git a/TODO.md b/TODO.md index c7a91b0..db1c3b9 100644 --- a/TODO.md +++ b/TODO.md @@ -1,9 +1,5 @@ # TODOs -## `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. 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/mod.rs b/src/cli/mod.rs index 96cd665..ae6da1a 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -373,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(), }) 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/manifest.rs b/src/manifest.rs index cb97327..f88c757 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. /// @@ -191,6 +200,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 +219,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 +298,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 +317,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 +328,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 +355,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 +836,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 +852,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 +876,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 +900,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] From 783c91fc3fcf6bacc8549da06836a616549a1c74 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 19:08:49 -0400 Subject: [PATCH 7/9] refactor(sort): share one Rust rank function; harden multi-repo sort tests The item sort rank was computed in two places (Rust and SQL) that could silently drift, and the multi-repo sort tests never actually exercised the Rust min_by rank path. Collapse to one shared rank function, and rework the tests to split competitors cross-repo so the ranked path is discriminative against byte order and guarded by an SQL oracle. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- TODO.md | 4 - src/cli/mod.rs | 9 +- src/db.rs | 6 +- src/frontmatter.rs | 29 +++ src/todo.rs | 6 +- tests/multi_repo_integration.rs | 333 ++++++++++++++++++++------------ 6 files changed, 247 insertions(+), 140 deletions(-) diff --git a/TODO.md b/TODO.md index db1c3b9..0e290b7 100644 --- a/TODO.md +++ b/TODO.md @@ -1,9 +1,5 @@ # TODOs -## `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. diff --git a/src/cli/mod.rs b/src/cli/mod.rs index ae6da1a..373daf6 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -408,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 diff --git a/src/db.rs b/src/db.rs index 75a4512..2eb675d 100644 --- a/src/db.rs +++ b/src/db.rs @@ -68,7 +68,7 @@ CREATE TABLE IF NOT EXISTS docs ( 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 + COALESCE(sort_order, priority, 6) -- must match frontmatter::rank(); 6 = frontmatter::UNRANKED ) WHERE status IN ('todo', 'in_progress', 'reminder'); CREATE INDEX IF NOT EXISTS idx_status ON docs(status, on_date, end_date); @@ -356,7 +356,9 @@ impl Cache { )); } - // TODO(sort-key-duplication): this must match todo_sort_cmp() in cli/mod.rs + // 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. // TODO(sql-terminal-status-strings) // ORDER BY: active items first, then by effective date, rank, title. qb.push_str(&format!( diff --git a/src/frontmatter.rs b/src/frontmatter.rs index 0d5e76f..c2e2de4 100644 --- a/src/frontmatter.rs +++ b/src/frontmatter.rs @@ -169,6 +169,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 @@ -1477,4 +1486,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/todo.rs b/src/todo.rs index cd73b80..089b212 100644 --- a/src/todo.rs +++ b/src/todo.rs @@ -1508,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) } } diff --git a/tests/multi_repo_integration.rs b/tests/multi_repo_integration.rs index 4c162ee..5282432 100644 --- a/tests/multi_repo_integration.rs +++ b/tests/multi_repo_integration.rs @@ -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"], ); } From 9a63028e74dd04a7b5f64aa8e99c94da951edf93 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 20:50:41 -0400 Subject: [PATCH 8/9] refactor(config): cache MCP config, extract graf_config_dir helper MCP config was re-read and re-parsed on every access, and the XDG config directory path (including the graf/ segment) was assembled independently at multiple call sites. Cache the parsed MCP config once, and route every caller through a single graf_config_dir() helper that owns the full path including the graf/ segment. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- Makefile | 1 + TODO.md | 32 ++++++++++++++++++------- src/cli/mcp.rs | 14 +++++++---- src/config.rs | 63 ++++++++++++++++++++++++++++++++++++++++--------- src/manifest.rs | 10 ++------ 5 files changed, 89 insertions(+), 31 deletions(-) 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 0e290b7..d081467 100644 --- a/TODO.md +++ b/TODO.md @@ -1,13 +1,5 @@ # TODOs -## `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. - ## `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. @@ -37,6 +29,30 @@ 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 diff --git a/src/cli/mcp.rs b/src/cli/mcp.rs index 8f93388..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 { @@ -86,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")) @@ -487,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, @@ -1217,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(), }) } @@ -1323,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/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/manifest.rs b/src/manifest.rs index f88c757..8609763 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -133,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. From 5fa5adf97f4ddf257b8371f486a552a51948fcc7 Mon Sep 17 00:00:00 2001 From: Randall Nortman Date: Mon, 20 Jul 2026 21:16:33 -0400 Subject: [PATCH 9/9] refactor(db): derive terminal/active status SQL from Status enum Terminal and active status string literals were hand-written into multiple SQL fragments, independent of the Status enum, so adding a status variant could silently desync the SQL from the enum. Derive the SQL literals from Status (via TERMINAL_STATUSES_SQL and Status::ALL), add a bridging test that trips when the SQL and enum diverge, and re-pin the frozen on-disk status spellings. Claude-Session: https://claude.ai/code/session_012E14JY77xhbbUcZ9gMf6qA --- CLAUDE.md | 2 +- TODO.md | 4 --- src/bin/migrate_recurrence.rs | 5 +-- src/db.rs | 56 ++++++++++++++++++++++++------- src/frontmatter.rs | 62 ++++++++++++++++++++++++++++++----- 5 files changed, 100 insertions(+), 29 deletions(-) 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/TODO.md b/TODO.md index d081467..b1309c0 100644 --- a/TODO.md +++ b/TODO.md @@ -1,9 +1,5 @@ # TODOs -## `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. - ## `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`. 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/db.rs b/src/db.rs index 2eb675d..4f918dd 100644 --- a/src/db.rs +++ b/src/db.rs @@ -39,6 +39,10 @@ pub(crate) const SCHEMA_VERSION: &str = "8"; /// `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, @@ -69,7 +73,7 @@ CREATE TABLE IF NOT EXISTS docs ( 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) -- must match frontmatter::rank(); 6 = frontmatter::UNRANKED -) WHERE status IN ('todo', 'in_progress', 'reminder'); +) 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); "; @@ -282,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() { @@ -339,11 +343,10 @@ impl Cache { let p_upper = qb.push_param(upper.to_string()); let p_lower = qb.push_param(lower.to_string()); - // TODO(sql-terminal-status-strings) qb.push_str(&format!( - " AND ((status IN ('done', 'cancelled') \ + " AND ((status IN {TERMINAL_STATUSES_SQL} \ AND ({EFF_DATE} IS NULL OR {EFF_DATE} >= {p_lower})) \ - OR (status NOT IN ('done', 'cancelled') \ + OR (status NOT IN {TERMINAL_STATUSES_SQL} \ AND {EFF_DATE} IS NOT NULL AND {EFF_DATE} <= {p_upper}))" )); } @@ -359,10 +362,9 @@ impl Cache { // 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. - // TODO(sql-terminal-status-strings) // 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 @@ -889,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]) { diff --git a/src/frontmatter.rs b/src/frontmatter.rs index c2e2de4..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) @@ -566,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#"--- @@ -686,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)); } }