Skip to content

Remove duplicate version flag handling from PersistentPreRunE#31

Draft
Copilot wants to merge 25 commits into
29-add-binary-creationfrom
copilot/sub-pr-30
Draft

Remove duplicate version flag handling from PersistentPreRunE#31
Copilot wants to merge 25 commits into
29-add-binary-creationfrom
copilot/sub-pr-30

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 15, 2025 •
edited
Loading

The version flag was being handled in both PersistentPreRunE and Execute(), creating redundant logic that needed maintenance in two places.

Changes

  • Removed PersistentPreRunE function with duplicate version flag handling from cmd/root.go
  • Removed errVersionShown sentinel error pattern
  • Retained early version flag check in Execute() that bypasses config initialization

The Execute() implementation correctly handles --version/-v before Cobra's command execution chain, eliminating the need for the PersistentPreRunE hook.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Anthony-Bible and others added 25 commits December 7, 2025 14:13
@Anthony-Bible @claude
feat(version): add failing tests for version system (TDD Red)
56624cf 
Tests for:
- Version variable initialization and ldflag injection
- Version command output format with --short flag support
- --version flag functionality on root command
- Version display without config dependency
- Graceful error handling for missing values

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
feat(version): implement version system (TDD Green)
a85c7fc 
- Add Version, Commit, BuildTime variables to cmd/root.go for ldflags injection
- Implement actual version display in version command with --short flag support
- Add --version and -v flags to root command that bypass config loading
- Ensure version works without configuration dependency
- All version tests now passing

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
refactor(version): improve version implementation (TDD Refactor)
79fd8f6 
- Extract version logic to dedicated internal/version package
- Centralize version data in VersionInfo struct with proper methods
- Add separation of concerns between CLI and version formatting
- Improve error handling for missing git info and build timestamps
- Eliminate code duplication and magic strings
- Add comprehensive documentation and Go idioms
- Maintain backward compatibility with existing build systems

All tests continue to pass with improved code organization.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
test(cmd): clean up version tests after refactor
06941b9 
- Remove duplicate flag setup in root tests
- Use actual runVersion function in version tests
- Remove unused slogger import
- Tests now properly use the refactored implementation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
feat(build): add install targets to Makefile
2ba4904 
- Add install target for main binary installation
- Add install-client target for client binary installation
- Add build-with-version target with ldflags for version injection
- Support cross-platform installation paths (Unix/Linux/macOS/Windows)
- Automatically detect GOPATH/bin or USERPROFILE/bin
- Provide user feedback about installation location and PATH requirements

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
ci(release): update GitHub Actions and fix version injection
7ca59ef 
- Replace deprecated create-release and upload-release-asset actions with softprops/action-gh-release@v2
- Fix version ldflags to use correct path (cmd.Version vs main.Version)
- Add client binary (codechunking-client) to releases alongside main binary
- Generate SHA256 checksums for all release assets
- Improve workflow structure with separate build and release jobs
- Add build arguments to Docker builds for version metadata
- Auto-generate release notes from git history

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
fix(build): correct ldflags path for version injection
3329203 
- Fix Makefile to use full module path in ldflags (codechunking/cmd.*)
- Update GitHub Actions workflow to use correct path for ldflags
- Remove debug code from version sync function
- Version injection now works correctly with build-with-version target

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
test(scripts): add failing tests for build automation (TDD Red)
062106a 
Tests for build.sh:
- Version handling from file and command line
- Git commit info integration
- Binary output verification (main and client)
- Correct ldflags usage for version injection
- Error handling for missing dependencies
- Various build modes (clean, verbose, cross-platform)

Tests for release.sh:
- Version argument validation
- VERSION file management
- Build script execution
- Release directory structure
- Binary versioning and copying
- SHA256 checksum generation
- Dry run and git tag functionality

