Todo burndown

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.
 

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
 

TODO.md

@@ -1,67 +1,68 @@
 # TODOs
 
-## `done-error-taxonomy-cleanup`
-
-`DoneError::Invalid(String)` and `DoneError::Infra(anyhow::Error)` (src/todo.rs) are two catch-all variants that every caller must special-case: the MCP layer routes `Infra` to `Err(e)` while emitting the rest as `Ok(envelope)`; CLI JSON mode does the same; CLI human mode stringifies everything. Consider splitting `Invalid` into a small set of named variants (path-not-md, replacing-with-no-stored, replacing-redundant, missing-anchor) so frontends can branch on named codes, and keeping `Infra` as the sole catch-all with a `DoneError::is_infra()` predicate so callers don't pattern-match on the specific variant.
-
-## `done-error-derive-serialize`
-
-`DoneError::to_json` (src/todo.rs) hand-rolls the `{error, reason, ...payload}` envelope by mutating a `serde_json::Value`. A `#[derive(Serialize)]` with `#[serde(tag = "error", rename_all = "snake_case")]` would replace the 80-line `code()` / `to_json()` pair with framework-driven serialization; the `reason` string can stay computed in a `reason()` accessor. Phase 3 added `ScheduleError` with the same hand-rolled idiom (code/reason/to_json/Display/Error/From<anyhow>) — the pattern is now load-bearing for two error types, with a likely third (`ReorderError`, `CancelError`) each time a mutator grows structured errors. Sweep both (all) enums together; the refactor is purely mechanical, no semantic change.
-
-## `done-anchored-double-rrule-parse`
-
-`done_anchored` in src/todo.rs parses and iterates the rrule twice on the stale-anchor branch — once via `compute_next_occurrence` for the naive next anchor, once via `compute_occurrence_after` for the skip-past target. A single helper that returns both values in one iteration would halve the parse+iterate work. Once per user action, so the impact is negligible; cleanup only.
-
-## `workspace-handle`
-
-Multiple callers carry `repo_dir` in their Options structs and independently call `open_and_sync`. Consider a higher-level `Workspace` handle that wraps `repo_dir` + `Cache`, so callers share a single entry point. See `db.rs`.
-
-## `cross-repo-ref-validation`
-
-`graf manifest check` validates manifest-level consistency (slugs unique, IDs match, paths exist) but does not validate cross-repo refs. A repo's `.graf/config.toml` `refs` section maps slugs to global IDs — `check` could verify those IDs exist in the application manifest and that the slugs don't conflict. Deferred per design doc: "Cross-repo ref validation (do slugs in refs resolve?) is deferred."
-
-## `sort-key-duplication`
-
-The SQL `ORDER BY` in `db.rs` and the Rust `todo_sort_cmp()` in `cli/mod.rs` encode the same sort logic independently. The Rust comparator is used for k-way merge of multi-repo results; the SQL ORDER BY handles single-repo. They must agree. The integration test `test_multi_repo_sort_matches_single_repo_sql_order` catches divergence. Consider generating the sort key from a shared definition to make divergence impossible.
-
-## `mcp-config-caching`
-
-`AppConfig::load_default()` is called on every MCP `graf_todo_query` tool call, re-reading and parsing the config file from disk each time. The MCP server is long-lived and config won't change mid-session. Load the config once at server startup and store it in `McpContext`, then reference it from tool `call()` methods without per-request I/O.
-
-## `xdg-path-duplication`
-
-`AppConfig::default_path()` and `AppManifest::default_path()` both implement the same XDG config directory logic (`XDG_CONFIG_HOME` → fallback `~/.config`). Extract a shared `graf_config_dir()` helper when a third caller appears.
-
-## `done-horizon`
-
-`--include-done --horizon N` exempts all terminal items from the horizon filter, returning every done/cancelled task regardless of when it was completed. For a user with many historical completed items this could produce a large result set. Consider adding a separate horizon for terminal items — e.g., only include items completed within the last N days — or filtering terminal items by `on_date`/`end_date` relative to the horizon cutoff. See the horizon exemption in `db.rs` `query_tasks()`.
-
-## `sql-terminal-status-strings`
-
-The SQL strings `status IN ('done', 'cancelled')` appear in the ORDER BY and horizon WHERE clauses of `db.rs`. These are hardcoded string literals that must stay in sync with `Status::Done` and `Status::Cancelled` serialization. If a new terminal status were added, these would need manual updates found only by text search. Consider building the SQL fragment from `Status::is_terminal()` variants or a shared constant.
-
-## `resolve-target-env-isolation`
-
-`resolve_target` unit tests call the real `AppManifest::load_default()`, so results depend on whether the user's `~/.config/graf/manifest.toml` exists and what slugs it contains. The function should accept a manifest (or manifest loader) as a parameter instead of reading the global config implicitly. This would also make `resolve_target` more testable in general. See `cli/mod.rs` `resolve_target()` and its tests.
-
-## `mcp-preflight-shared-error-type`
-
-`DoneError::PathNotFound`/`InvalidDate` (`src/todo.rs`) and `ScheduleError::PathNotFound`/`InvalidDate` (`src/todo.rs`) are structurally identical variants with identical `code()`, `reason()`, and `to_json()` arms. When `cancel-mcp-envelope` lands a third copy will appear. Extract a shared `McpPreflightError` enum (in `src/cli/mcp.rs` or a new `src/cli/mcp_errors.rs`) and embed it via a variant in each tool-specific error enum. Best tackled alongside `done-error-derive-serialize`. See `TODO(mcp-preflight-shared-error-type)` comment at `src/todo.rs`.
-
-## `recurring-phase-2-remove-flat-rrule`
+## `recurring-phase-2-remove-flat-rrule` — BLOCKED as of 2026-07-20
 
 `Frontmatter.rrule: Option<String>` is retained as a legacy-shape compat field (`src/frontmatter.rs`). Reindex eagerly migrates old files into the nested `recurrence` block, and no write path emits flat `rrule`. Once we're confident no repos still carry legacy-shape files (or are willing to surface them as parse errors for manual fix-up), remove the field and the associated `Field::Rrule` variant, plus the legacy-compat branches in `maybe_migrate_legacy` and `effective_rrule`.
 
