Skip to content

Add --only-commit flag to stellar version#2592

Merged
leighmcculloch merged 4 commits into
mainfrom
feat-stellar-version-commit-sha
May 28, 2026
Merged

Add --only-commit flag to stellar version#2592
leighmcculloch merged 4 commits into
mainfrom
feat-stellar-version-commit-sha

Conversation

@leighmcculloch

Copy link
Copy Markdown
Member

What

Add an --only-commit flag to stellar version that prints just the commit sha of the build, alongside the existing --only-version and --only-version-major flags.

Why

The commit sha is already embedded in the build and shown as part of the full version output, but there is no way to extract it on its own. A dedicated flag makes the commit sha consumable by scripts and tooling without needing to parse the multi-line full version output.

Known limitations

N/A

@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX May 28, 2026
@leighmcculloch leighmcculloch marked this pull request as ready for review May 28, 2026 12:39
@leighmcculloch leighmcculloch requested review from Copilot and fnando May 28, 2026 12:39
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 19s —— View job


Claude PR Review

  • Load context and changed file
  • Review the diff for correctness, quality, and edge cases
  • Post review feedback

Small, well-scoped change. The new flag mirrors the existing --only-version / --only-version-major pattern exactly and reuses the already-exposed git() helper, so there's no new surface area. Logic looks correct.

Findings

1. FULL_HELP_DOCS.md is out of date (medium)

FULL_HELP_DOCS.md is auto-generated by cargo run --package doc-gen (per Makefile:46-48) and currently lists only the existing two flags at lines 4503–4504. With this PR it should also include --only-commit. Running make docs should regenerate it.

- `--only-version` — Print only the version
- `--only-version-major` — Print only the major version
+ `--only-commit` — Print only the commit sha

Fix this →

2. Empty-string output when GIT_REVISION is unset (low / consider)

git() returns env!("GIT_REVISION"), which can be the empty string. long() handles this defensively (version.rs:47-51 omits the (…) segment when empty), but --only-commit will just print a blank line. That's probably fine for script consumers (they can test for empty output), but it's worth a deliberate decision — alternatives: print nothing and exit non-zero, or print unknown. Up to you; the current behavior is consistent with the sibling flags and arguably the most script-friendly.

3. Mutually exclusive flags fail silently (nit, pre-existing)

--only-version --only-commit would silently pick --only-version because of the if / else if ordering. This isn't introduced by your PR — it's the existing pattern — but adding a third flag makes it slightly more likely a user combines them. Could be addressed later with clap's conflicts_with_all or an ArgGroup. Not blocking.

Other notes

  • No tests exist for the sibling flags, so the absence of a test here is consistent with the surrounding code.
  • No security or performance concerns — output is a build-time constant.

Overall: LGTM once FULL_HELP_DOCS.md is regenerated.
• Branch: feat-stellar-version-commit-sha

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a --only-commit output mode to the stellar version command so scripts can retrieve the embedded git SHA without parsing the multi-line version output.

Changes:

  • Introduces --only-commit flag to print just the build’s commit SHA.
  • Updates stellar version dispatch logic to support the new output mode.

Comment thread cmd/soroban-cli/src/commands/version.rs
Comment thread cmd/soroban-cli/src/commands/version.rs
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 49s —— View job


Claude PR Review

  • Load context and changed files
  • Review the diff for correctness, quality, and edge cases
  • Verify previous review feedback was addressed
  • Post review feedback

The follow-up commit 74c53e8 regenerates FULL_HELP_DOCS.md, which addresses the previous review's only blocker — --only-commit is now documented at FULL_HELP_DOCS.md:4909. The functional change in cmd/soroban-cli/src/commands/version.rs remains a clean, well-scoped addition that mirrors the sibling --only-version / --only-version-major flags.

Findings

1. Doc regen produced a much larger diff than expected — verify it's intentional (medium / worth a quick sanity check)

74c53e8 is +2314/-1811 in FULL_HELP_DOCS.md, far more than what the new --only-commit line accounts for. Skimming the diff, this is almost entirely a syntax refactor of the existing content (bullets -*, blank-line spacing tweaks, etc.). Both clap-markdown rev and cmd/doc-gen/src/main.rs are unchanged between origin/main and HEAD, which means FULL_HELP_DOCS.md on main was drifted from what make docs actually emits — and your regen reconciles it.

