{"metabox":"1","type":"annotation","subject":"src/cli/commands/emit.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:03:14.919264Z","id":"d7b8f76adcbb216d79c38355947ea09f8936f77b8c71601d5b88382b98454962","body":{"detail":"When 'qualifier emit <custom-type> <subject> --body ...' is invoked with a record_type other than annotation/epoch/dependency, build_record() builds an envelope with id:\"\" (line 150) and round-trips through Record. The deserializer routes custom types to Record::Unknown(value), and finalize_record's Unknown arm is a passthrough (annotation.rs:831 'other => other'), so the empty id is never replaced. Reproduced: 'qualifier emit https://example.com/custom/v1 test.rs --body {\"foo\":\"bar\"}' wrote a line with \"id\":\"\". Such records are not addressable by id-prefix and break content-addressing.","kind":"concern","span":{"start":{"line":130},"end":{"line":160},"content_hash":"e578dcb79e1d71479ff8f865f5cb9715e5d8a838bb2e5a5957a70a9997a08087"},"suggested_fix":"Compute a BLAKE3 id for Unknown records too — either by adding an Unknown arm to finalize_record (hashing the canonical-ordered envelope serialization with id=\"\"), or by computing the id directly in build_record before the from_value round-trip.","summary":"emit with custom record types produces records with empty 'id'","tags":["review","bug"]}}
{"metabox":"1","type":"annotation","subject":"src/cli/commands/emit.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:03:25.068400Z","id":"12542525d52f83be6475df6bc3d6fb7e2aca3740a603987f1ba741fe2e4003c5","body":{"detail":"build_record() for non-annotation types constructs a serde_json::Map (BTreeMap by default; serde_json is built without the preserve_order feature) and inserts envelope fields in canonical order, but BTreeMap reorders them alphabetically on serialization. Reproduced output: '{\"body\":...,\"created_at\":...,\"id\":\"\",\"issuer\":...,\"metabox\":\"1\",\"subject\":...,\"type\":...}' — alphabetical, not the spec's metabox/type/subject/issuer/issuer_type/created_at/id/body order. By contrast, the typed Annotation/Epoch/Dependency arms serialize correctly because struct field order is preserved by serde derive. This violates SPEC §canonical-form invariants and makes Unknown-record IDs (once the empty-id bug is fixed) inconsistent with typed-record IDs.","kind":"concern","span":{"start":{"line":135},"end":{"line":153},"content_hash":"b5197b71132a5750988461104da0c346c8190e102715e7c413df4e0cb0e217a4"},"suggested_fix":"Either enable serde_json's 'preserve_order' feature (Cargo.toml) so Map insertion order is preserved, or build the JSON via a typed wrapper struct with the correct field order, mirroring AnnotationCanonicalView.","summary":"emit's Unknown records serialize in alphabetical key order, not canonical Metabox envelope order","tags":["review","bug","canonical-form"]}}
{"metabox":"1","type":"annotation","subject":"src/cli/commands/freshness.rs","issuer":"mailto:claude-review@anthropic.com","issuer_type":"ai","created_at":"2026-05-06T21:03:32.428514Z","id":"65a84bf71d2265d5e185340d4643c8ec8c3e93f51308cdb5d4c2a3beb706512a","body":{"detail":"freshness::run() sets 'let root = Path::new(\".\")' and discovers from there. Other commands (show, ls, praise, reply, resolve, compact --all) call qual_file::find_project_root(Path::new(\".\")) first and only fall back to '.' if no VCS marker is found. Reproduced: from a subdir of a project that has annotations under .qual at the root, 'qualifier review' prints 'No .qual files found.' while every other command sees the records. The same flaw also means span freshness checks resolve subject paths relative to CWD rather than the project root.","kind":"concern","span":{"start":{"line":32},"end":{"line":32},"content_hash":"cc8a38222603f710e47857d33fe01e5f7a8719fa785bc2c50253041e1e72a8dd"},"suggested_fix":"Mirror show.rs / ls.rs: 'let root = qual_file::find_project_root(Path::new(\".\")); let discover_root = root.as_deref().unwrap_or(Path::new(\".\"));' and pass discover_root to discover(). Then use discover_root (or the resolved project root) when interpreting subject as a path for check_freshness.","summary":"qualifier review walks from CWD instead of the project root, so it finds nothing when run from a subdirectory","tags":["review","bug"]}}
{"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:41.980160Z","id":"da1fabb901f1ff8a287213848ab2415caefb01ac1b65c936b821fa07f665a6ef","body":{"detail":"Args::unqualified has the help text 'Show only unannotated artifacts (no annotations)' but the implementation comments 'we can't fully list \"what doesn't exist\"; approximate by listing nothing. The flag stays as a placeholder.' and produces an empty Vec. Users who pass --unqualified see 'No matching artifacts found.' (or '(unqualified listing requires a project file index — not implemented)' depending on path) with no indication that the flag is non-functional. This is a footgun.","kind":"concern","span":{"start":{"line":46},"end":{"line":60},"content_hash":"e04a25a83b943bab5118494b489b3f410a4bef03e7b7ae0a8cff6cb9e6f8c082"},"suggested_fix":"Either remove the flag and return an error suggesting an alternative, or implement it: walk the project tree (already needed for discover) and list files that have no matching subject in by_subject. The walk could reuse the ignore::Walk with a filter for source-like extensions, or accept an explicit --include glob.","summary":"ls --unqualified is a documented stub that always returns empty","tags":["review","bug","ux"]}}
{"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"]}}