-## `migration-rrule-syntax-validation`
-
-`maybe_migrate_legacy` in `src/migrate.rs` rewrites legacy frontmatter without validating that the `rrule` string is parseable. A file with a broken `rrule` (e.g. `FREQ=BOGUS`) migrates into the nested shape still broken; runtime breakage surfaces only when `compute_next_occurrence` is invoked. The intended behavior is that a `FREQ=BOGUS` rrule records a `FileError` and leaves the file untouched, but neither lint nor parse validates rrule syntax elsewhere, so migration isn't the natural place to add enforcement. Consider adding an rrule-syntax lint rule that applies uniformly; migration would then surface the issue via the post-rewrite lint pass.
-
-## `ephemeral-lint-errors`
-
-`TodoQueryResult.lint_errors` is populated from the current sync pass's `sync.errors`, which only contains errors for files the sync actually re-linted (step 5 working-tree changes, step 4 HEAD diff, step 2 reconcile-dirty). A file with a lint violation that was committed long ago and hasn't been touched since gets linted once when indexed; on subsequent queries its lint error is not re-emitted — the error message is never persisted in the cache.
-
-Null-`effective_date` rows are exempt from this silence because `todo_query` partitions them from `tasks` into `lint_errors` at the response-building layer on every call (see the invariant on `TodoQueryResult` and the test `test_null_effective_date_partitioned_across_queries`). But other lint rules (e.g. terminal item with `check_in_date` set, `end_date` without `start_date`) still exhibit the silence.
-
-`graf lint` runs a full-repo scan and is the authoritative way to surface all current violations. `graf todo`'s `lint_errors` is therefore a "what changed in this sync pass" channel plus the null-date partition, not a comprehensive "what's wrong in the repo" report.
-
-Fix options: (1) persist lint error messages in the cache (column on `docs` or a side table), refresh the set on each file re-lint; (2) re-run `check_rules` at query time against every cache row (likely too expensive for large repos); (3) document the current semantics and treat `graf lint` as the authoritative surface. Option (1) has the best cost/value ratio and would let `graf todo`'s `lint_errors` become trustworthy in steady state.
+## `persist-parse-errors`
+
+Lint-rule violations are persisted per-doc in `docs.lint_errors`, so `graf todo` reports them on every query. Parse and validation failures are not: a file that fails to parse has its cache row deleted (`reconcile_dirty` and `apply_changed_files` in `src/db.rs`), so its error is reported only on the sync pass that touched it — the same ephemerality that persisted lint state fixed for lint rules.
+
+Persisting these means keeping rows for files with no valid frontmatter, which have no `tldr` and no dates: either a nullable-everything doc row or a separate errors table. That is a schema question of its own, and was out of scope for the lint-persistence work. Until then, `graf lint` remains the way to surface standing parse failures.
+
+
+## `resolve-slug-arm-duplication`
+
+`resolve_repos`' slug arm (`src/cli/mod.rs`) and `resolve_target`'s
+`DocRef::Qualified` arm independently perform the same sequence:
+`find_repo_by_slug` → error on miss → `AppManifest::expand_path` →
+`load_repo_config_required` → construct `ResolvedRepo`. The "unknown repo slug"
+error wording has already diverged slightly between the two arms, and any future
+per-repo metadata added to `ResolvedRepo` must be plumbed twice. Now that both
+arms share a `manifest: Option<&AppManifest>` parameter, extract
+`fn resolve_manifest_slug(manifest: &AppManifest, slug: &Slug) -> Result<ResolvedRepo>`
+and call it from both. Deferred: the resolver-threading design deliberately
+scoped to "plain data parameter, no restructuring," so this cleanup is a
+separate change.
+
+## `xdg-empty-unset`
+
+`graf_config_dir_from()` (`src/config.rs`) uses an empty-but-set
+`$XDG_CONFIG_HOME` verbatim, resolving config/manifest relative to the process
+CWD. The XDG Base Directory spec requires an empty variable to be treated as
+unset (fall back to `$HOME/.config`). The behavior is pre-existing (the old
+`env::var` block did the same) and was deliberately preserved as a pure
+extraction — the `config_dir_honors_empty_xdg_verbatim` test pins it so a future
+fix is a deliberate change. Fixing it (`xdg_config_home.filter(|s|
+!s.is_empty())`, invert the test) is a behavior change with CI/systemd/container
+blast radius, so it is deferred to a design decision rather than folded into a
+refactor.
+
+## `clippy-gate-skips-tests`
+
+The `make check` clippy gate (`Makefile`, `clippy:` target) runs `cargo clippy
+-- -D warnings` without `--all-targets`, so test code is ungated. `cargo clippy
+--all-targets` currently emits ~25 warnings in test modules (useless `vec!`,
+redundant closures, etc.). CLAUDE.md's "clippy must pass with no warnings" thus
+holds only for lib/bin code while test-code lint rot accumulates silently. The
+warnings predate this work. Fixing means adding `--all-targets` to the clippy
+target and burning down the existing test-code warnings — a project-wide
+lint-gate policy change, separate from any feature work.
+
+## `frozen-series-effective-date`
+
+`EFF_DATE` in `src/db.rs` resolves an item's effective date as
+`COALESCE(tentative_date, check_in_date, on_date, end_date, due_date)`, on the
+premise that terminal items have their scheduling dates stripped and so sort
+and report by completion date. A frozen series breaks that premise: anchored
+exhaustion (`anchored_terminal_completion` in `src/todo.rs`) preserves
+`check_in_date` frozen at the final occurrence, so an exhausted series reports
+and sorts by that occurrence rather than by its `on_date` completion date —
+off by however long completion lagged the last occurrence.
+
+Which date such an item should show is a product question against the phase-2
+freeze semantics (PRD-data-model §2), not a local bug fix: either the terminal
+branch of the effective-date expression prefers `on_date`/`end_date` (a schema
+bump plus an index review, since `EFF_DATE` must stay textually identical
+across the schema index, SELECT, WHERE and ORDER BY), or the final-occurrence
+ordering is declared intended and documented as such.

