review: 9 findings on diff/stdin/agents surface; bump to 0.6.0

AGENTS.md

@@ -38,7 +38,11 @@ When making changes, verify that all affected surfaces stay consistent:
 - **SPEC.md** — Section 7 (Library API) must match public function signatures. Section 10 (File Discovery) must match discovery behavior. Update the spec version when semantics change.
 - **README.md** — Core Concepts and CLI Commands table should reflect current behavior.
 - **site/** — `site/js/playground.js` contains a JavaScript scoring engine for the web playground. If scoring logic, record format, or field names change, update it to match.
-- **Cargo.toml** — Bump the crate version for any user-visible change (new feature, behavior change, bug fix). Coordinate with `SPEC.md` version when the spec itself changes.
+- **Cargo.toml** — Bump the crate version for any user-visible change, in the same commit as the change. Follow semver:
+  - **Minor** (`0.X.0`) for new features, new commands/flags, behavior changes, or anything that expands the user-visible surface.
+  - **Patch** (`0.X.Y`) only for bug fixes and internal refactors that do not change observable behavior.
+  - Update `Cargo.lock` in the same commit (rebuild or `cargo update -p qualifier`).
+  - Coordinate with `SPEC.md` version when the spec itself changes.
 - **Tests** — Many test files have local `make_att()`/`make_record()` helpers that construct records by hand. When adding or renaming fields on `Annotation`, `Epoch`, or `DependencyRecord`, update all helpers (~6 locations across `src/` and `tests/`). Run `cargo test --all-features` to catch any you miss.
 - **Golden IDs** — `tests/integration.rs` pins BLAKE3 IDs for annotation, epoch, and dependency records. Any change to canonical form (field order, new envelope fields, MCF rules) will break these. Update the expected hashes after confirming the new values are correct.
 

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "qualifier"
-version = "0.5.1"
+version = "0.6.1"
 edition = "2024"
 description = "Deterministic quality annotations for software artifacts"
 license = "MIT OR Apache-2.0"

src/cli/commands/.qual

@@ -5,3 +5,11 @@
 {"metabox":"1","type":"annotation","subject":"src/cli/commands/ls.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:03:49.490119Z","id":"2ee5791b02b2ca3aa1647983ac4c19645065f6a4124edf9fc5a1be35893ab1c4","body":{"detail":"Two related issues in the row-building code: (1) by_subject is populated from ALL records — superseded ones are not filtered out via compact::filter_superseded — so a subject whose only concern was resolved still shows up with that concern's kind in its kinds vector, and the displayed annotation count includes the historical record. (2) When --kind is passed, the filter selects subjects with ANY kind matching, but '({n} annotations)' prints kinds.len() (total), not the number of records of the requested kind. Example: a file with 3 annotations of which 1 is a 'concern' will print '(3 annotations)' under 'qualifier ls --kind concern', misleading the user about how many concerns exist.","kind":"concern","span":{"start":{"line":33},"end":{"line":60},"content_hash":"f9f6b082a63495dbcb0ca45ed1fd7010f6021fd0d075403c72f78e45d5be4e37"},"suggested_fix":"Run filter_superseded over the discovered records before grouping, and when --kind is set, count only records whose kind matches the filter (or display both totals). Optionally also exclude resolve-kind tombstones unless --all is added (mirroring show.rs).","summary":"ls counts include superseded records and conflate kinds when --kind is set","tags":["review","bug","ux"]}}
 {"metabox":"1","type":"annotation","subject":"src/cli/commands/record.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:04:31.366792Z","id":"b5b9f7b2f284c6b9337232eb07844ceb19d16531fa7dbaf308ca7901db003143","body":{"detail":"Each of record.rs, reply.rs, resolve.rs, and emit.rs reproduces the same blocks: (1) 'normalize_issuer_uri(args.issuer.or_else(detect_issuer).unwrap_or_else(|| \"mailto:unknown@localhost\".into()))' — and the .unwrap_or_else fallback is dead code because detect_issuer's chain ends with Some(format!(\"mailto:{user}@localhost\")) and never returns None; (2) 'match args.issuer_type { Some(s) => s.parse::<IssuerType>().map_err(...), None => None }'; (3) parse-existing-file + check_supersession_cycles + validate_supersession_targets when supersedes is set. Drift risk and tedious changes.","kind":"suggestion","span":{"start":{"line":110},"end":{"line":119},"content_hash":"18ed8dfd9ee052067dada024dd819c48b12fe0e8c66dad08fcdaddbbac595d71"},"suggested_fix":"Extract a shared helper, e.g. mod cli::common with build_issuer(args_issuer: Option<&str>, args_issuer_type: Option<&str>) -> Result<(String, Option<IssuerType>)> and verify_supersession(qual_path: &Path, candidate: &Record) -> Result<()>. Each command then calls into the helper. Bonus: fix the dead 'mailto:unknown@localhost' fallback while consolidating.","summary":"issuer URI/issuer-type plumbing and supersession pre-flight are duplicated across record/reply/resolve/emit","tags":["review","refactor","duplication"]}}
 {"metabox":"1","type":"annotation","subject":"src/cli/commands/show.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:04:59.688554Z","id":"d02772f915399edb55722a27d5a55ff6a62d4ea4ce6176cb621465edc3f391e6","body":{"detail":"show.rs treats 'no records found' as Err(Validation), causing a non-zero exit and an error printed to stderr. ls.rs for the same case prints 'No matching artifacts found.' on stdout and exits 0. Scripts piping show into other tools have to either suppress stderr or whitelist that specific message; a programmatic consumer can't distinguish 'no annotations' from 'invalid invocation' by exit code alone. The praise command also returns Err on empty, matching show but still inconsistent with ls.","kind":"suggestion","span":{"start":{"line":50},"end":{"line":50},"content_hash":"af1349b9f5f9a1a6a0404dea36dcc9499bcb25c9adc112b7cc9a93cae41f3262"},"suggested_fix":"Standardize: 'no records' is informational, not an error. Print a friendly message on stdout (or empty JSON [] for --format json) and exit 0 in show/praise too. Reserve non-zero exit for actual errors (invalid kind, malformed args, IO failure). If a script wants to error on empty, it can pipe through 'test -s' or check JSON length.","summary":"show returns exit code 1 when no records exist for a subject, but ls returns 0 — pick one convention","tags":["review","ux","exit-code"]}}
+{"metabox":"1","type":"annotation","subject":"src/cli/commands/diff.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:24:21.788558Z","id":"3d50a1caf30fa746a15f2002c10be5b1ee4c8e8675a2f9acca77c8ebf54d3c53","body":{"detail":"term_width() at line 671-677 reads std::env::var(\"COLUMNS\"). $COLUMNS is a shell variable, not exported into the environment by default in bash/zsh. Most users running 'qualifier diff' from an interactive shell will hit the .unwrap_or(80) fallback even on a 200-column terminal, defeating the 'wrap to terminal width' premise the module-level doc and 3e6cdeb commit message imply. The doc-comment 'set by most shells when stdout is a TTY' is misleading — set, yes; exported, no.","kind":"concern","span":{"start":{"line":671},"end":{"line":677},"content_hash":"f1c6fd525c580c75d01a2b6aa07ba62527fef6b3828c3c1dd25f010aa5bbdbdd"},"suggested_fix":"Use the terminal_size crate (already common, ~150 LOC, no transitive cost) or call libc::ioctl(STDOUT_FILENO, TIOCGWINSZ, ...) directly. Keep $COLUMNS as a manual override above the ioctl result for predictable testing.","summary":"term_width() reads only $COLUMNS, which most shells don't export — wrapping rarely adapts to the real terminal","tags":["review","bug"]}}
+{"metabox":"1","type":"annotation","subject":"src/cli/commands/record.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:24:31.108520Z","id":"8e9e728f37b4f84b00c22112318f8154e4fa5a452b770990250d468e63b71f08","body":{"detail":"Args.file (line 65: 'Explicit .qual file to write to (overrides layout resolution)') is read in run() at line 146 but never threaded into run_batch()/process_one(). Inside process_one (line 327), qual_file::resolve_qual_path(record.subject(), None) hard-codes None, so 'qualifier record --stdin --file foo.qual <feed' writes wherever layout resolution lands — usually a different file. No error, no warning. Confusing for the documented use case 'feed many records into one file'.","kind":"concern","span":{"start":{"line":308},"end":{"line":348},"content_hash":"786285ab278562a65c628f3e2b039ee393fdcbe062fb2cd8b46decce06acc7c3"},"suggested_fix":"Plumb the --file value through run_batch into process_one and pass it as the second arg of resolve_qual_path. Or, if intentional, document under --stdin that --file is ignored and reject the combination at clap parse time.","summary":"process_one ignores --file in stdin batch mode, silently dropping the documented override","tags":["review","bug"]}}
+{"metabox":"1","type":"annotation","subject":"src/cli/commands/record.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:24:37.457424Z","id":"0c0883f876cd4fb8833a8b042cc7b2bb4c4432e85118bd03b35ea7b3702823c8","body":{"detail":"build_record_from_overrides (line 446) is called once per stdin line. When a line omits the 'issuer' key, it calls detect_issuer() at line 483, which spawns 'git config user.email' (line 543) and falls through to 'hg config ui.username' (line 553). For a 1000-line batch with no issuer override, that's ~2000 subprocess forks for an answer that does not change inside one run. Visible as latency on big batches and as noise in process auditing logs.","kind":"concern","span":{"start":{"line":478},"end":{"line":485},"content_hash":"16bd138029bc2e9f1ace04ec1f01301b78eecdaec9988e945699aa4a10733afe"},"suggested_fix":"Cache the detected issuer once at the top of run_batch (or in a OnceCell/LazyLock at module scope) and reuse it for every line. Same fix would help run() too if the user pipes record into a tight loop.","summary":"stdin batch re-runs detect_issuer() per line, forking git/hg subprocesses N times for invariant data","tags":["review","perf"]}}
+{"metabox":"1","type":"annotation","subject":"src/cli/commands/diff.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:24:52.211441Z","id":"6591fa81e85f4ca1e812185bb84d9a5a020d1129adda0d5381b3c76e074afcdb","body":{"detail":"qual_file::discover at line 155 honors .qualignore via the ignore crate (qual_file.rs:191 add_custom_ignore_filename(\".qualignore\")), but load_records_at_ref enumerates blobs straight from the git tree (enumerate_qual_blobs at line 369) with no .qualignore consideration. The doc-comment at line 76-77 claims 'the ref-side enumeration is governed by git itself' to justify --no-ignore not applying there, but git only filters .gitignore'd files at write time — anything tracked at <ref> is in the tree regardless of a current .qualignore. Result: a .qual record present at <ref> in a path that is now .qualignore'd disappears from the new-side scan, so it falls into the Resolved bucket as 'removed (no successor)' even though the file is sitting on disk untouched.","kind":"concern","span":{"start":{"line":155},"end":{"line":161},"content_hash":"6ede983c4bbf197d193554361b40b8327c3a5194673d8371c5befa4e3f238f21"},"suggested_fix":"Apply the same ignore::Walk filter to the relative paths returned from enumerate_qual_blobs before loading them, OR document the asymmetry in the --no-ignore help and the diff.md agents page so users aren't surprised by phantom resolutions.","summary":"ref-side enumeration ignores .qualignore, so ignored paths surface asymmetrically as 'Resolved'","tags":["review","bug"]}}
+{"metabox":"1","type":"annotation","subject":"src/cli/commands/diff.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-07T18:24:54.747652Z","id":"33421b18a81c5b13900ec3bce2d68d57546cf9723edf42aeb4c8f50ef91a656f","body":{"detail":"tree.traverse().breadthfirst visits every entry in the tree at <ref>. For a small repo this is fine, but in a monorepo with hundreds of thousands of blobs the cost is O(repo-size) even when there are only a few .qual files. is_qual_path at line 402 is just a path-suffix check, so all the work fetching tree entries just to throw most away is wasted.","kind":"suggestion","span":{"start":{"line":369},"end":{"line":400},"content_hash":"cb831fda1ace50c3aac82f867711e2c6c22ee4777177eddb4cdfc86954be7f91"},"suggested_fix":"gix supports pathspec-filtered traversal; alternatively iterate the tree once and prune subtrees that cannot contain .qual files (no early signal exists, but a depth-first visitor that stops descending into vendored dirs configurable via .qualignore would help). At minimum, benchmark on a >100k-file tree before this becomes a complaint.","summary":"enumerate_qual_blobs walks the entire tree looking at every blob to find .qual files","tags":["review","perf"]}}
+{"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"]}}

src/cli/commands/agents/.qual

@@ -0,0 +1 @@
+{"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"]}}

src/cli/mod.rs

@@ -14,8 +14,11 @@ const HELP_TEMPLATE: &str = "\
 {about-with-newline}
 {usage-heading} {usage}
 
+If you are an AI coding agent, run `qualifier agents` first — it covers
+the conventions and pitfalls you need before recording any annotation.
+
 For AI agents:
-  agents     Self-contained guide for AI coding agents (start here)
+  agents     Read this before recording annotations. Self-contained agent guide.
 
 Record observations:
   record     Record an annotation: `qualifier record <kind> <location> [message]`
@@ -57,7 +60,7 @@ pub struct Cli {
 
 #[derive(Subcommand)]
 pub enum Commands {
-    /// Self-contained guide for AI coding agents (start here)
+    /// Read this before recording annotations. Self-contained agent guide.
     Agents(commands::agents::Args),
 
     /// Record an annotation: `qualifier record <kind> <location> [message]`

tests/cli_integration.rs

@@ -3542,9 +3542,14 @@ fn test_top_level_help_shows_agents_group() {
         stdout.contains("For AI agents:"),
         "help should show the agents group header: {stdout}"
     );
-    // The agents row should appear under that header, with the "start here" nudge.
+    // An imperative directive sits above the subcommand list so an agent
+    // scanning the verb table doesn't mistake it for flavor text.
     assert!(
-        stdout.contains("agents") && stdout.contains("start here"),
-        "help should mention the agents subcommand: {stdout}"
+        stdout.contains("If you are an AI coding agent, run `qualifier agents` first"),
+        "help should print the agent directive above the subcommand list: {stdout}"
+    );
+    assert!(
+        stdout.contains("agents") && stdout.contains("Read this before recording annotations"),
+        "help should mention the agents subcommand with imperative description: {stdout}"
     );
 }