Skip to content

fix(cli): close 7 review findings on default subcommand + --command#13

Merged
brettdavies merged 5 commits intodevfrom
feat/post-review-fixes-003
Apr 16, 2026
Merged

fix(cli): close 7 review findings on default subcommand + --command#13
brettdavies merged 5 commits intodevfrom
feat/post-review-fixes-003

Conversation

@brettdavies
Copy link
Copy Markdown
Owner

Summary

Resolves the seven recommended fixes from the post-merge /ce-review of PR #12 (commit
4afef67). All actionable findings closed; main.rs trimmed back under the 200-line trigger.

Changelog

Added

  • value_hint = ValueHint::CommandName on --command so zsh, fish, and elvish completions
    suggest PATH commands instead of file paths. Bash is patched post-generation in
    scripts/generate-completions.sh.
  • after_help text on Cli documenting the implicit default subcommand and the
    bare-invocation contract directly in anc --help output.
  • Mutual exclusion: --command and --source now error at parse time instead of
    silently producing an empty result.

Changed

  • anc -q / anc --quiet (top-level flag without subcommand) now prints help and exits 2
    instead of panicking via unreachable!() (pre-existing bug).
  • anc help and anc help check now work — clap's auto-generated help subcommand was
    missing from our known-subcommand set and got misclassified as a path.
  • anc --command <NAME> where NAME collides with a subcommand name (e.g.
    anc --command check) now resolves NAME as a binary on PATH instead of producing a
    confusing clap error.
  • anc --command rg and anc --output json --source (no positional argument) now work —
    the pre-parser detects subcommand-scoped flags and injects check accordingly.
  • anc -- . (POSIX double-dash separator) now runs check against . instead of
    producing undefined behavior.

Documentation

  • README and AGENTS.md exit-code tables clarify that exit 2 is overloaded (failures,
    errors, and usage errors all share it). Suggest parsing stderr (Usage: text) to
    distinguish.

Type of Change

  • fix: Bug fix (non-breaking change which fixes an issue)

Related Issues/Stories

Findings Resolved

# Severity Title Resolution
1 P2 clap help subcommand missing from known set Append \"help\" to known after get_subcommands()
2 P2 --command <NAME> value collides with subcommand Pair value-taking flags with their values via clap introspection
3 P2 Flag-value tokens silently fragile Same fix as #2; also tracks subcommand-scoped flags to inject check when no positional follows
4 P2 anc -q panics via unreachable!() (pre-existing) None arm now renders help to stderr and exits 2
5 P2 --command + --source silent empty result conflicts_with = \"source\" on command arg
6 P2 Bare anc exit 2 conflates user-error vs check-failure Documented in README/AGENTS exit-code tables
7 P2 Default-subcommand magic not in --help after_help block on Cli documents it
8 P2 anc -- . POSIX double-dash undefined New branch in inject_default_subcommand
10 P2 main.rs over 200-line refactor trigger Extract inject_default_subcommand + tests to new src/argv.rs module (538 → 203 lines)
14 P3 Bash --command completion uses compgen -f ValueHint::CommandName fixes zsh/fish/elvish; bash post-patched in gen script
22 P3 anc . succeeds where it previously errored Documented in PR Changelog above

Findings not addressed (intentional): #11/#12/#17/#18 (subsumed by #10 refactor), #15/#16/#19/#20/#21/#23/#24
(low-priority test/style nits or pre-public-schema concerns).

Testing

  • Unit tests added/updated (244 → 253, +9 covering all 5 fixed edge cases)
  • Integration tests added/updated (26 → 34, +8 covering same)
  • All tests passing
  • Pre-push hook clean (fmt + clippy -Dwarnings + test + Windows compat)
  • Manual probes: every previously-broken invocation now does the right thing,
    bare anc still exits 2 (fork-bomb guard intact), explicit subcommands still work

Test Summary:

  • Unit tests: 253 passing (was 244)
  • Integration tests: 34 passing, 3 ignored (was 26 / 3)
  • Coverage: every fix has both a unit test (where applicable) and an integration test

Files Modified

Modified:

  • src/cli.rsafter_help text, value_hint on --command, conflicts_with = \"source\"
  • src/main.rsNone arm prints help instead of panicking; refactor extracts injection
    to argv module
  • tests/integration.rs — 8 new tests covering the fixed edge cases
  • scripts/generate-completions.sh — post-process bash to substitute compgen -c for
    --command (clap_complete bash backend ignores ValueHint::CommandName)
  • completions/anc.{bash,zsh,fish} — regenerated to expose new behavior
  • README.md, AGENTS.md — exit-code clarification

Created:

  • src/argv.rs — new module hosting inject_default_subcommand and 19 unit tests

Breaking Changes

  • No breaking changes

Every previous valid invocation continues to produce identical output. Previously-broken
invocations (anc help, anc -q, anc --command check, anc --command rg,
anc --output json --source, anc -- .) now behave correctly.

