From c50b7c6d4200eaa360bdebd10bf47be77d8f2797 Mon Sep 17 00:00:00 2001 From: Alex Kesling Date: Thu, 7 May 2026 17:01:26 -0400 Subject: [PATCH 1/3] review: 9 findings on diff/stdin/agents surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recorded via `qualifier record` during a focused pass over the new `qualifier diff`, `record/emit --stdin`, and `agents` surfaces added in 0.5.0–0.5.1. Concerns: - `term_width()` reads only `$COLUMNS` (not exported by most shells), so the wrap-to-terminal feature almost always falls back to 80 cols (src/cli/commands/diff.rs) - `--file` override silently dropped under `record --stdin`; the documented "write everything to one file" use case writes wherever layout resolution lands (src/cli/commands/record.rs) - `detect_issuer()` re-spawns git/hg per stdin line, ~2N forks for invariant data on a batch of N (src/cli/commands/record.rs) - ref-side enumeration in `qualifier diff` ignores `.qualignore`, so paths excluded only on HEAD surface asymmetrically as "Resolved/removed" even though the file is untouched (src/cli/commands/diff.rs) - `agents` subcommand uses `process::exit(2)` for unknown topics instead of returning `Err`; inconsistent exit code and bypasses the top-level error path (src/cli/commands/agents/mod.rs) Suggestions: - `enumerate_qual_blobs` walks the entire tree at to find a few `.qual` files — O(repo-size) on monorepos - `supersedes_index` HashMap silently keeps only one closer when multiple HEAD records supersede the same id - stdin batch under `--format json` calls `process::exit(1)` to suppress the top-level error prefix; should be a first-class `Error::AlreadyReported` variant Plus one praise: the merge-base default with `--from-tip` escape hatch is exactly the right model for PR-style diffing. --- src/cli/commands/.qual | 8 ++++++++ src/cli/commands/agents/.qual | 1 + 2 files changed, 9 insertions(+) create mode 100644 src/cli/commands/agents/.qual diff --git a/src/cli/commands/.qual b/src/cli/commands/.qual index d455aab..bad5f8e 100644 --- a/src/cli/commands/.qual +++ b/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::().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)> 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 is in the tree regardless of a current .qualignore. Result: a .qual record present at 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 . 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"]}} diff --git a/src/cli/commands/agents/.qual b/src/cli/commands/agents/.qual new file mode 100644 index 0000000..79c2c63 --- /dev/null +++ b/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"]}} From c147592b0c98d7b31ccae2961363d3107a8184f8 Mon Sep 17 00:00:00 2001 From: Alex Kesling Date: Thu, 7 May 2026 17:01:35 -0400 Subject: [PATCH 2/3] chore: bump to 0.6.0; tighten AGENTS.md version-bump rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 0.5.0 → 0.5.1 jump that landed the new `qualifier diff` top-level command and `record/emit --stdin` batch mode (~1000 LOC of new user-visible surface) was a patch bump, but by semver that's a minor. Correct it forward to 0.6.0 rather than retroactively rewriting 0.5.1. Also expand the AGENTS.md "Keeping Things in Sync" entry for Cargo.toml so future agents/contributors get the gradient explicitly: - minor for new features / new commands / behavior changes - patch only for bug fixes or behavior-preserving refactors - update Cargo.lock in the same commit --- AGENTS.md | 6 +++++- Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0af6a56..f59e5c1 100644 --- a/AGENTS.md +++ b/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. diff --git a/Cargo.lock b/Cargo.lock index f5b1ddb..d60fbe9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1929,7 +1929,7 @@ dependencies = [ [[package]] name = "qualifier" -version = "0.5.1" +version = "0.6.0" dependencies = [ "blake3", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 02671c6..e6c6089 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "qualifier" -version = "0.5.1" +version = "0.6.0" edition = "2024" description = "Deterministic quality annotations for software artifacts" license = "MIT OR Apache-2.0" From dd093ff2423f8a551fa85a957465e4cf2d30dd11 Mon Sep 17 00:00:00 2001 From: Alex Kesling Date: Thu, 7 May 2026 22:28:44 -0400 Subject: [PATCH 3/3] feat(help): make `qualifier agents` directive imperative, not parenthetical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous help template put `agents` in the "For AI agents:" group with a description ending "(start here)". An agent scanning the verb table — looking for `record`/`show`/`ls` — easily reads `(start here)` as flavor text and proceeds without ever opening the agents page, missing pitfalls like "don't volunteer praise annotations". This is exactly what happened during the review pass for PR #8. Two changes: - Add an imperative one-liner ABOVE the subcommand list so the directive isn't competing with 12 other entries: If you are an AI coding agent, run `qualifier agents` first — it covers the conventions and pitfalls you need before recording any annotation. - Rephrase the agents row description from a passive label ("Self-contained guide for AI coding agents (start here)") to an imperative ("Read this before recording annotations. Self-contained agent guide."). Updates the help-template integration test to assert both the directive and the new description. Patch bump 0.6.0 → 0.6.1: text-only UX change, no surface added. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/cli/mod.rs | 7 +++++-- tests/cli_integration.rs | 11 ++++++++--- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d60fbe9..f4cfce5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1929,7 +1929,7 @@ dependencies = [ [[package]] name = "qualifier" -version = "0.6.0" +version = "0.6.1" dependencies = [ "blake3", "chrono", diff --git a/Cargo.toml b/Cargo.toml index e6c6089..4034541 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "qualifier" -version = "0.6.0" +version = "0.6.1" edition = "2024" description = "Deterministic quality annotations for software artifacts" license = "MIT OR Apache-2.0" diff --git a/src/cli/mod.rs b/src/cli/mod.rs index aea4d1f..0552769 100644 --- a/src/cli/mod.rs +++ b/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 [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 [message]` diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index 1c2326e..d4a7dfc 100644 --- a/tests/cli_integration.rs +++ b/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}" ); }