review: 9 findings on diff/stdin/agents surface; bump to 0.6.0#8
Merged
Conversation
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 <ref> 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.
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
|
🔍 Preview deployed: https://c43b25f1.qualifier-dev.pages.dev |
…etical 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
qualifier diff,record/emit --stdin, andagentssurfaces added in 0.5.0–0.5.1 (5 concerns, 3 suggestions, 1 praise). Runqualifier diff mainafter checkout to see them under "Added on this branch".0.5.1 → 0.6.0. The diff/stdin/agents work was a minor's worth of new surface but landed as a patch; correcting forward.AGENTS.md"Keeping Things in Sync" so the next time this happens the rule is unambiguous: minor for new features, patch only for fixes,Cargo.lockin the same commit.Highlights from the review
Concerns
diff.rs:671—term_width()reads only$COLUMNS, which most shells don't export. Wrap-to-terminal effectively always falls back to 80 cols.record.rs:308—--fileoverride silently dropped under--stdin.record.rs:478—detect_issuer()re-spawns git/hg per stdin line; ~2N forks for invariant data.diff.rs:155— ref-side enumeration ignores.qualignore, producing phantom "Resolved/removed" entries for paths excluded only on HEAD.agents/mod.rs:40—process::exit(2)for unknown topics; inconsistent exit code, bypasses the top-level error path.Suggestions
diff.rs:369—enumerate_qual_blobswalks the whole tree to find a few.qualfiles.diff.rs:430—supersedes_indexcollapses duplicates: only one closer shown when multiple HEAD records supersede the same id.record.rs:296— stdin batch under--format jsonprocess::exit(1)s to suppress the error prefix; should be a first-classError::AlreadyReported.Praise
diff.rs:132— merge-base default with--from-tipescape hatch is the right model for PR-style diffing.Test plan
cargo test --all-featurespasses (14/14).cargo clippy --all-targets --all-features -- -D warningsclean.qualifier lsshows the 9 new annotations on the touched files.qualifier diff mainoutput renders the new findings.Need help on this PR? Tag
@codesmithwith what you need.