That's fine, but two things worth confirming before merge:

  • Make sure no human-curated content in FULL_HELP_DOCS.md was unintentionally lost in the regen (the file is auto-generated per Makefile:46-48, so in principle there shouldn't be any, but the size of the diff makes it worth a 30-second scan).
  • Consider splitting the formatting reflow out of this PR (or at minimum keeping it as the separate commit it already is) so reviewers and git blame aren't muddled. The commit boundary you already have (74c53e8 for the reflow, a6b2208 for the feature) is good — please don't squash on merge.

2. Mutually-exclusive flags still resolved by if/else precedence (low, pre-existing — also raised by copilot)

--only-version --only-commit silently picks --only-version (version.rs:21-29). Not introduced by this PR, but adding a third flag in the chain makes accidental combinations more likely. clap's ArgGroup with multiple = false or conflicts_with_all = ["only_version", "only_version_major"] would make the CLI reject the combination instead of guessing. Not blocking — if you want to defer to a follow-up, that's reasonable.

3. No test for --only-commit (nit — copilot also raised this)

cmd/crates/soroban-test/tests/it/version.rs exercises the default stellar version output but has no coverage for any of the --only-* flags. Adding a case here would be cheap and lock in the contract for script consumers:

#[test]
fn version_only_commit() {
    let sandbox = TestEnv::default();
    sandbox
        .new_assert_cmd("version")
        .arg("--only-commit")
        .assert()
        .success()
        .stdout(format!("{}\n", soroban_cli::commands::version::git()));
}

Consistent with the lack of coverage for --only-version / --only-version-major, so not blocking — but if you're adding one, it'd be reasonable to add the trio.

4. Empty-string output when GIT_REVISION is unset (previously flagged, still worth a deliberate decision)

git() returns env!("GIT_REVISION") (version.rs:41-43), which can be empty. long() defensively omits the (…) segment in that case (version.rs:47-51), but --only-commit will print a blank line. This is consistent with how --only-version / --only-version-major behave for their respective sources, and arguably the most script-friendly behavior (a script can test for empty output). Calling it out as a deliberate-and-fine choice, not a request to change.

Fix #2 (add ArgGroup) →Fix #3 (add test) →

Summary

LGTM. The feature itself is correct and matches the established pattern; the previous blocker (regenerating FULL_HELP_DOCS.md) is resolved. The only thing I'd encourage before merge is a quick eyeball over the large doc diff to confirm it's pure regen drift and no hand-curated content was lost — beyond that, the open suggestions (mutex enforcement, test) are nice-to-have rather than blocking.
• Branch: feat-stellar-version-commit-sha

@leighmcculloch leighmcculloch enabled auto-merge (squash) May 28, 2026 12:59
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 28s —— View job


Claude PR Review

  • Load context and changed files
  • Inspect the new commit d05a4dc (docs refactor)
  • Review the diff for correctness, quality, and edge cases
  • Post review feedback

The latest push (d05a4dc) cleans up the docs noise from the previous round — the net diff against origin/main is now exactly what the PR sets out to do:

FULL_HELP_DOCS.md                       | 1 +
cmd/soroban-cli/src/commands/version.rs | 5 +++++
2 files changed, 6 insertions(+)

That fully resolves my previous concern about the +2314/-1811 reflow in 74c53e8 muddying the diff. d05a4dc looks like it reverts the formatting churn while keeping the one substantive doc line (FULL_HELP_DOCS.md:4505). The three-commit history is still a bit noisy (feature → unintended reflow → revert-of-reflow), so if you'd like a tidier blame, squashing on merge would collapse it to a single conceptual change — but the net state is correct either way.

Findings

1. Mutually-exclusive flags still resolved by if/else precedence (low, pre-existing — copilot also raised this)

--only-version --only-commit silently picks --only-version (cmd/soroban-cli/src/commands/version.rs:21-29). Not introduced by this PR, but adding a third flag in the chain makes accidental combinations more likely. clap's ArgGroup with multiple = false or conflicts_with_all = ["only_version", "only_version_major"] on each flag would make the CLI reject the combination instead of guessing. Not blocking — reasonable as a follow-up.

Fix this →

2. No test for --only-commit (nit — copilot also raised this)

cmd/crates/soroban-test/tests/it/version.rs exercises default stellar version output but has no coverage for any of the --only-* flags. A test would lock in the contract for script consumers:

#[test]
fn version_only_commit() {
    let sandbox = TestEnv::default();
    sandbox
        .new_assert_cmd("version")
        .arg("--only-commit")
        .assert()
        .success()
        .stdout(format!("{}\n", soroban_cli::commands::version::git()));
}

Consistent with the lack of coverage for the sibling flags, so not blocking — but if you add one, adding the trio would be cheap.

Fix this →

3. Empty-string output when GIT_REVISION is unset (previously flagged, still applies)

git() returns env!("GIT_REVISION") (cmd/soroban-cli/src/commands/version.rs:41-43), which can be empty. long() defensively omits the (…) segment in that case, but --only-commit will print a blank line. Consistent with how the sibling flags behave for their respective sources, and arguably the most script-friendly behavior. Calling it out as a deliberate-and-fine choice, not a request to change.

Summary

LGTM. The previously-flagged doc-diff concern is resolved; remaining items (ArgGroup, tests) are nice-to-have and consistent with the surrounding code's style. Net diff is minimal and focused.
• Branch: feat-stellar-version-commit-sha

@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 17s —— View job


Claude PR Review

  • Load context and inspect new commit 67689f3
  • Review the diff for correctness, quality, and edge cases
  • Verify previous review feedback was addressed
  • Post review feedback

The new commit 67689f3 addresses the mutually-exclusive-flags concern that copilot and I both flagged previously. The implementation uses clap's group = "only" attribute on all three flags (cmd/soroban-cli/src/commands/version.rs:8,11,14), which is the idiomatic clap-derive approach: an implicit ArgGroup is created with the default multiple = false, so clap will now reject combinations like --only-version --only-commit at parse time rather than silently picking one by if/else precedence. Nice — this is cleaner than conflicts_with_all, since adding a fourth flag in the future just needs group = "only" rather than updating N×N conflict lists.

Net PR diff against origin/main is now:

FULL_HELP_DOCS.md                       | 1 +
cmd/soroban-cli/src/commands/version.rs | 7 +++++--
2 files changed, 7 insertions(+), 2 deletions(-)

Small, focused, and correct.

Findings

1. No test for --only-commit or the new mutex behavior (nit — copilot also raised this)

cmd/crates/soroban-test/tests/it/version.rs still only covers the default stellar version output. With the new group = "only" constraint, there are now two contracts worth pinning down for script consumers:

  • stellar version --only-commit prints git() + newline
  • stellar version --only-version --only-commit exits non-zero (clap rejects it)

The second one is the more valuable test, since the group attribute is easy to accidentally delete in a future refactor and a parse-time failure wouldn't be caught by a happy-path test. Still not blocking — consistent with the lack of coverage for the sibling flags.

#[test]
fn version_only_commit() {
    let sandbox = TestEnv::default();
    sandbox
        .new_assert_cmd("version")
        .arg("--only-commit")
        .assert()
        .success()
        .stdout(format!("{}\n", soroban_cli::commands::version::git()));
}

#[test]
fn version_only_flags_are_mutually_exclusive() {
    let sandbox = TestEnv::default();
    sandbox
        .new_assert_cmd("version")
        .args(["--only-version", "--only-commit"])
        .assert()
        .failure();
}

Fix this →

2. Empty-string output when GIT_REVISION is unset (previously flagged, deliberate-and-fine)

git() returns env!("GIT_REVISION") (cmd/soroban-cli/src/commands/version.rs:41-43), which can be empty. long() defensively omits the (…) segment in that case (version.rs:47-51), but --only-commit will print a blank line. Consistent with the sibling flags' behavior for their respective sources, and arguably the most script-friendly choice (a script can test for empty output). Noting it as a settled decision, not a request to change.

Summary

LGTM. All previously-raised review items either resolved (FULL_HELP_DOCS.md regen, mutex enforcement) or explicitly settled (empty-GIT_REVISION behavior). Only outstanding item is the absence of tests, which is consistent with the surrounding code and reasonable to defer.
• Branch: feat-stellar-version-commit-sha

Comment thread cmd/soroban-cli/src/commands/version.rs
@leighmcculloch leighmcculloch merged commit f5f9d4e into main May 28, 2026
222 checks passed
@leighmcculloch leighmcculloch deleted the feat-stellar-version-commit-sha branch May 28, 2026 22:25
@github-project-automation github-project-automation Bot moved this from Backlog (Not Ready) to Done in DevX May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants