Skip to content

Add bv version command group with show and advance subcommands - #313

Merged
rdeago merged 3 commits into
Tenacom:mainfrom
rdeago:issue/274-bv-version-command
Aug 2, 2026
Merged

Add bv version command group with show and advance subcommands#313
rdeago merged 3 commits into
Tenacom:mainfrom
rdeago:issue/274-bv-version-command

Conversation

@rdeago

@rdeago rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member

Proposed changes

This is the last phase of #267 (buildvana.json configuration and native versioning): a bv version command group for working with native versioning outside a release.

  • bv version show (default subcommand, so plain bv version works too) prints the computed current version, latest and latest stable published versions, public-release/prerelease flags, and current branch. The report is the command's deliverable: it prints to the console regardless of verbosity, and the command defaults to minimal verbosity so the report is all you get by default (--verbosity normal restores diagnostics) — the same results-vs-logs separation git/npm/dotnet query commands use.
  • bv version advance [CHANGE] applies a version-spec change (none | unstable | stable | minor | major, same vocabulary as bv release --bump) to the VERSION file. By default the request runs through the same analysis as bv release (latest-stable comparison + public API check, honoring --check-public-api / release.checkPublicApi), so bare bv version advance means "make VERSION comply with SemVer". --force applies the request verbatim. The change is left uncommitted for review.

To support this, the bespoke CLI framework gained real subcommand support: commands register pipe-separated aliases of space-separated paths ("version show | version" — first alias canonical, segments lowercased at registration), the registry arranges them in a tree walked at dispatch (children win over positionals; duplicate paths fail fast), settings types declare positional arguments via the new BvArgumentAttribute (<NAME> required, [NAME] optional), and commands can declare a default verbosity. Help pages render SUBCOMMANDS (with a [default] marker) and ARGUMENTS sections.

Deltas from the issue text

The issue predates Phase 7's final shape:

  • No Spectre.Console.Cli — the bespoke framework was extended with a command tree instead.
  • The version source of truth is the VERSION file (not current-version.json), and the method is VersionFile.ApplyChange (not ApplyVersionSpecChange).
  • --commit was dropped by design decision: commit-message conventions ([skip ci] placement etc.) vary by host, and git commit is one command away.

Checklist of related issues / discussions

Types of changes

This pull request introduces the following types of changes:

  • Bug fix
  • New feature
  • Test addition / update (no changes to non-test code)
  • Refactor (no changes in public API syntax or semantics)
  • Performance improvement (no changes in public API syntax or semantics)
  • Documentation (docs directory) update
  • Dependency addition / update
  • Changes to the build scripts
  • Changes to CI (workflows, bot / app configurations)
  • Changes to repository files (.gitattributes, .gitignore)
  • Other

Breaking changes

This pull request introduces breaking changes:

  • Yes
  • No

Checklist

  • For all types of changes:
  • For code changes only:
    • The project builds on my machine, via the provided build script, with zero warnings
    • I have added tests that prove my feature works / my fix is effective
    • I have added / modified XML documentation according to changes in code
    • I have checked that all the links I added or modified in XML documentation point to their intended destination

🤖 Generated with Claude Code

rdeago and others added 2 commits August 2, 2026 03:48
The bespoke CLI framework now supports multi-level subcommands: commands
register pipe-separated aliases of space-separated paths (first alias is
canonical, segments are lowercased), and the registry arranges them in a
tree walked at dispatch time. Settings types can declare positional
arguments via the new BvArgumentAttribute; commands can declare a default
verbosity, so query commands can default to Minimal and print their
deliverable unconditionally via IAnsiConsole.

`bv version show` (default subcommand of `bv version`) reports current,
latest, and latest stable versions, release flags, and current branch.
`bv version advance [CHANGE]` applies a version-spec change to the VERSION
file through the same analysis as `bv release` (`--force` skips it),
leaving the change uncommitted for review.

Closes Tenacom#274.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The property exists to satisfy CA1019; asserting it in the attribute
tests also keeps ReSharper from flagging its getter as unused.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rdeago rdeago added the enhancement [issue/PR] requests / implements new or improved functionality. label Aug 2, 2026
@github-actions github-actions Bot added area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). area:code [issue/PR] affects project code (excluding tests). labels Aug 2, 2026
@rdeago

rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

@claude please review this PR.

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @rdeago's task in 3m 57s —— View job


Claude finished reviewing this PR

  • Gather context (read diff, changed files)
  • Understand the request
  • Review command tree / registry changes
  • Review bv version show / bv version advance commands (verified service APIs)
  • Review new attribute/validator/help-renderer changes
  • Review tests
  • Post final review feedback

Summary

This is a solid, well-tested implementation. The command-tree extension to the bespoke CLI framework (CommandNode, CommandRegistry.Resolve/BuildTree, BvArgumentAttribute, help-renderer updates) is clean, and the new version show/version advance commands wire correctly into VersionService, VersionFile, and GitService. I traced through the alias/tree-walk logic (version bare → version show alias, version advance minor → node walk + leftover positional, unknown-subcommand vs. unexpected-positional distinction) and it's internally consistent with the accompanying tests in CommandRegistryTests.cs / CommandArgumentValidatorTests.cs.

I also verified the service-layer wiring for VersionAdvanceCommand:

  • VersionService.ComputeVersionSpecChange, VersionFile.ApplyChange/.Save, GitService.CurrentBranch all exist with the signatures used, no typos/mismatches.
  • ApplyChange correctly returns false for a no-op None request, and VersionAdvanceCommand handles that with the "Version spec not changed." message without calling Save.
  • VersionFile.Save (src/Buildvana.Core.Versioning/VersionFile.cs:87) is a pure local File.WriteAllText — no git staging/commit anywhere in the path, matching the "left uncommitted for review" design in the PR description.
  • One thing worth being aware of (not a bug, just confirming intended behavior): ComputeVersionSpecChange (VersionService.cs:124-186) overrides an explicit stable/unstable request with Major/Minor when the SemVer analysis determines an increment is required — so bv version advance stable isn't guaranteed to actually produce a stable version if the analysis disagrees. That matches "bare bv version advance means make VERSION comply with SemVer" from the PR description, but might surprise someone passing an explicit change without --force.