src/bin/migrate_recurrence.rs

@@ -225,10 +225,7 @@ fn validate(
         let mut done: Vec<Instance> = 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 {

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,

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(

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());
         }
     }
 

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,30 +26,43 @@ pub struct McpArgs {
 struct McpContext {
     /// Default repo from --repo flag at server startup.
     default_repo: Option<String>,
+    /// App config, loaded once at server startup.
+    config: AppConfig,
 }
 
 impl McpContext {
     /// Resolve the repo for a tool call. Uses the tool-level `repo` param
     /// if provided, otherwise falls back to the server-level default.
     fn resolve_single(&self, tool_repo: Option<&str>) -> Result<ResolvedRepo> {
         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<Vec<ResolvedRepo>> {
         let repo_arg = tool_repo.or(self.default_repo.as_deref());
-        super::resolve_repos(repo_arg)
+        let manifest = AppManifest::load_default()?;
+        super::resolve_repos(repo_arg, manifest.as_ref())
     }
 
     /// Resolve a target document path for mutation tools.
     /// Passes `tool_repo` for conflict detection, server default as fallback.
+    ///
+    /// The manifest is loaded per call (not cached) so tool calls always see
+    /// current manifest edits on this long-running server.
     fn resolve_target(
         &self,
         path_str: &str,
         tool_repo: Option<&str>,
     ) -> Result<(ResolvedRepo, PathBuf)> {
-        super::resolve_target(path_str, tool_repo, self.default_repo.as_deref())
+        let manifest = AppManifest::load_default()?;
+        super::resolve_target(
+            path_str,
+            tool_repo,
+            self.default_repo.as_deref(),
+            manifest.as_ref(),
+        )
     }
 }
 
@@ -75,6 +89,7 @@ graf_ref_attachments, graf_ref_links, graf_ref_context_link.";
 pub fn run(args: McpArgs) -> Result<()> {
     let ctx = McpContext {
         default_repo: args.repo,
+        config: AppConfig::load_default()?,
     };
 
     let server = Server::new("graf", env!("CARGO_PKG_VERSION"))
@@ -449,7 +464,8 @@ impl Tool for TodoQueryTool {
                 "repo": repo_property(),
                 "horizon_days": {
                     "type": "integer",
-                    "description": "Only include items up to N days in the future"
+                    "description": "Only include active items up to N days in the future \
+                                    and done/cancelled items completed within the last N days"
                 },
                 "include_done": {
                     "type": "boolean",
@@ -475,10 +491,8 @@ impl Tool for TodoQueryTool {
 
     fn call(&self, args: Value) -> Result<Value> {
         let repos = self.0.resolve_all(args["repo"].as_str())?;
-        // TODO(mcp-config-caching)
-        let config = graf::config::AppConfig::load_default()?;
         let explicit_limit = args["limit"].as_u64().map(|v| v as usize);
-        let limit = explicit_limit.or(config.todo.effective_limit());
+        let limit = explicit_limit.or(self.0.config.todo.effective_limit());
 
         let result = super::multi_repo_todo_query(
             &repos,
@@ -1205,6 +1219,7 @@ mod tests {
     fn cancel_tool_ctx(dir: &TempDir) -> TodoCancelTool {
         TodoCancelTool(McpContext {
             default_repo: Some(dir.path().to_str().unwrap().to_string()),
+            config: AppConfig::default(),
         })
     }
 
@@ -1311,7 +1326,10 @@ mod tests {
 
     #[test]
     fn cancel_schema_requires_today() {
-        let tool = TodoCancelTool(McpContext { default_repo: None });
+        let tool = TodoCancelTool(McpContext {
+            default_repo: None,
+            config: AppConfig::default(),
+        });
         let schema = tool.input_schema();
         let required = schema["required"].as_array().expect("required is array");
         let required_strs: Vec<&str> = required.iter().filter_map(|v| v.as_str()).collect();