All tests currently failing as scripts don't exist yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
feat(scripts): implement build and release automation (TDD Green)
b27299e 
- Add scripts/build.sh for version-aware builds with cross-platform support
- Add scripts/release.sh for automated release creation with checksums
- Add VERSION file to track current version (0.1.0)
- Remove scripts/*.sh from .gitignore to track build automation
- Scripts support CGO_ENABLED=1 for main binary (tree-sitter dependency)
- Scripts build client binary statically (CGO_ENABLED=0)
- Core functionality verified: builds work manually with version injection

Note: Some tests failing due to timeouts and unbound variable issues;
will be addressed in refactor phase. Core build functionality is working.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
refactor(scripts): improve build automation robustness (TDD Refactor)
304da09 
- Fix unbound variable error by using proper bash parameter expansion
- Change validate_version to return 1 instead of exit 1 for better error handling
- Add platform option value validation to prevent cross-platform test failures
- Preserve empty string arguments for validation testing
- Add TEST_MODE environment variable support for faster builds in tests
- Improve argument parsing and validation logic
- All individual tests now pass with TEST_MODE optimizations

Core functionality improvements:
- validate_version uses ${1:-} for safe parameter handling
- Platform flag requires explicit value checking with ${2:-}
- Empty arguments preserved for validation test coverage
- Build optimizations (-w -s flags) applied in TEST_MODE for faster execution

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible @claude
docs: update documentation for Issue 29 - Binary Creation Functionality
e3ff049 
- Fix README.md installation section with correct module paths
- Add comprehensive binary download instructions from GitHub releases
- Document version command usage and expected output format
- Clarify differences between main binary and client binary
- Create detailed INSTALL.md guide covering multiple installation methods
- Document build scripts usage and cross-platform compilation
- Update Makefile to use new build script by default
- Add troubleshooting section for common installation issues
- Include Docker installation instructions
- Document CGO requirements for tree-sitter support

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible
chore(release): bump VERSION to v1.4.0
8ac0c74 
test(scripts): always append version arg in build_test; include stdout/stderr in release_test failure message
@Anthony-Bible
Remove squash guide
520dbe9
@Anthony-Bible
fix(version,cli,build): handle --version early, unify ldflags, and ha…
e2e9455 
…rden build/release scripts

- CLI: show version and prevent command execution when --version/-v used; skip full config init for version or "version" subcommand
- cmd: avoid resetting internal/version in syncLegacyVersionVars; only set when vars present
- tests: reset internal/version state before/after version tests
- build: use full package path for ldflags (codechunking/cmd) in GitHub workflow and Dockerfile
- scripts: document get_version, move TEST_MODE optimizations earlier, use safe run_cmd invocation (no eval), and simplify/strict copy_binaries behavior
- docs: update README and INSTALL expected version output format
@Anthony-Bible @claude
fix(cli): address PR review comments for version handling
da18f63 
- Simplify PersistentPreRunE version flag handling with sentinel error pattern
- Remove unreliable c.Run = func(){} workaround (Copilot review comment)
- Use errVersionShown sentinel to properly stop command execution
- Update Execute() to handle sentinel error gracefully (exit 0)
- Clarify intentional config loading design (not temporary code)
- Add comprehensive test coverage for internal/version package

The version flag now reliably prevents subcommand execution by returning
a sentinel error that Execute() filters out. This is cleaner than the
previous approach of setting c.Run to an empty function.

Addresses Copilot review comments on PR #30

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Anthony-Bible
refactor(cmd): simplify version handling; remove sentinel error
6737678 
- remove errVersionShown sentinel and PersistentPreRunE early-exit logic in cmd/root.go; adjust Execute error handling
- update handleConfigError comment
- update tests to call runVersion directly and use version package helpers (cmd/root_test.go)
- bump VERSION to v3.0.0 and standardize displayed versions with leading "v" in README.md and INSTALL.md
- remove legacy script tests under scripts/test (build_test.go, release_test.go)
@Anthony-Bible
Update Makefile
dd18cbe 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update README.md
49319a8 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update scripts/release.sh
f0f0f6a 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update scripts/release.sh
923288a 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update INSTALL.md
c13173a 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update internal/version/version.go
a1bc3dd 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update README.md
08fedb7 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Anthony-Bible
Update scripts/build.sh
2cdab03 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Initial plan
70b57cf
Copilot AI mentioned this pull request Dec 15, 2025
Merged
Copilot AI changed the title [WIP] Update to address feedback on binary creation implementation Remove duplicate version flag handling from PersistentPreRunE Dec 15, 2025
Copilot AI requested a review from Anthony-Bible December 15, 2025 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Anthony-Bible Anthony-Bible Awaiting requested review from Anthony-Bible

Assignees

@Anthony-Bible Anthony-Bible

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Anthony-Bible