Conversation
There was a problem hiding this comment.
Pull request overview
Adds a lightweight --version / -V flag to the openab binary so users can quickly confirm the installed version without needing to locate config/chart metadata.
Changes:
- Detect
--version/-Vas the first CLI argument before config resolution. - Print
openab <version>usingenv!("CARGO_PKG_VERSION")and exit successfully. - Preserve existing “first arg = config path” startup flow when the version flag is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@the3mi any inputs? |
|
Thanks for this PR! PR ReviewAuthor Context (verified against current main)
🎨 Design / ArchitectureFound 2 issues: 1. PR is based on stale branch — clap already exists
2. Implementation approach is unnecessary
⚙️ FunctionalityFound 1 issue: 1. Would bypass clap's argument parsing
🔀 Complexity1 issue: More code than needed — clap handles this with one keyword 🧪 Testing1 issue: No automated test for version flag Recommended FixThe goal (adding #[derive(Parser)]
#[command(name = "openab", version)] // ← just add `version`
struct Cli { ... }This gives Suggested next steps:
Let me know if you have any questions! |
|
Thank you very much for the careful review and the very clear guidance. I followed the suggested direction locally and prepared the minimal clap-based fix on top of current
I also verified the local patch with targeted tests and At the moment I am blocked on opening the replacement PR from my fork because my fork is still significantly behind upstream So the next step on my side is straightforward once that fork/token issue is resolved: I can immediately push the already-prepared minimal branch and open the replacement PR exactly in the shape you suggested. Thank you again for the review and for pointing out the correct upstream direction. |
masami-agent
left a comment
There was a problem hiding this comment.
Review — PR #320
Agreeing with the3mi's analysis. The goal is correct but the implementation is based on a stale branch.
Issue
This PR manually parses --version / -V via std::env::args().nth(1), but main already has clap 4 with #[derive(Parser)] (merged via PR #191). The code this PR modifies no longer exists on main.
Fix
On current main, this is a 1-line change:
#[derive(Parser)]
#[command(name = "openab", version)] // just add `version`
struct Cli { ... }clap handles --version, -V, and version display in --help automatically. No manual parsing needed.
Recommendation
@canyugs — please:
- Rebase onto current
main - Replace the manual parsing with
#[command(version)] - Force-push the update
Or close this and open a fresh PR with the 1-line fix. Either way works.
The PR description and prior art research are thorough — just needs the implementation updated to match current main.
8007f3d to
2f7e3fa
Compare
2f7e3fa to
741f968
Compare
|
Addressed all review feedback:
Ready for re-review. |
PR Review — Re-review after updateThank you @chenjian-agent for the revised PR and for pushing the clap-based fix! Author Context
🔀 ComplexityFound 0 issues. Single-line change ( 🎨 Design / ArchitectureFound 0 issues.
⚙️ FunctionalityFound 0 issues. The clap
Manual pre-parse logic is no longer needed. No behavior change to the normal startup flow (version flag short-circuits before subcommand parsing).
|
PR Review — #320 feat: add --version / -V flagPR: feat: add --version / -V flag Dimensions
Gate Checks (CI should verify)
Verdict✅ Approve. This is the correct, idiomatic way to add Reviewed by openab-reviewer agent |
Use clap's built-in version support instead of manual arg parsing. Rebased onto current main which already has clap 4 + subcommands. Change: add `version` to `#[command(name = "openab", version)]`
741f968 to
2d57043
Compare
masami-agent
left a comment
There was a problem hiding this comment.
Re-review — ✅ Approved
Clean rewrite. The stale branch issue is resolved — PR is now rebased on current main with the clap Cli struct.
Change
-#[command(name = "openab")]
+#[command(name = "openab", version)]One line, zero new dependencies. clap automatically provides --version, -V, and version display in --help using env!("CARGO_PKG_VERSION") from Cargo.toml.
Verified
- ✅ Rebased on current
main(hasClistruct withOption<Commands>) - ✅ +1/-1 line — minimal change
- ✅ Single file (
src/main.rs) - ✅ No Cargo.toml/lock changes
Recommend squash merge.
masami-agent
left a comment
There was a problem hiding this comment.
Re-review — PR #320
Clean 1-line change: adds version to the clap #[command] attribute in src/main.rs. This enables openab --version / openab -V using the version from Cargo.toml automatically.
✅ Verified
- Diff is exactly 1 line changed:
#[command(name = "openab")]→#[command(name = "openab", version)] - Rebased on current
main - No test needed — this is a clap framework attribute, not custom logic
- PR description quality is excellent (prior art, alternatives, ASCII diagram)
🟡 NIT (non-blocking)
- PR description mentions
openab 0.7.5in the validation section butCargo.tomlis now0.7.6— cosmetic only, does not affect behavior
LGTM. Approving as agent reviewer. Ready for maintainer merge.
obrutjack
left a comment
There was a problem hiding this comment.
Clean 1-line change, clap handles --version / -V natively from Cargo.toml. No new deps, no risk. LGTM.
Merge checklist:
- ✅ CI green
- ✅ No version regression (0.7.7)
- ✅ Backward compatible
- ✅ No secrets
- ✅ masami-agent approved
Pending code owner review from @thepagent.
|
@the3mi — Done! The branch has been rebased onto current |
What problem does this solve?
Running
openab --versioncurrently fails withfailed to read --version: No such file or directory. There is no way to check which version of openab is installed without inspecting the binary directly or checking the Helm chart. This makes debugging deployment mismatches difficult.Closes #
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365150664560881/1493789779999985785
At a Glance
Prior Art & Industry Research
OpenClaw:
OpenClaw (TypeScript / Commander.js) handles this via custom pre-parse detection in
src/cli/program/help.ts— checks for-V,--version, and a root-level-valias before Commander parses arguments. The version string is resolved dynamically frompackage.jsonwith fallbacks to a build-time__OPENCLAW_VERSION__global. Output format:OpenClaw 2026.4.14-beta.1 (abc1234)including git commit hash.Hermes Agent:
Hermes Agent (Python / argparse) registers
-V/--versionasaction="store_true"on the main parser, plus a dedicatedhermes versionsubcommand. Version is hardcoded inhermes_cli/__init__.py. Output:Hermes Agent v0.9.0 (2026.4.13)with additional Python version and dependency info.Comparison:
-V,--version,-v-V,--version-V,--versionenv!("CARGO_PKG_VERSION")(Cargo.toml)Name X.Y.Z (git-hash)Name vX.Y.Z (date)openab X.Y.ZProposed Solution
Use clap's built-in version support via
#[command(name = "openab", version)]. clap reads the version fromCargo.tomlat compile time viaenv!("CARGO_PKG_VERSION")and automatically handles--versionand-V.Change: 1 word added to
src/main.rs, zero new dependencies.Why This Approach?
clap already in the project.
mainalready has clap 4 with#[derive(Parser)]and subcommands — using#[command(version)]is the idiomatic 1-line solution with no new dependencies.env!("CARGO_PKG_VERSION")over hardcoding. Like OpenClaw'spackage.jsonapproach, this sources the version from the single authoritative location (Cargo.toml) at compile time — no risk of drift.No git hash (unlike OpenClaw). The build environment for release binaries may not have git history available; skipping the hash avoids a fragile build-time
git rev-parsedependency. Can be added later if desired.Alternatives Considered
versionsubcommand (like Hermes'hermes version) — more discoverable but inconsistent with Unix conventions where--versionis the standard flag. Rejected.Validation
cargo checkpassescargo testpassesopenab --versionprintsopenab 0.7.5and exits 0;openab -Vsame;openab run config.tomlstill starts normally