diff --git a/.qual b/.qual index 5d40fbf..a5535a2 100644 --- a/.qual +++ b/.qual @@ -1 +1,2 @@ {"metabox":"1","type":"annotation","subject":"Cargo.toml","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:04:09.856919Z","id":"ccfe88fa371549c80feff4d15e6cd0db767a7c428b4b12f7d9750c0f7bd5060c","body":{"detail":"Cargo.toml lists petgraph = \"0.7\" but no source file imports it: 'grep -rn petgraph src tests' returns nothing. The dependency was added for src/graph.rs (DependencyGraph, toposort, cycle detection), which was yanked in the same wave as scoring. Carrying it inflates build times and the binary size for no gain.","kind":"suggestion","span":{"start":{"line":24},"end":{"line":24},"content_hash":"bab5a998dd0d510c555f7bc68c7296f6068164a3305155a633cc11fd62f102da"},"suggested_fix":"Remove the petgraph entry from [dependencies]. If you anticipate bringing the graph engine back per SPEC §12 Future Considerations, leave a brief commit-message note rather than a dead dep — re-adding the line is cheap.","summary":"petgraph dependency is unused after the graph engine yank","tags":["review","cleanup"]}} +{"metabox":"1","type":"annotation","subject":"Cargo.toml","issuer":"mailto:alex@empathic.dev","issuer_type":"ai","created_at":"2026-05-08T03:15:18.383650Z","id":"23e83817b9194995f38452eb705bb1c3f5253b0f991f01c1c203d21929b92ae2","body":{"kind":"resolve","summary":"petgraph removed from Cargo.toml","supersedes":"ccfe88fa371549c80feff4d15e6cd0db767a7c428b4b12f7d9750c0f7bd5060c"}} diff --git a/Cargo.lock b/Cargo.lock index f4cfce5..9057fc1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -492,12 +492,6 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" -[[package]] -name = "fixedbitset" -version = "0.5.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d674e81391d1e1ab681a28d99df07927c6d4aa5b027d7da16ba32d1d21ecd99" - [[package]] name = "fnv" version = "1.0.7" @@ -1844,16 +1838,6 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" -[[package]] -name = "petgraph" -version = "0.7.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3672b37090dbd86368a4145bc067582552b29c27377cad4e0a306c97f9bd7772" -dependencies = [ - "fixedbitset", - "indexmap", -] - [[package]] name = "plain" version = "0.2.3" @@ -1938,7 +1922,6 @@ dependencies = [ "figment", "gix", "ignore", - "petgraph", "rand", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 4034541..625579f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ cli = ["dep:clap", "dep:comfy-table", "dep:figment", "dep:gix", "dep:rand"] blake3 = "1" chrono = { version = "0.4", features = ["serde"] } ignore = "0.4" -petgraph = "0.7" serde = { version = "1", features = ["derive"] } serde_json = "1" thiserror = "2" diff --git a/src/cli/.qual b/src/cli/.qual index 58198f6..a83dbfa 100644 --- a/src/cli/.qual +++ b/src/cli/.qual @@ -1,3 +1,5 @@ {"metabox":"1","type":"annotation","subject":"src/cli/config.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:03:58.787261Z","id":"8773c911ade69c06156975ba4838c7899a7e1dee800a1770a97f7e651eefc0e9","body":{"detail":"load() ends with 'figment.extract().unwrap_or_default()'. If a user has a typo in ~/.config/qualifier/config.toml or .qualifier.toml, or sets QUALIFIER_FORMAT=unknown, the extraction errors and load() silently returns Config::default(). The user has no signal that their configuration was ignored; surprising behavior. Also: load() is currently unused at the call sites I checked (no caller threads Config into commands), so this may be latent dead code, but the silence is still wrong if/when wired up.","kind":"concern","span":{"start":{"line":60},"end":{"line":60},"content_hash":"2b2f08a521620d4dd79a46b7d69d311323a0d2947dcc293738e6706dcf057f91"},"suggested_fix":"Surface extraction errors. At minimum, eprintln! a warning ('qualifier: ignoring invalid config from : ') and return defaults. Better: return Result and let run() decide whether to continue. Also worth verifying load() is actually invoked — if not, either wire it in or delete it.","summary":"figment configuration errors are silently swallowed via unwrap_or_default","tags":["review","config"]}} {"metabox":"1","type":"annotation","subject":"src/cli/config.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:04:04.520797Z","id":"fe7660012ad315765368de10edde22ab1f4eb42e8aac605719580760f83eb2d4","body":{"detail":"load() reads std::env::var(\"HOME\") to locate ~/.config/qualifier/config.toml. On Windows, HOME is typically unset (the analogue is USERPROFILE, and the user-config XDG-style location differs). The 'if let Ok(home)' branch is silently skipped, so Windows users cannot configure qualifier via the user-level file.","kind":"concern","span":{"start":{"line":43},"end":{"line":49},"content_hash":"96a6f11015c6b3a729e8f05558c7bccd797b0f0062edddf1f2e0e8a7c81f6018"},"suggested_fix":"Use the 'directories' (or 'dirs') crate's config_dir() / home_dir() to resolve the user-config path portably, falling through to USERPROFILE/HOMEDRIVE+HOMEPATH or platform APIs as that crate does. Or document that the user-level config is POSIX-only and surface an explicit warning when no home dir is found.","summary":"user-level config path uses HOME and is silently skipped on Windows","tags":["review","portability"]}} {"metabox":"1","type":"annotation","subject":"src/cli/mod.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:04:43.911212Z","id":"c0fd1201cf8e6e1cb4e833e4a81f3c8a64b64571ffb25d9b371880af811f0345","body":{"detail":"The constant HELP_TEMPLATE renders the subcommand list (and their grouping under 'Record observations / Inspect annotations / Maintain / Other') as plain text. The Commands enum is the source of truth for parsing, but if a subcommand is added, renamed, or removed, nothing fails — the inline comment ('If you add, rename, or remove a subcommand, update HELP_TEMPLATE to match') is the only safeguard. The risk is real: 'review' is the public command name (line 30) but the source file is freshness.rs and the variant is Commands::Review wired to commands::freshness::run — a rename like that has already happened once. A future rename will silently desync the help.","kind":"suggestion","span":{"start":{"line":13},"end":{"line":43},"content_hash":"f094e3abd8e8f1deb10ea4bdf4ca05ec5cbbd1c019ea28060f3f0563281e2c76"},"suggested_fix":"Add a unit test that parses HELP_TEMPLATE for the bullet names ('record', 'reply', 'resolve', 'emit', 'show', 'ls', 'praise', 'review', 'compact', 'haiku', 'agents') and asserts each appears as a Commands variant via clap's introspection (Cli::command().get_subcommands().map(|c| c.get_name())). The test would also catch stray entries removed from the enum but lingering in the template.","summary":"HELP_TEMPLATE manually mirrors the Commands enum with no compile-time check that they agree","tags":["review","drift-risk"]}} +{"metabox":"1","type":"annotation","subject":"src/cli/config.rs","issuer":"mailto:alex@empathic.dev","issuer_type":"ai","created_at":"2026-05-08T03:52:20.232490Z","id":"39a16d39e1465e6dbc6b66fb37746aa26e4d9a2dc274bbafb6776f508106d305","body":{"kind":"resolve","summary":"load() now returns Result; cli::run validates config eagerly and exits non-zero on malformed TOML or env vars","supersedes":"8773c911ade69c06156975ba4838c7899a7e1dee800a1770a97f7e651eefc0e9"}} +{"metabox":"1","type":"annotation","subject":"src/cli/config.rs","issuer":"mailto:alex@empathic.dev","issuer_type":"ai","created_at":"2026-05-08T03:52:20.250956Z","id":"874345a974d314a69d7d202a9c85db88220ef5ec153ee85a5ac574232271912e","body":{"kind":"resolve","summary":"user-config path now falls back to USERPROFILE when HOME is unset, covering Windows","supersedes":"fe7660012ad315765368de10edde22ab1f4eb42e8aac605719580760f83eb2d4"}} diff --git a/src/cli/commands/.qual b/src/cli/commands/.qual index bad5f8e..d80b0b2 100644 --- a/src/cli/commands/.qual +++ b/src/cli/commands/.qual @@ -13,3 +13,4 @@ {"metabox":"1","type":"annotation","subject":"src/cli/commands/diff.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:25:02.835698Z","id":"4cf7a6d3d7986f0b33c10000eb8e45927a60cb7ef97b211a2ff7d59878ffa1ad","body":{"detail":"Line 430-433 builds 'HashMap<&str, &Record>' from filter_map+collect over 'new'. If two HEAD records both supersede the same old id (legal — supersession is acyclic but not unique), the HashMap silently keeps whichever comes last in iteration order, and the Resolved-section closer line shows just that one. The user may not notice the other supersession at all.","kind":"suggestion","span":{"start":{"line":430},"end":{"line":442},"content_hash":"918a0d6117d7bc75d3c50bb5d69cef3dc700607b76ebd6948bd84ea5ffde013d"},"suggested_fix":"Use HashMap<&str, Vec<&Record>> and either join all closers in print_resolved or surface the multiplicity as a warning. At minimum, add a debug_assert!(prior.is_none()) or a stderr 'note:' line so the loss isn't invisible.","summary":"supersedes_index collapses duplicates: only one closer is shown when multiple HEAD records supersede the same id","tags":["review","correctness"]}} {"metabox":"1","type":"annotation","subject":"src/cli/commands/record.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:25:20.512585Z","id":"1f436b7904d5c534cc45b342e96d453cbbe25418ee6d6719339e6abcefad1622","body":{"detail":"When --stdin --continue-on-error --format json sees any failed line, line 296-298 invokes std::process::exit(1) so the top-level main() doesn't add a 'qualifier: ...' line on top of the JSONL summary already on stderr. Effective, but it skips Drop, skips any future shared post-processing in the binary, and embeds the formatting concern (which layer prints the trailing prefix) inside the command implementation. Easy to forget if a future refactor adds a finalizer in lib.rs.","kind":"suggestion","span":{"start":{"line":296},"end":{"line":298},"content_hash":"755a2d5457d7d1510e682c48a00bbbaef3d818716df63d2558edb78990207fc1"},"suggested_fix":"Add a sibling Error variant — e.g. Error::AlreadyReported — that the top-level handler recognizes and exits non-zero without prefixing. Or thread a 'json mode' flag through to the top-level handler so the prefix is suppressed there. Either is more discoverable than process::exit at the leaf.","summary":"stdin batch under --format json calls process::exit(1) to suppress the top-level error prefix — bypasses normal error flow","tags":["review","consistency"]}} {"metabox":"1","type":"annotation","subject":"src/cli/commands/diff.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:25:26.434773Z","id":"ea460cac29b6ea81575b5ca5287bdfd17093c5808a97abaef6a6917f6e2b6c66","body":{"detail":"Defaulting the comparison commit to merge-base(HEAD, ref) is exactly the right call for a PR-style 'what changed?' workflow — listing records that landed on main after the branch forked under 'Resolved' would be misleading. The fallback path (line 145-151) when no merge-base exists prints to stderr and degrades to ref-tip rather than failing, which is friendlier than aborting on a fresh clone with shallow history. The --from-tip flag opts back into the literal compare with documented justification. Worth keeping as-is.","kind":"praise","span":{"start":{"line":132},"end":{"line":153},"content_hash":"7eb354d4c800b570c20fbe5f46d471dff527bdfaafda89931df92a10307018b2"},"summary":"merge-base default with --from-tip escape hatch correctly isolates 'what this branch introduced' from 'what landed on main since fork'","tags":["review","design"]}} +{"metabox":"1","type":"annotation","subject":"src/cli/commands/freshness.rs","issuer":"mailto:alex@empathic.dev","issuer_type":"ai","created_at":"2026-05-08T03:46:18.486799Z","id":"454690068da144f2ca15944df9505654b206ff8fbe1e791d130b03d5f2cc5afb","body":{"kind":"resolve","summary":"review now resolves project root from canonical CWD before discover, and joins the project root with each subject when checking spans","supersedes":"65a84bf71d2265d5e185340d4643c8ec8c3e93f51308cdb5d4c2a3beb706512a"}} diff --git a/src/cli/commands/agents/.qual b/src/cli/commands/agents/.qual index 79c2c63..7376ad7 100644 --- a/src/cli/commands/agents/.qual +++ b/src/cli/commands/agents/.qual @@ -1 +1,2 @@ {"metabox":"1","type":"annotation","subject":"src/cli/commands/agents/mod.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T20:28:53.045932Z","id":"436a095a263be032611015f79b3a8fae2afe062d0b251a849647ab20491277cb","body":{"detail":"Every other command propagates errors via crate::Result so the top-level CLI handler renders them uniformly. agents::run for an unknown topic eprintln!s and calls std::process::exit(2). Exit code 2 is also out of band — the rest of qualifier surfaces validation/io/json errors through Error::Validation, which the binary maps to 1. A scripted user who switch-cases on $? will see two distinct error codes for what is morally the same class of error (bad input). Also no 'did-you-mean' suggestion despite a small known-topic list.","kind":"concern","span":{"start":{"line":40},"end":{"line":45},"content_hash":"a917c7bd000d28be64496d6940f7e62622b576188320437b43264e3389db5c1d"},"suggested_fix":"Return Err(Error::Validation(format!(\"no such topic '{name}'. Available: {}\", topic_names()))) and let the top-level handler exit. If exit-2-vs-1 differentiation is intentional (e.g. signaling 'usage error' vs 'runtime error'), introduce that distinction as a first-class Error variant rather than a one-off process::exit.","summary":"agents subcommand uses process::exit(2) for unknown topics instead of returning Err — inconsistent with the rest of the CLI","tags":["review","consistency"]}} +{"metabox":"1","type":"annotation","subject":"src/cli/commands/agents/mod.rs","issuer":"mailto:alex@empathic.dev","issuer_type":"ai","created_at":"2026-05-08T03:28:48.758123Z","id":"c104ac338bd9942bee1f53e226b0845a0ee62f5ed64d421840bd0508e3079b03","body":{"kind":"resolve","summary":"agents subcommand now returns Err(Validation) for unknown topics, exit code 1 to match the rest of the CLI","supersedes":"436a095a263be032611015f79b3a8fae2afe062d0b251a849647ab20491277cb"}} diff --git a/src/cli/commands/agents/mod.rs b/src/cli/commands/agents/mod.rs index 8fd2c2b..b2cd07f 100644 --- a/src/cli/commands/agents/mod.rs +++ b/src/cli/commands/agents/mod.rs @@ -37,11 +37,10 @@ pub fn run(args: Args) -> crate::Result<()> { print!("{}", page.body); Ok(()) } else { - eprintln!( - "qualifier agents: no such topic '{name}'. Available: {}", + Err(crate::Error::Validation(format!( + "no such topic '{name}'. Available: {}", topic_names() - ); - std::process::exit(2); + ))) } } } diff --git a/src/cli/commands/freshness.rs b/src/cli/commands/freshness.rs index c95fc24..b63d552 100644 --- a/src/cli/commands/freshness.rs +++ b/src/cli/commands/freshness.rs @@ -1,10 +1,8 @@ -use std::path::Path; - use clap::Args as ClapArgs; use crate::compact::filter_superseded; use crate::content_hash::{self, FreshnessStatus}; -use crate::qual_file; +use crate::qual_file::{self, find_project_root}; #[derive(ClapArgs)] pub struct Args { @@ -29,8 +27,10 @@ struct CheckResult { } pub fn run(args: Args) -> crate::Result<()> { - let root = Path::new("."); - let qual_files = qual_file::discover(root, !args.no_ignore)?; + let cwd = std::env::current_dir()?; + let project_root = find_project_root(&cwd); + let discover_root = project_root.as_deref().unwrap_or(cwd.as_path()); + let qual_files = qual_file::discover(discover_root, !args.no_ignore)?; if qual_files.is_empty() { if args.format == "json" { @@ -87,7 +87,8 @@ pub fn run(args: Args) -> crate::Result<()> { format!("{}:{}{}", att.subject, span.start.line, end) }; - let status = content_hash::check_freshness(Path::new(&att.subject), span); + let subject_path = discover_root.join(&att.subject); + let status = content_hash::check_freshness(&subject_path, span); results.push(CheckResult { subject: att.subject.clone(), diff --git a/src/cli/config.rs b/src/cli/config.rs index 4d0814b..81e2d83 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -35,27 +35,38 @@ impl Default for Config { } } +/// Resolve the user home directory across platforms. +/// +/// Prefers `$HOME` (POSIX) and falls back to `$USERPROFILE` (Windows). Returns +/// `None` when neither is set so the user-level config merge is skipped rather +/// than silently failing. +fn user_home_dir() -> Option { + std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .map(PathBuf::from) +} + /// Load configuration by merging all sources. -pub fn load(project_root: Option<&Path>) -> Config { +/// +/// Returns an error if any present config file is malformed or any +/// `QUALIFIER_*` env var fails to deserialize. Missing config files are not +/// an error. +pub fn load(project_root: Option<&Path>) -> crate::Result { let mut figment = Figment::new().merge(Serialized::defaults(Config::default())); - // User-level config: ~/.config/qualifier/config.toml - if let Ok(home) = std::env::var("HOME") { - let user_config = PathBuf::from(home) - .join(".config") - .join("qualifier") - .join("config.toml"); + if let Some(home) = user_home_dir() { + let user_config = home.join(".config").join("qualifier").join("config.toml"); figment = figment.merge(Toml::file(user_config)); } - // Project-level config: /.qualifier.toml if let Some(root) = project_root { let project_config = root.join(".qualifier.toml"); figment = figment.merge(Toml::file(project_config)); } - // Environment variables: QUALIFIER_ISSUER, QUALIFIER_FORMAT, etc. figment = figment.merge(Env::prefixed("QUALIFIER_")); - figment.extract().unwrap_or_default() + figment + .extract() + .map_err(|e| crate::Error::Validation(format!("invalid configuration: {e}"))) } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 0552769..c89581b 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -103,6 +103,17 @@ pub fn run() { ); } + // Validate config eagerly so a malformed .qualifier.toml or + // ~/.config/qualifier/config.toml fails before the command runs. The + // result is discarded for now — no command consumes Config yet — but + // surfacing the parse error here is the contract callers expect. + let cwd = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")); + let project_root = crate::qual_file::find_project_root(&cwd); + if let Err(e) = config::load(project_root.as_deref()) { + eprintln!("qualifier: {e}"); + std::process::exit(1); + } + let result: crate::Result<()> = match cli.command { Commands::Agents(args) => commands::agents::run(args), Commands::Record(args) => commands::record::run(*args), diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index d4a7dfc..cf4d62b 100644 --- a/tests/cli_integration.rs +++ b/tests/cli_integration.rs @@ -1860,6 +1860,47 @@ fn test_review_subject_filter() { ); } +#[test] +fn test_review_finds_annotations_from_subdirectory() { + // Regression: `qualifier review` walked from `Path::new(".")` instead of + // the project root, so it returned "no .qual files found" whenever the + // user invoked it from anywhere but the project root. + let dir = tempfile::tempdir().unwrap(); + git_init(dir.path()); + + let src_dir = dir.path().join("src"); + std::fs::create_dir(&src_dir).unwrap(); + std::fs::write(src_dir.join("lib.rs"), "fn alpha() {}\nfn beta() {}\n").unwrap(); + + // Record from the project root. + let (_, _, rc) = run_qualifier( + dir.path(), + &[ + "record", + "concern", + "src/lib.rs:2", + "look at beta", + "--issuer", + "mailto:test@test.com", + ], + ); + assert_eq!(rc, 0, "record should succeed"); + + // Run review from a nested subdirectory — it must still find the + // annotation (and the span must hash FRESH because the file is unchanged). + let nested = dir.path().join("src"); + let (stdout, stderr, code) = run_qualifier(&nested, &["review"]); + assert_eq!(code, 0, "review from subdir should succeed: {stderr}"); + assert!( + stdout.contains("FRESH"), + "annotation should be discovered and FRESH from subdir: {stdout}" + ); + assert!( + stdout.contains("1 fresh"), + "summary should report 1 fresh from subdir: {stdout}" + ); +} + // --- qualifier emit (raw record write) --- #[test] @@ -2326,10 +2367,17 @@ fn test_agents_bare_invocation_succeeds() { } #[test] -fn test_agents_unknown_topic_exits_2() { +fn test_agents_unknown_topic_returns_error() { let dir = tempfile::tempdir().unwrap(); let (_stdout, stderr, code) = run_qualifier(dir.path(), &["agents", "bogus-topic"]); - assert_eq!(code, 2, "unknown topic should exit 2: stderr={stderr}"); + assert_eq!( + code, 1, + "unknown topic should exit 1 like other validation errors: stderr={stderr}" + ); + assert!( + stderr.starts_with("qualifier:"), + "stderr should use the standard top-level error prefix: {stderr}" + ); assert!( stderr.contains("no such topic"), "stderr should explain: {stderr}" @@ -2530,6 +2578,35 @@ fn test_record_stdin_error_includes_line_number() { ); } +// --- qualifier config loading --- + +#[test] +fn test_malformed_project_config_fails_loudly() { + // Regression: figment errors used to be swallowed via unwrap_or_default(), + // so a typo in .qualifier.toml became silent broken behavior. Now the + // config layer should surface invalid TOML as a top-level error. + let dir = tempfile::tempdir().unwrap(); + git_init(dir.path()); + std::fs::write( + dir.path().join(".qualifier.toml"), + "this is = not [valid TOML\n", + ) + .unwrap(); + + let (_, stderr, code) = run_qualifier(dir.path(), &["ls"]); + assert_ne!(code, 0, "malformed config should fail: stderr={stderr}"); + assert!( + stderr.starts_with("qualifier:"), + "should use the standard top-level error prefix: {stderr}" + ); + assert!( + stderr.to_lowercase().contains("config") + || stderr.to_lowercase().contains("toml") + || stderr.contains(".qualifier.toml"), + "error should explain it's a config issue: {stderr}" + ); +} + // --- qualifier diff --- /// Initialize a git repo in `dir` with name+email config.