fix: unified base path resolution semantics#2227
fix: unified base path resolution semantics#2227aknysh merged 3 commits intoaknysh/fix-import-not-foundfrom
Conversation
…ndent semantics
Establish a unified convention where:
- Empty ("") = smart defaults (git root search)
- Dot (".", "./foo", "..", "../foo") = explicit anchor, context-dependent
- In atmos.yaml: anchor to config directory (config-file convention)
- In env var/CLI/provider: anchor to CWD (shell convention)
- Bare ("foo", "stacks") = search path, source-independent (always git root search)
- Absolute ("/path") = pass through unchanged
This eliminates the incongruence where "" and "." were treated identically or where
the same bare path value behaved differently based on source. The convention is now
unambiguous: empty is absence of opinion, dot is contextual anchor, bare is a search.
## Changes
**Core implementation:**
- Add `BasePathSource` field to `AtmosConfiguration` to track source (runtime vs config)
- Make `resolveAbsolutePath()` source-aware: dot-prefix resolves to CWD for runtime, config dir for config
- Mark source as "runtime" when base_path comes from env var, CLI flag, or provider parameter
- Remove `resolveSimpleRelativeBasePath()` — no longer needed with source-aware resolution
- Extract `resolveDotPrefixPath()` helper to reduce cyclomatic complexity
- Bare paths continue through git root search regardless of source
**Testing:**
- Add comprehensive tests proving dot-prefix is source-dependent, bare paths are not
- Update existing tests to reflect new convention (ATMOS_BASE_PATH=. now goes to CWD)
- All tests pass; no regressions
**Documentation:**
- Rewrite PRD with 4-category classification (Empty/Dot/Bare/Absolute)
- Add 18-row comprehensive examples table showing all scenarios
- Include source-awareness explanation and consistency proof
- Update fix documentation to reference source-aware resolution
Fixes #2183 (Tyler Rankin's ATMOS_BASE_PATH=.terraform/modules/monorepo scenario).
Fixes #1858 (empty base_path with ATMOS_CLI_CONFIG_PATH).
Stacked onto PR #2215 (aknysh/fix-import-not-found).
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR addresses "failed to find import" errors by implementing source-aware base path resolution. It introduces a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Update CLI test cases to use correct relative paths (e.g., "../..") instead of "." for ATMOS_BASE_PATH env var, since env vars are now "runtime" source and "." resolves to CWD (shell convention) not config dir. Add unit tests for source-aware resolution: dot-prefix fallback, absolute pass-through, bare path without git root, and BasePathSource tracking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## aknysh/fix-import-not-found #2227 +/- ##
===============================================================
- Coverage 76.88% 76.88% -0.01%
===============================================================
Files 1001 1001
Lines 95412 95411 -1
===============================================================
- Hits 73354 73353 -1
+ Misses 17789 17788 -1
- Partials 4269 4270 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Use t.TempDir() instead of constructing path from filepath.Separator, which lacks a drive letter on Windows and isn't recognized as absolute. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lative paths (#2215) * fix: resolve explicit base paths relative to CWD, not git root When ATMOS_BASE_PATH env var, --base-path flag, or atmos_base_path provider parameter provides a simple relative path (e.g., ".terraform/modules/monorepo"), resolve it relative to CWD instead of routing through git root discovery. This restores pre-v1.202.0 behavior for explicitly set paths while preserving git root discovery for default/empty base paths. Two-pronged fix: 1. configAndStacksInfo.AtmosBasePath (provider/CLI): convert to absolute CWD-relative immediately in InitCliConfig 2. ATMOS_BASE_PATH env var (via Viper): tryResolveWithGitRoot now validates the git-root-joined path exists before using it, and falls back to CWD-relative resolution if it doesn't Also improves error messages for "failed to find import" using the error builder pattern with actionable hints and context. Fixes #2183 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add git root discovery compatibility tests and update fix doc Verify that the os.Stat fallback in tryResolveWithGitRoot doesn't break "run Atmos from any subdirectory" behavior. Add table-driven tests for resolveSimpleRelativeBasePath helper. Document git root discovery compatibility analysis with integration test evidence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback and CI test failure - Distinguish os.Stat ENOENT from permission/I/O errors in tryResolveWithGitRoot using os.IsNotExist() checks - Use error builder pattern (ErrStatFile, ErrPathResolution) for non-ENOENT failures instead of silently falling back - Remove WithExitCode(2) from error builders — restores default exit code 1, fixing helmfile apply non-existent CI test - Replace hardcoded Unix path in test with filepath.Abs() - Rewrite TestTryResolveWithGitRoot_ExistingPathAtGitRoot to actually call resolveAbsolutePath instead of just checking os.Stat preconditions - Add TestTryResolveWithGitRoot_CWDFallback covering the core fix path - Add TestTryResolveWithGitRoot_NeitherExists for fallback to git root Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: reconcile base path resolution PRD with source-dependent semantics Add FR6 (Runtime Source Resolution) formalizing that --base-path, ATMOS_BASE_PATH env var, and atmos_base_path provider parameter resolve simple relative paths relative to CWD, not git root. Document the design rationale for source-dependent resolution, add runtime test cases, implementation details, and Issue #2183 resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: fix CWD fallback test to run inside git repo for proper coverage The CWDFallback test was creating a temp dir outside the git repo, causing getGitRootOrEmpty() to return "" and skip the os.Stat fallback logic entirely. Now creates the test dir inside the repo so git root discovery works, increasing tryResolveWithGitRoot coverage from 76% to 84%. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update Go dependencies to latest compatible versions Updated: anthropic-sdk-go v1.27.0, googleapis/gax-go v2.19.0, google.golang.org/api v0.272.0, modernc.org/sqlite v1.47.0, google.golang.org/genproto 20260316, charmbracelet/x/exp/slice 20260316. Pinned: gocloud.dev v0.41.0 and go-fsimpl v0.3.1 (gomplate/v3 incompatible with newer versions due to s3blob.URLOpener changes). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [autofix.ci] apply automated fixes * fix: unified base path resolution semantics (#2227) * docs(prd): formalize base path resolution convention with source-dependent semantics Establish a unified convention where: - Empty ("") = smart defaults (git root search) - Dot (".", "./foo", "..", "../foo") = explicit anchor, context-dependent - In atmos.yaml: anchor to config directory (config-file convention) - In env var/CLI/provider: anchor to CWD (shell convention) - Bare ("foo", "stacks") = search path, source-independent (always git root search) - Absolute ("/path") = pass through unchanged This eliminates the incongruence where "" and "." were treated identically or where the same bare path value behaved differently based on source. The convention is now unambiguous: empty is absence of opinion, dot is contextual anchor, bare is a search. ## Changes **Core implementation:** - Add `BasePathSource` field to `AtmosConfiguration` to track source (runtime vs config) - Make `resolveAbsolutePath()` source-aware: dot-prefix resolves to CWD for runtime, config dir for config - Mark source as "runtime" when base_path comes from env var, CLI flag, or provider parameter - Remove `resolveSimpleRelativeBasePath()` — no longer needed with source-aware resolution - Extract `resolveDotPrefixPath()` helper to reduce cyclomatic complexity - Bare paths continue through git root search regardless of source **Testing:** - Add comprehensive tests proving dot-prefix is source-dependent, bare paths are not - Update existing tests to reflect new convention (ATMOS_BASE_PATH=. now goes to CWD) - All tests pass; no regressions **Documentation:** - Rewrite PRD with 4-category classification (Empty/Dot/Bare/Absolute) - Add 18-row comprehensive examples table showing all scenarios - Include source-awareness explanation and consistency proof - Update fix documentation to reference source-aware resolution Fixes #2183 (Tyler Rankin's ATMOS_BASE_PATH=.terraform/modules/monorepo scenario). Fixes #1858 (empty base_path with ATMOS_CLI_CONFIG_PATH). Stacked onto PR #2215 (aknysh/fix-import-not-found). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: update tests for source-aware base path resolution Update CLI test cases to use correct relative paths (e.g., "../..") instead of "." for ATMOS_BASE_PATH env var, since env vars are now "runtime" source and "." resolves to CWD (shell convention) not config dir. Add unit tests for source-aware resolution: dot-prefix fallback, absolute pass-through, bare path without git root, and BasePathSource tracking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use real absolute path in test for Windows compatibility Use t.TempDir() instead of constructing path from filepath.Separator, which lacks a drive letter on Windows and isn't recognized as absolute. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com> Co-authored-by: aknysh <andriy.knysh@gmail.com> * fix: use error builder pattern for all path resolution errors Replace raw fmt.Errorf calls in resolveDotPrefixPath, tryResolveWithGitRoot, and tryResolveWithConfigPath with errUtils.Build(errUtils.ErrPathResolution) for consistent error classification via errors.Is(). Update fix doc to reflect source-aware resolution with BasePathSource tracking instead of the old pre-normalize approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate path resolution error handling to improve coverage Extract absPathOrError() helper to consolidate 7 identical filepath.Abs error builder patterns into one function. Remove dead isExplicitRelative parameter and code block from tryResolveWithGitRoot (unreachable since dot-prefixed paths are routed to resolveDotPrefixPath before reaching it). resolveDotPrefixPath: 80% → 100%, tryResolveWithConfigPath: 80% → 100%. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add simple pseudocode and quick-reference to base path PRD Restore the simple if/else pseudocode block (per review feedback) and add a comprehensive quick-reference showing resolution examples for config-file source, runtime source, and no-git-repo scenarios. Both blocks validated against the implementation in pkg/config/config.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
What
Establishes a unified convention for base path resolution where the meaning of a value is determined by its form and the context is determined by its source:
"") → smart defaults (git root search)".","./foo","..","../foo") → explicit anchor to "here" (context-dependent: config dir in atmos.yaml, CWD in env vars/CLI/provider)"foo","stacks") → search path (git root search, source-independent)"/path") → pass through unchangedThis eliminates source-dependent behavior for bare paths:
ATMOS_BASE_PATH=stacksnow goes through the same git root search asbase_path: stacksin atmos.yaml.Why
The previous PRD had inverted semantics for runtime sources (FR6), causing confusion about whether env vars should be CWD-relative or config-relative. The new convention is logically consistent:
.works in every shell tool)This fixes issues #2183 and #1858 while preserving the "run from anywhere" behavior that users expect.
Testing
References
Stacked on #2215 (aknysh/fix-import-not-found).
Fixes #2183 (Spacelift scenario).
Fixes #1858 (ATMOS_CLI_CONFIG_PATH with empty base_path).
Summary by CodeRabbit
Bug Fixes
New Features