Minor observations

  1. BvArgumentAttribute.cs:16-17 documents an invariant — "Multiple arguments bind in property declaration order, which must list the required ones first" — but nothing enforces it (unlike the analogous NodeDescriptions typo-guard the same PR adds in CommandRegistry.BuildTreeAndValidateDescriptions). Not exploitable today since VersionAdvanceSettings declares a single optional argument, but if a future command declares [optional, required] in that order, CommandArgumentValidator's generic bounds-check would still "work" positionally, yet the ordering contract would silently be violated with no test or runtime guard to catch it. Consider either a lightweight static assertion (mirroring the NodeDescriptions pattern) or dropping the sentence if it's meant as guidance only. Fix this →

  2. CommandRegistry.cs and the help renderer have no dedicated unit tests for BvHelpRenderer itself (e.g. WriteGroupHelp/WriteSubcommands output, the [default] marker), but this matches pre-existing practice — there was no BvHelpRenderer test file before this PR either, so it's not a regression, just noting the gap stays open.

Nothing else stood out — style guide adherence (partial classes, extension blocks, line wrapping, BOM/file-format conventions) looks consistent with the rest of the codebase, and the CHANGELOG entry accurately describes the new commands.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rdeago

rdeago commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review! Addressed as follows:

Finding 1 — fixed in eb72e71. The required-before-optional invariant is now enforced, not just documented: CommandRegistry.ValidateArgumentOrder runs at discovery time and throws InvalidOperationException when a settings type declares a required positional argument after an optional one, following the same fail-fast pattern as the existing PipelineCommandNames and NodeDescriptions guards. The invariant matters beyond help output: CommandArgumentValidator checks required-ness by index over the declared list, so a misordered declaration would bind positionals to the wrong properties and produce misleading "missing required argument" errors. The BvArgumentAttribute remarks now point at the enforcement, and two new tests cover both sides of the guard using dedicated fake settings types (FakeOrderedArgumentSettings / FakeMisorderedArgumentSettings), since no real command currently declares more than one argument.

Finding 2 — intentionally not addressed. Dedicated BvHelpRenderer tests would amount to golden-text assertions on formatted output: high churn, low bug-catching power, and (as the review notes) consistent with existing practice. If help rendering ever grows real branching logic, that will be the time to add targeted tests.

@claude please take another look.

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @rdeago's task in 1m 29s —— View job


Follow-up review complete

  • Gather context (re-read trigger comment, inspect commit eb72e71)
  • Review commit eb72e71 (required-before-optional enforcement)
  • Verify new tests (FakeOrderedArgumentSettings, FakeMisorderedArgumentSettings, CommandRegistryTests)
  • Confirm Finding 2 rationale (no action needed)
  • Post final feedback

Summary

Finding 1 is properly addressed in eb72e71:

  • CommandRegistry.ValidateArgumentOrder (src/Buildvana.Tool/Infrastructure/Execution/CommandRegistry.cs:143-173) walks each command's settings-type properties in declaration order (same BindingFlags.Public | Instance | DeclaredOnly reflection call already used by CommandArgumentValidator.DeclaredArguments, so the "declaration order" assumption is now consistently relied upon in two places), flags the first [BvArgument]-decorated optional property it sees, and throws InvalidOperationException if a required one follows. This is wired into Discover() (CommandRegistry.cs:206), right after the existing PipelineCommandNames typo-guard and before the sorted list is returned, so a misordered settings type now fails at startup rather than silently miscomputing "missing required argument" in CommandArgumentValidator.Validate (CommandArgumentValidator.cs:50-56) — exactly the failure mode the original finding described.
  • BvArgumentAttribute's remarks (BvArgumentAttribute.cs:16-18) now point at the enforcement instead of just documenting the invariant.
  • The two new fakes (FakeOrderedArgumentSettings, FakeMisorderedArgumentSettings) and their tests (CommandRegistryTests.cs:98-111) cover both the accepting and rejecting paths, matching the existing style of the BuildTree duplicate-path test right above them.

The fix follows the same fail-fast pattern as the pre-existing PipelineCommandNames/NodeDescriptions guards, error message style is consistent, and the exposed-for-testing/production-uses-it-implicitly split mirrors BuildTree. I didn't spot any issues with this change.

Finding 2 (no BvHelpRenderer unit tests): agreed this is fine to leave as-is — golden-text tests on formatted help output would be high-churn for low bug-catching value, and it's a pre-existing gap rather than something introduced by this PR.

Note: I wasn't able to run dotnet build/dotnet bv pack/tests in this session — Bash commands with output redirection or piping required approval that wasn't available non-interactively, so this review is based on static code reading only. The change is small and self-contained (reflection over already-attributed properties, no new dependencies), so I'd expect it to build cleanly, but you may want to confirm locally per the sanity-check workflow in .claude/rules/workflow.md if you haven't already.

Nothing else to flag — this addresses the review feedback well.
· Branch: issue/274-bv-version-command

@rdeago
rdeago merged commit c853808 into Tenacom:main Aug 2, 2026
6 checks passed
@rdeago
rdeago deleted the issue/274-bv-version-command branch August 2, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:code [issue/PR] affects project code (excluding tests). area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). enhancement [issue/PR] requests / implements new or improved functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Versioning Phase 8 — bv version command

1 participant