Commit f70da2f
feat: Remove deep exits from version command (#1783)
* feat: Remove deep exits from version command and allow execution with invalid config
The version command is a diagnostic tool that must always work, even when atmos.yaml is invalid, missing, or malformed. This change:
1. Removes the log.Fatal() deep exit from internal/exec/version.go, replacing it with proper error returns
2. Adds version command detection in cmd/root.go PersistentPreRunE to suppress config errors for version
3. Moves InitializeMarkdown() calls after error checking to prevent deep exits with invalid configs
4. Adds comprehensive integration tests ensuring version works with invalid YAML, aliases, and schema
5. Documents the version command implementation and avoiding deep exits pattern in PRDs
6. Excludes test fixtures from YAML validation in pre-commit hooks
This establishes the pattern for refactoring all commands to remove deep exits, enabling proper error handling, better testability, and command composability through the registry pattern.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Enable Forbidigo enforcement for viper.BindEnv/BindPFlag and update flag-handler agent
The pkg/flags/ infrastructure has been fully implemented but documentation and linter rules were outdated, leading to incorrect advice about flag handling.
**Linter Changes (.golangci.yml):**
- Uncommented Forbidigo rules banning viper.BindEnv() outside pkg/flags/
- Uncommented Forbidigo rules banning viper.BindPFlag() outside pkg/flags/
- Updated comments from "FUTURE ENFORCEMENT" to "ENFORCED"
- Linter now catches any direct Viper binding calls and directs to flag-handler agent
**Documentation Changes (CLAUDE.md):**
- Removed outdated "pkg/flags/ does not exist yet" warnings
- Added comprehensive flag handling documentation showing correct StandardParser pattern
- Documented that viper.BindEnv/BindPFlag are BANNED with Forbidigo enforcement
- Added example code showing correct usage with flags.NewStandardParser()
- References cmd/version/version.go as the canonical example
**Agent Updates (.claude/agents/flag-handler.md):**
- Updated description to reflect pkg/flags/ is FULLY IMPLEMENTED
- Expanded AUTO-INVOKE triggers to catch all flag-related discussions
- Removed all "future architecture" warnings
- Updated mission to enforce correct patterns, not describe hypothetical ones
- Agent now correctly states that viper.BindEnv/BindPFlag are banned
This prevents future incidents where incorrect flag handling advice is given. The flag-handler agent should now be automatically invoked whenever flags are discussed, and Forbidigo will catch any violations in code.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Skip markdown initialization when config invalid and add missing periods
Two refinements to configuration error handling and documentation compliance:
**Configuration Error Handling (cmd/root.go):**
- Wrap InitializeMarkdown calls in guard checking if initErr == nil
- Prevents calling markdown renderers when config is invalid
- Previously moved after error check but still called unconditionally
- Now only initializes markdown when config loaded successfully
**Documentation Compliance (CLAUDE.md):**
- Add missing terminal periods per godot linter rules
- Line 386: "...patterns" → "...patterns."
- Line 435: "...implementation" → "...implementation."
- All comments and documentation must end with periods
These changes ensure version command can run even when markdown initialization would fail due to invalid config, and maintain documentation linter compliance.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* docs: Update version command PRD with final implementation and diagnostic rationale
Update the atmos-version-command.md PRD to reflect the final working state of the version command implementation and strengthen the diagnostic command rationale.
**Implementation Updates:**
- Updated Execute() code snippet to show the `if initErr == nil` guard around InitializeMarkdown calls
- Updated line number reference from 568-581 to 571-586 (current state)
- Expanded Key Design Decision explanation with double-guard rationale:
1. Version command check happens first (allows version with invalid config)
2. Markdown renderers skipped when config invalid (prevents deep exit)
- Clarified that without the initErr guard, version would still fail despite bypass logic
**Diagnostic Command Rationale:**
- Added prominent statement: "Version is a diagnostic command. Users run it first when troubleshooting. Therefore, it must work even when everything else is broken."
- Expanded list of scenarios when users run version (debugging, upgrades, installation verification)
- Added explicit list of config failures version must tolerate (invalid YAML, aliases, schema, missing files, corrupted state)
- Emphasized that version is the foundation of diagnostic workflow
- Highlighted the bootstrapping problem: can't fix config without knowing version
The PRD now accurately documents the final implementation with the markdown initialization guard and provides comprehensive rationale for why version must be exceptionally resilient.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Fix pre-commit hook configuration issues
Fixed two pre-commit hook failures:
1. **golangci-lint config validation**:
- Removed invalid `exclude` properties from forbidigo pattern objects
- The forbidigo linter schema doesn't allow `exclude` as an additional property within pattern objects
- Moved exclusions to proper `issues.exclusions.rules` section
- Added exclusion rules for `viper.BindEnv` and `viper.BindPFlag` in `pkg/flags/` and test files
2. **YAML validation**:
- Added `tests/fixtures/` to check-yaml exclude pattern
- The `tests/fixtures/scenarios/invalid-config-yaml/atmos.yaml` file is intentionally invalid (test fixture)
- Pre-commit was incorrectly trying to validate this test fixture
The forbidigo configuration now follows the correct schema:
- Pattern objects only contain `pattern`, `msg`, and optionally `pkg` fields
- Exclusions are handled via `issues.exclusions.rules` with `path` and `text` matching
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* feat: Leverage atmos error infrastructure in version command
Enhanced error handling in version command to use the new error
builder infrastructure from PR #1763:
**Error Sentinels Added (errors/errors.go):**
- ErrVersionDisplayFailed - Failed to display version information
- ErrVersionCheckFailed - Failed to check for version updates
- ErrVersionFormatInvalid - Invalid version output format
- ErrVersionCacheLoadFailed - Failed to load version check cache
- ErrVersionGitHubAPIFailed - Failed to query GitHub API for releases
**Config Error Enrichment (cmd/root.go):**
- Version command config errors now include helpful hints
- Other commands' config errors provide actionable guidance
- Exit code 2 for configuration/usage errors
**Version Command Error Enrichment (internal/exec/version.go):**
- Invalid format errors include examples and hints
- GitHub API errors explain rate limiting and connectivity issues
- Cache loading errors provide helpful context
- All errors include structured debugging context
**Example File Added:**
- internal/exec/examples/version_format_example.md
- Shows correct usage for JSON and YAML formats
- Demonstrates piping to jq and version checking
**Benefits:**
- Improved user experience with actionable error messages
- Better debugging with structured context
- Consistent error handling across the codebase
- Maintains version command resilience (exits 0 for warnings)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Correct version format example to match Atmos conventions
Fixed the version format example file to follow the terse, concise format
used throughout Atmos cmd/markdown usage files.
**Changes:**
- Rewrote version_format_example.md to use dash-prefixed descriptions
- Removed verbose headers (###), sample outputs, and explanations
- Kept only essential usage patterns matching cmd/markdown/* format
- Location remains internal/exec/examples/ (correct for go:embed)
**Updated atmos-errors agent:**
- Documented correct format for usage examples in error messages
- Clarified difference between error examples and CLI help files
- Added guidelines based on existing cmd/markdown/ patterns
**Format now matches:**
```
- Brief description
\`\`\`
$ command example
\`\`\`
```
This aligns with all existing usage files in cmd/markdown/ while
keeping the file in the correct location for embedding from
internal/exec/.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Move version format example inline and follow Atmos conventions
Removed the `internal/exec/examples/` directory pattern and converted
the version format example to an inline constant, which is cleaner and
follows Go best practices.
**Changes:**
- Converted `version_format_example.md` to inline const in version.go
- Removed `_ "embed"` import (no longer needed)
- Changed from `WithExampleFile()` to `WithExample()`
- Deleted `internal/exec/examples/` directory
- Added reference copy to `cmd/version/markdown/` for consistency
**Rationale:**
- Go embed doesn't allow `../` paths across module boundaries
- `internal/exec/` cannot embed from `cmd/version/markdown/`
- Inline constants are simpler and don't create new directory patterns
- Reference file in `cmd/version/markdown/` maintains consistency with
other version subcommand usage files
This aligns with Atmos patterns while avoiding the creation of a new
`internal/exec/examples/` convention.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Enrich invalid log level errors with markdown formatting
This commit fixes a regression where invalid log level errors were
displayed as plain text instead of markdown-formatted errors with
proper sections (# Error, ## Explanation).
Changes:
- Updated pkg/logger/utils.go to return properly wrapped errors
- Enhanced cmd/root.go Execute() to enrich log level errors before
returning them, initializing markdown renderer when needed
- Modified pkg/config/utils.go checkConfig() to preserve error format
- Fixed internal/exec/version_test.go to use errors.Is() instead of
exact equality check for enriched errors
- Refactored error handling into handleConfigInitError() to reduce
complexity and improve maintainability
The fix ensures that when an invalid log level is detected during
config validation, the error is enriched with explanation text and
properly formatted as markdown for user-friendly output.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Update version test to use correct error sentinel and fix JSON regex
This commit fixes two test failures:
1. TestDisplayVersionInFormat/Invalid_format: Updated to expect
errUtils.ErrVersionFormatInvalid instead of ErrInvalidFormat, and
added errUtils import to version_test.go
2. TestCLICommands/version_--format_json_with_invalid_config: Fixed
regex pattern to handle multi-line JSON output by adding (?s) flag
to make . match newlines
These failures occurred because the version command returns enriched
errors with exit codes, and the JSON output is formatted with newlines
for readability.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* test: Fix version test failures and add error context testing helpers
Fixed two failing tests in version_additional_test.go:
1. TestVersionExec_Execute_PrintStyledTextError - Added missing mock
expectation for PrintMessage("") call that occurs before PrintStyledText
2. TestDisplayVersionInFormat_ErrorContexts - Updated to use
errors.GetAllSafeDetails to traverse error chain for context validation,
replacing simple string contains check that failed with error builder
Added new testing helpers in errors/testing.go:
- HasContext(err, key, value) - Check if error has specific context
- GetContext(err, key) - Retrieve context value by key
These helpers simplify error context assertions from 13 lines of
boilerplate to a single line, making tests cleaner and more maintainable.
Example usage:
assert.True(t, errUtils.HasContext(err, "format", "xml"))
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* refactor: Remove pointless error enrichment from debug log
Removed error enrichment that added user-facing hints to an error
that was only logged at DEBUG level and never shown to users.
The version command continues successfully regardless of config
errors, so enriching the error with hints like "The version command
works even with invalid configuration" is contradictory and wasteful.
Before:
- Built enriched error with hints and context
- Logged to DEBUG (never shown to user)
- Version command continued anyway
After:
- Log the original error to DEBUG
- No wasted enrichment for invisible logs
- Same behavior, clearer code
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* test: Delete tautological tests that don't test production code
Removed 7 useless tests (204 lines, 42% of the file) that were either:
1. Testing Go language features (struct initialization, field assignment)
2. Testing mock behavior instead of production logic
3. Testing trivial if statements (if disabled { return false })
Deleted tests:
- TestNewVersionExec - tests that constructor returns non-nil (duh)
- TestVersionStruct - tests that v.Field = "x" makes v.Field == "x" (wtf)
- TestGetLatestVersion_DisabledCheck - tests if !enabled { return false }
- TestIsCheckVersionEnabled_CacheLoadError - tests if err != nil { return false }
- TestIsCheckVersionEnabled_FrequencyCheck - just returns what the mock returns
- TestGetLatestVersion_EmptyReleaseTag - tests if str == "" { return false }
- TestExecute_WithFormatFlag - mock expectations that prove nothing
Kept tests that actually verify business logic:
- Error handling and enrichment
- Version string comparison with "v" prefix trimming
- Format validation and output generation
Before: 486 lines
After: 282 lines
Savings: 42% reduction in test bloat
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* fix: Fix false positive matching in HasContext error helper
Fixed critical bug where HasContext("format", "xml") would match
"format=xml2" or "myformat=xml" due to substring matching.
Before (broken):
expectedPair := key + "=" + value
if strings.Contains(detailStr, expectedPair) { return true }
After (correct):
parts := strings.SplitN(pair, "=", 2)
if parts[0] == key && parts[1] == value { return true }
Now properly parses "key1=value1 key2=value2" format by:
1. Splitting on space to get individual pairs
2. Splitting each pair on first '=' (handles values with '=')
3. Exact key and value equality checks
Added tests for false positive cases:
- Similar key prefix: "myformat=xml" doesn't match "format"
- Similar value prefix: "format=xml2" doesn't match "xml"
- Value contains search: "format=xml_extended" doesn't match "xml"
Uses same parsing logic as GetContext for consistency.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* docs: Fix atmos-errors agent to use succinct example format
Updated agent instructions to match the actual production format we use:
Before (verbose, with output):
- Showed full command + output examples
- Used markdown headings and sections
- Included JSON/YAML output displays
After (succinct, commands only):
- Bullet points with single commands
- NO output shown
- Just `$ command` in code blocks
- Matches version.go format exactly
Key changes:
- Examples are inline const strings (not separate files)
- No go:embed directives needed
- Format: `- Description\n\n` + "```" + `\n$ command\n` + "```"
- Clear rules: NO output sections, keep it succinct
This ensures the agent generates examples matching our production code.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
* refactor: Replace ugly string concatenation with go:embed
Fixed terrible advice in agent instructions and production code.
Before (horrible):
const example = - Example + backticks + command + backticks
After (clean):
//go:embed examples/version_format.md
var versionFormatExample string
Changes:
1. Created internal/exec/examples/version_format.md with clean markdown
2. Updated version.go to use go:embed instead of string hell
3. Fixed agent instructions to recommend go:embed for multi-line content
Why this matters:
- No more escaped backticks and string concatenation
- Proper syntax highlighting in editors
- Easy to edit markdown without Go string escaping
- Actually readable and maintainable
The agent now correctly instructs to use go:embed for examples.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
---------
Co-authored-by: Claude (via Conductor) <[email protected]>
Co-authored-by: Claude <[email protected]>
Co-authored-by: Andriy Knysh <[email protected]>1 parent d2cf477 commit f70da2f
File tree
24 files changed
+3884
-43
lines changed- .claude/agents
- cmd
- version/markdown
- docs/prd
- errors
- internal/exec
- examples
- pkg
- config
- logger
- tests
- fixtures/scenarios
- invalid-config-aliases
- invalid-config-schema
- invalid-config-yaml
- test-cases
24 files changed
+3884
-43
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
414 | 414 | | |
415 | 415 | | |
416 | 416 | | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
417 | 517 | | |
418 | 518 | | |
419 | 519 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
12 | 28 | | |
13 | 29 | | |
14 | 30 | | |
| |||
21 | 37 | | |
22 | 38 | | |
23 | 39 | | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
24 | 55 | | |
25 | 56 | | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
31 | 63 | | |
32 | 64 | | |
33 | 65 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
131 | 131 | | |
132 | 132 | | |
133 | 133 | | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
134 | 140 | | |
135 | 141 | | |
136 | 142 | | |
| |||
291 | 297 | | |
292 | 298 | | |
293 | 299 | | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
294 | 309 | | |
295 | 310 | | |
296 | 311 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
375 | 375 | | |
376 | 376 | | |
377 | 377 | | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
378 | 437 | | |
379 | 438 | | |
380 | 439 | | |
| |||
0 commit comments