Deployment Notes

  • No special deployment steps required

Ships with Plan 003 in the v0.1.0 release/* batch when Plan 002 (release infra) runs.
No standalone release branch.

Checklist

  • Code follows project conventions and style guidelines
  • Commit messages follow Conventional Commits
  • Self-review of code completed
  • Tests added/updated and passing
  • No new warnings or errors introduced
  • Changes are backward compatible

Five issues surfaced by post-merge code review on commit 4afef67:

- `anc help` and `anc help check` were misclassified as paths because
  clap's auto-generated `help` subcommand is not returned by
  `Cli::command().get_subcommands()`. Add it explicitly to the known set.
- `anc --command <X>` where X collides with a subcommand name (e.g.
  `anc --command check`) classified X as the explicit subcommand instead
  of the flag's value. The pre-parser now consults clap introspection to
  learn which flags consume the next token and skips both as a unit.
- `anc --command rg` and `anc --output json --source` (no positional
  argument, only subcommand-scoped flags) failed because the pre-parser
  needs no positional to inject `check`. It now tracks whether any
  encountered flag was subcommand-scoped and injects accordingly.
- `anc -- .` (POSIX double-dash separator) was untested and ill-defined.
  Now injects `check` before the separator so clap routes `. ` to Check.
- `anc -q` / `anc --quiet` (top-level flag without subcommand) panicked
  via `unreachable!()` — pre-existing bug. The `None` arm now renders
  help to stderr and exits 2, mirroring `arg_required_else_help`.

Also from the same review:

- Add `conflicts_with = "source"` on `--command` so the contradictory
  combination errors at parse time instead of producing a silent empty
  result.
- Add `value_hint = ValueHint::CommandName` on `--command` so zsh, fish,
  and elvish completions suggest PATH commands instead of file paths. Bash
  is patched post-generation in scripts/generate-completions.sh because
  clap_complete's bash backend ignores the hint.
- Add `#[command(after_help = ...)]` on `Cli` documenting the implicit
  default subcommand and the bare-invocation contract directly in
  `anc --help` output.

README and AGENTS.md updated to clarify that exit 2 is overloaded
(failures, errors, and usage errors all share it).

Test parity preserved: 244 unit + 26 integration before, 253 unit + 34
integration after. Pre-push hook clean (fmt, clippy -Dwarnings, test,
Windows compat).
main.rs grew to 538 lines after Plan 003 — well over the 200-line
refactor trigger in CLAUDE.md. Most of the growth was pre-parse logic
and its tests, which have nothing to do with main.rs's orchestration
role.

Move the entire argv-injection machinery (`inject_default_subcommand`
plus its 19 unit tests) into a new `src/argv.rs` module. main.rs goes
from 538 → 203 lines and recovers focus on its single job: parse, route
to a subcommand, run checks, format output.

`resolve_command_on_path` stays in main.rs — it's small (~25 lines),
operates on AppError, and is only used by run(). Splitting it would
trade size for indirection.

No behavior change. Test parity preserved.
CLAUDE.md previously mentioned `docs/solutions/` only as ce-compound's
output destination. Agents in fresh sessions had no signal that the
directory was a searchable archive of past solutions worth checking
before re-researching a documented problem.

Add a brief Documented Solutions section noting the symlink target,
frontmatter fields, and `qmd query` invocation. Phrased as awareness,
not a directive — agents apply judgment about when to consult it.
Two changes bundled because they share the same workflow:

1. Update Plan 003 to reflect the as-shipped reality. The original plan
   documented the design that landed in PR #12, but post-merge ce-review
   surfaced 7 edge cases that PR #13 closed. Adds:
   - Status block at top with PR #12/#13 links and a pointer to the
     compounded learning in docs/solutions/.
   - status: shipped + shipped: 2026-04-15 in frontmatter.
   - Annotated deferred-questions with status notes.
   - New Post-Implementation Notes section: final code locations
     including src/argv.rs, the 7 edge cases beyond the original design,
     design-time decisions that survived contact with reality, and
     test-parity table.

2. Refresh .markdownlint-cli2.yaml from the dotfiles canonical
   (~/dotfiles/stow/claude/dot-markdownlint-cli2.yaml). The local copy
   was 1096 bytes from February; the canonical is 1802 bytes with proper
   MD013 line_length: 120 plus a fuller rule set. The stale config was
   wrapping prose too aggressively.
Adds the new `target/**` ignore (Rust build artifacts) and the
`# Canonical version: 2026.04.15` calver line. Per-repo copies
preserve that calver line so future drift checks are a one-grep
comparison against the dotfiles canonical.
@brettdavies brettdavies merged commit fa58f89 into dev Apr 16, 2026
6 checks passed
@brettdavies brettdavies deleted the feat/post-review-fixes-003 branch April 16, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant