fix(config): honor --config across internal reloads and multi-file merges - #2875
Open
Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Open
fix(config): honor --config across internal reloads and multi-file merges#2875Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Conversation
…ray merges
Internal call sites that re-invoke InitCliConfig(schema.ConfigAndStacksInfo{}, false)
mid-command no longer silently discard --config/--config-path/--base-path, fixing
`atmos --config <file> terraform plan/test` falling back to plain auto-discovery
(closes #2868). A second --config file with a conflicting array-typed value (e.g.
stacks.included_paths) no longer aborts stack discovery for entries that still
match, and `atmos config get` now reports the effective, fully-merged configuration
instead of a stale single-file value (closes #2867).
Also precomputes VendorDirAbsolutePath/WorkflowsDirAbsolutePath (same base_path
resolution fix as #2864) so vendor/workflow path joins don't re-derive a possibly
still-relative BasePath.
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
August 5, 2026 19:36
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
August 5, 2026 19:37 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
August 5, 2026 19:37 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Resource Changes Found for
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesConfiguration resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant LoadConfig
participant Profiles
participant ConfigGet
participant StackDiscovery
CLI->>LoadConfig: provide config files, paths, and profiles
LoadConfig->>Profiles: resolve contributor profile locations
Profiles-->>LoadConfig: merged effective configuration
LoadConfig->>ConfigGet: provide merged configuration
ConfigGet-->>CLI: return requested dot-notation value
LoadConfig->>StackDiscovery: discover included stack manifests
StackDiscovery-->>LoadConfig: return manifests or no-manifest error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
cmd/config/operations_test.go (1)
154-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing one test streams stub in this package.
The comment states this mirrors
configSchemaTestStreamsinschema_test.go. Both types are in packageconfigand implement the same five methods identically. Go allows one shared stub for both files.Move a single
configTestStreamsinto a shared test helper file in this package and delete both copies. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/config/operations_test.go` around lines 154 - 166, Replace the duplicate configGetTestStreams and configSchemaTestStreams implementations with one shared configTestStreams stub in a package-level test helper file. Update both test files to use configTestStreams and remove the redundant type and methods while preserving the existing stdio reader and buffer behavior.pkg/config/multifile_array_merge_test.go (1)
124-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTurn the stage 2 and stage 3 checkpoints into assertions, or drop them.
Stages 2 and 3 capture values and only log them. They verify nothing. The doc comment explains they existed to locate where the value diverged, and that investigation is now complete — the fix landed in
pkg/config/utils.go, not in the merge.Both stages have a known expected value now. Assert it, so a future regression in
mergeConfigFileormergeImportsfails the test instead of writing a line to the log.♻️ Assert the intermediate values
// Stage 2: after merging fragment.yaml on top (mergeConfigFile only, no mergeImports yet). require.NoError(t, mergeConfigFile(fragmentFile, v)) - afterMergeConfigFile := v.Get("stacks.included_paths") - t.Logf("stage 2 (after mergeConfigFile(fragment.yaml)): %#v", afterMergeConfigFile) + require.Equal(t, []interface{}{"deploy/**/*", "other/**/*"}, v.Get("stacks.included_paths"), + "stage 2: MergeConfig must fully replace the array, not union or revert it") // Stage 3: after mergeImports runs (a no-op for files with no `import:` key, per // processConfigImportsWithFSAndBasePathSource's early return -- confirming that, // or finding it ISN'T a no-op, is the point of this checkpoint). _, err := mergeImports(v, tmpDir, "", "") require.NoError(t, err) - afterMergeImports := v.Get("stacks.included_paths") - t.Logf("stage 3 (after mergeImports): %#v", afterMergeImports) + require.Equal(t, []interface{}{"deploy/**/*", "other/**/*"}, v.Get("stacks.included_paths"), + "stage 3: mergeImports is a no-op here and must not disturb the merged array") // Stage 4: after the final v.Unmarshal into the typed struct (what loadConfigFromCLIArgs // itself does last). var atmosConfig schema.AtmosConfiguration require.NoError(t, v.Unmarshal(&atmosConfig, atmosDecodeHook())) - t.Logf("stage 4 (after v.Unmarshal): %#v", atmosConfig.Stacks.IncludedPaths)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/multifile_array_merge_test.go` around lines 124 - 135, Update the stage 2 and stage 3 checkpoints in the test to assert their known expected values instead of only logging afterMergeConfigFile and afterMergeImports. Keep the existing mergeConfigFile and mergeImports calls, and retain the final behavior checks while making regressions in either intermediate stage fail the test.pkg/config/load_config_args.go (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider retiring
loadConfigFromCLIArgsonce its tests move to the real path.
LoadConfigno longer calls this function. Only tests do. The doc comment says so directly, which is honest, but it leaves a second, partial copy of the config tail in production code.This copy already diverges from
LoadConfig's tail. It skipspromoteAtmosEnvFromConfig, profile loading,restoreCaseSensitiveCommandEnvMaps, theprofiles.base_pathsync,bridgeVendorUpdaterConfig, and the container-runtime bridge. A test that passes here no longer proves the production path behaves the same way, and the gap will widen as the real tail grows.
pkg/config/multifile_array_merge_test.goalready exercises the production path throughInitCliConfigin three of its tests. If the two remainingloadConfigFromCLIArgscallers move toLoadConfigorInitCliConfig, this function can be deleted. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/load_config_args.go` around lines 61 - 69, Retire the unused production-only loadConfigFromCLIArgs path after migrating its remaining tests to exercise LoadConfig or InitCliConfig, preserving equivalent test coverage through the real configuration flow. Remove the function and any now-unused references or imports, while leaving mergeConfigFromCLIArgs and the existing production tail unchanged.pkg/config/utils.go (1)
163-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared glob-resolution block.
This block is now byte-identical to lines 57-78 in
FindAllStackConfigsInPathsForStack, including the hint text, the context keys, and the sentinel check. The comment manages that by pointing at the sibling, which works only as long as somebody remembers to update both.A small helper would remove the duplication:
// resolveStackGlobMatches returns the stack manifests matching p, or nil when p is // valid but currently matches nothing. func resolveStackGlobMatches(atmosConfig *schema.AtmosConfiguration, p string) ([]string, error) { patterns := []string{p} if filepath.Ext(p) == "" { patterns = getStackFilePatterns(p, true) } var allMatches []string for _, pattern := range patterns { matches, err := u.GetGlobMatches(pattern) if err == nil && len(matches) > 0 { allMatches = append(allMatches, matches...) } } if len(allMatches) > 0 { return allMatches, nil } // GetGlobMatches reports "pattern matched nothing" by wrapping ErrFailedToFindImport. // That is expected and must not abort discovery for the other entries // (cloudposse/atmos#2867). Only a different underlying error is genuine. if _, err := u.GetGlobMatches(patterns[0]); err != nil && !errors.Is(err, errUtils.ErrFailedToFindImport) { return nil, errUtils.Build(err). WithHintf("Verify `stacks.base_path` in `atmos.yaml` points to the correct directory"). WithHint("Check that the stacks directory exists and contains stack configuration files"). WithContext("pattern", patterns[0]). WithContext("stacks_base_path", atmosConfig.StacksBaseAbsolutePath). Err() } return nil, nil }Both callers then reduce to a call plus a
continuewhen the result is empty. Note theForStackvariant takesatmosConfigby value, so it would pass&atmosConfig.As per coding guidelines: "Before implementing functionality, search
internal/exec/andpkg/and extend existing code rather than duplicating it."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/utils.go` around lines 163 - 179, The glob-resolution and error-handling logic is duplicated between the current caller and FindAllStackConfigsInPathsForStack. Extract it into a shared resolveStackGlobMatches helper using the existing pattern expansion, GetGlobMatches, sentinel check, and contextual error construction, then update both callers to use it and continue when no matches are returned; pass the ForStack configuration by address as needed.Source: Coding guidelines
pkg/config/config_test.go (1)
542-561: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the effective path-resolution contract in the regression test.
The new case sets
BasePathtobaseDir, which is already absolute. It therefore cannot detect the failure mode where a consumer incorrectly re-joins a raw relative base path. Add table-driven cases that use the repository’sCliConfigPathconvention, assertfilepath.IsAbs, and verify both derived paths under the resolved base. Add explicit expectations for empty and absolute nested values, or reject unsupported absolute values in the implementation.As per coding guidelines and the PR objectives, new Go features require comprehensive, behavior-focused unit tests for the effective path-resolution case. Based on learnings, do not assume a path join treats an absolute second component as an override; test the contract or enforce relative values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config_test.go` around lines 542 - 561, Expand the table-driven regression coverage around AtmosConfigAbsolutePaths to use relative and absolute CliConfigPath-style base paths, asserting filepath.IsAbs and the expected VendorDirAbsolutePath and WorkflowsDirAbsolutePath under the resolved base. Include explicit cases for empty and absolute nested BasePath values, or update the implementation to reject unsupported absolute nested values and test that behavior.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/config/operations_test.go`:
- Around line 193-203: Add viper.Reset protection to the affected test in the
setup around configGetCmd.RunE: reset the global Viper before the test runs and
register t.Cleanup(viper.Reset) afterward, matching the pattern used by the
multifile array merge tests. Add the required viper import.
In `@internal/exec/validate_component.go`:
- Around line 38-43: Update getWorkflowsDirToUse in
internal/exec/validate_component.go and the corresponding Vendor.BasePath
fallback in internal/exec/vendor_utils.go to reuse the existing absolute-aware
base-path resolution helper instead of directly joining raw BasePath values.
Preserve the configured absolute override behavior while ensuring relative and
absolute child paths resolve consistently under the configured top-level base
path.
In `@pkg/config/config.go`:
- Around line 532-542: Update the filepath.Abs error handling in the vendor and
workflow path-resolution code to wrap failures with repository-standard static
errors and contextual %w messages identifying whether vendor or workflow
resolution failed. Preserve the existing early returns and assignments while
applying the wrapper consistently to both error branches.
In `@pkg/config/load.go`:
- Around line 407-412: The profile-loading flow must not treat the
semicolon-delimited result of connectPaths as one directory. Update
discoverProfileLocations or its caller to split CliConfigPath into individual
CLI config directories and search each contributor separately, preserving
single-path behavior; alternatively validate and reject multiple CLI config
directories before profile loading.
In `@pkg/config/multifile_array_merge_test.go`:
- Line 263: Update the test cleanup around ATMOS_PROFILE in the affected test to
preserve its original environment value: capture whether it was set and its
value before the test changes it, then restore that exact state with deferred
cleanup, including unsetting it only when it was originally absent. Replace the
direct os.Unsetenv cleanup while keeping the existing error handling.
- Around line 252-257: Update the opening identifier in the doc comment above
TestInitCliConfig_ProfileAppliedOnTopOfConfigFlag to match that exact test
function name, leaving the remainder of the comment unchanged.
---
Nitpick comments:
In `@cmd/config/operations_test.go`:
- Around line 154-166: Replace the duplicate configGetTestStreams and
configSchemaTestStreams implementations with one shared configTestStreams stub
in a package-level test helper file. Update both test files to use
configTestStreams and remove the redundant type and methods while preserving the
existing stdio reader and buffer behavior.
In `@pkg/config/config_test.go`:
- Around line 542-561: Expand the table-driven regression coverage around
AtmosConfigAbsolutePaths to use relative and absolute CliConfigPath-style base
paths, asserting filepath.IsAbs and the expected VendorDirAbsolutePath and
WorkflowsDirAbsolutePath under the resolved base. Include explicit cases for
empty and absolute nested BasePath values, or update the implementation to
reject unsupported absolute nested values and test that behavior.
In `@pkg/config/load_config_args.go`:
- Around line 61-69: Retire the unused production-only loadConfigFromCLIArgs
path after migrating its remaining tests to exercise LoadConfig or
InitCliConfig, preserving equivalent test coverage through the real
configuration flow. Remove the function and any now-unused references or
imports, while leaving mergeConfigFromCLIArgs and the existing production tail
unchanged.
In `@pkg/config/multifile_array_merge_test.go`:
- Around line 124-135: Update the stage 2 and stage 3 checkpoints in the test to
assert their known expected values instead of only logging afterMergeConfigFile
and afterMergeImports. Keep the existing mergeConfigFile and mergeImports calls,
and retain the final behavior checks while making regressions in either
intermediate stage fail the test.
In `@pkg/config/utils.go`:
- Around line 163-179: The glob-resolution and error-handling logic is
duplicated between the current caller and FindAllStackConfigsInPathsForStack.
Extract it into a shared resolveStackGlobMatches helper using the existing
pattern expansion, GetGlobMatches, sentinel check, and contextual error
construction, then update both callers to use it and continue when no matches
are returned; pass the ForStack configuration by address as needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dcd92e57-1fcf-4aa6-a1ea-396b818750c8
📒 Files selected for processing (28)
cmd/config/operations.gocmd/config/operations_test.gointernal/exec/describe_workflows_test.gointernal/exec/validate_component.gointernal/exec/vendor_utils.gointernal/exec/workflow.gointernal/exec/workflow_utils.gopkg/config/base_path_resolution_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/load.gopkg/config/load_config_args.gopkg/config/multifile_array_merge_test.gopkg/config/utils.gopkg/datafetcher/schema/atmos/config/1.0.jsonpkg/schema/schema.gopkg/vendoring/resolve.gotests/snapshots/TestCLICommands_atmos_--chdir_config_isolation.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.goldentests/snapshots/TestCLICommands_atmos_workflow_file_not_found.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_manifest.stderr.goldentests/snapshots/TestCLICommands_indentation.stdout.goldentests/snapshots/TestCLICommands_secrets-masking_describe_config.stdout.goldenwebsite/docs/cli/commands/config/config-get.mdxwebsite/docs/cli/configuration/profiles.mdx
…liConfigPath for profiles CI (linux/macos/windows) failed because getWorkflowsDirToUse/getVendorDirToUse made the "Vendoring from" log message and the invalid/missing workflow manifest error messages show a full, environment-length-dependent absolute path instead of the previous cwd-relative one. That broke word-wrapped golden snapshots and literal-pattern test assertions differently on every runner. Reuse the existing displayPath() helper (validate_schema.go) so these messages stay short and machine-independent again, while the underlying file resolution stays absolute and correct. Also addresses CodeRabbit findings on PR #2875: - discoverProfileLocations treated CliConfigPath's ";"-joined multi-directory form (from connectPaths, reachable now that --config flows into profile loading) as one directory, producing paths like "dirA;dirB;/.atmos/profiles" that could never exist. Split and search each contributor directory. - Wrap the new Vendor/Workflows filepath.Abs failures with the existing absPathOrError/ErrPathResolution helper instead of returning a raw error. - getWorkflowsDirToUse/getVendorDirToUse's fallback join now uses the absolute-aware u.JoinPath instead of filepath.Join. - Reset viper and restore ATMOS_PROFILE around two tests that mutated global state without cleanup; fixed a doc comment that named the wrong test function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
August 5, 2026 21:15 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (83.33%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2875 +/- ##
==========================================
+ Coverage 82.75% 82.77% +0.02%
==========================================
Files 1860 1861 +1
Lines 180240 180494 +254
==========================================
+ Hits 149150 149399 +249
+ Misses 23306 23303 -3
- Partials 7784 7792 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Maksym Vlasov (MaxymVlasov)
approved these changes
Aug 6, 2026
… precedence changes A field-test pass against this branch's config-loading changes (multi-file --config merging, profile precedence, config get/set) found 8 real, live-reproduced issues and fixes them all, each with a failing test committed first: - config set/delete/format silently edited only the FIRST --config file while config get reported the fully-merged value -- a false-success bug whenever a later file also set the same key. Now refuses ambiguous multi-file --config with a clear error (pkg/config/config_edit.go: new ErrAmbiguousConfigFile/ResolveConfigOverride, shared by cmd/config/operations.go and the duplicate in pkg/mcp/config/config.go). - ATMOS_CONFIG/ATMOS_CONFIG_PATH with multiple comma-separated values worked for some commands (via pkg/config's own os.Args/env fallback) but broke ~40 others reading these flags through pkg/flags' Viper-based ParseGlobalFlags, which splits env-sourced values on whitespace, not commas. Fixed once at that shared choke point by exporting the existing --profile fix (parseViperProfilesFromEnv -> cfg.FixViperEnvStringSliceQuirk) and applying it there too. - profiles.base_path declared in a non-first --config file resolved against the FIRST file's directory regardless of which file actually declared it, silently failing to find profiles that exist. Added per-file directory tracking (mirroring the existing base_path tracking in mergeFiles) threaded through to discoverProfileLocations. - Vendor/workflow error messages (ErrEmptySources, ErrMissingVendorConfigDefinition, ErrDuplicateComponents, ErrComponentNotDefined, ErrNoComponentsWithTags, and others) still leaked absolute paths right next to the "Vendoring from" line already fixed in the prior commit -- a half-fixed pattern. Wrapped 13 sites in displayPath(), plus fixed a copy/paste bug in one workflow directory-read error that showed the raw unresolved config value instead of the path actually searched. - displayPath() itself was silently defeated whenever the working directory was reached through a symlink (e.g. macOS's /tmp), because os.Getwd() preserves the logical $PWD path while git-root-discovery-resolved config paths are physical. Fixed with a two-attempt comparison (raw first, then both sides resolved via the directory, since the target file often doesn't exist yet). - --config-path always wins over --config regardless of CLI argument order (undocumented, now documented, not code-changed -- effort didn't justify a fix for this ordering nuance). - Documented the previously-undocumented ATMOS_CONFIG/ATMOS_CONFIG_PATH env vars and fixed --config's flag-type description on config-set/delete/format.mdx (all three incorrectly said "string" instead of "string slice"). Also updates the field-test skill to default to testing the current branch's diff against its base branch when no explicit target is given, instead of asking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
deployed
to
screengrabs
August 6, 2026 14:55 — with
GitHub Actions
Active
GitHub Actions
Active
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/load_config_args.go (1)
51-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack
profiles.base_pathprovenance for--config-path.
profilesBasePathConfigDironly comes from--configfiles. A later--config-pathconfiguration can overrideprofiles.base_path, but profile discovery still resolves the final value against an earlier--configdirectory.Return declaration provenance from
mergeConfigFromDirectories. Apply the directory from the last merged configuration that declaresprofiles.base_path. Add coverage for mixed--configand--config-pathinput.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/load_config_args.go` around lines 51 - 58, Update the config-loading flow around mergeConfigFromDirectories and profilesBasePathConfigDir to return and apply the directory provenance of profiles.base_path from the last merged --config-path configuration that declares it, overriding earlier --config provenance. Extend coverage to verify mixed --config and --config-path inputs resolve profiles from the later declaration.pkg/config/load.go (1)
398-407: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFill fallback selection fields independently.
The fallback runs only when all three fields are empty. If an internal caller supplies
AtmosBasePathbut omitsAtmosConfigFilesFromArg,--configandATMOS_CONFIGare ignored. The same problem applies to other partial selections.Read the fallback once. Fill each empty field without replacing a caller-provided field. Add regression cases for each partial-selection combination.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/load.go` around lines 398 - 407, The fallback logic around getConfigSelectionFromFlagsOrEnv must run when any selection field is missing, not only when all three are empty. Read the fallback once, then independently populate each empty AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, and AtmosBasePath field while preserving caller-provided values; add regression coverage for every partial-selection combination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/field-test/SKILL.md:
- Around line 3-4: Update the field-test targeting algorithm in the skill
instructions to resolve the actual pull-request base when available, rather than
relying on the upstream tracking branch. Compare that base against the complete
worktree, including staged, unstaged, and untracked changes, and inspect git
status before prompting for a target when no explicit target is provided.
Preserve the existing explicit-target behavior and investigation-only scope.
- Around line 45-50: Update the implementation-inspection guidance in the
field-test skill to require inspecting every changed production package
identified by the diff, including internal/exec/ when present, rather than
treating it only as phased-out context. Explicitly retain the requirement to
inspect both internal/exec/ and pkg/ when determining implementation scope or
extending functionality, while also checking thin cmd/<command>/ call sites and
relevant error paths.
In `@internal/exec/validate_schema.go`:
- Around line 372-375: Update relPath to reject only paths whose first relative
component is the parent-directory segment "..", while preserving relative names
such as "..vendor.yaml" and other in-directory files. Add a regression test
covering an in-directory filename beginning with ".." and verify displayPath
keeps it relative.
In `@internal/exec/vendor_utils_test.go`:
- Around line 1376-1380: Update the “ErrEmptySources” test case in the vendor
execution tests so its atmosVendorSpec includes a valid, non-empty Imports
configuration that resolves to zero sources, avoiding the missing-definition
guard in ExecuteAtmosVendorInternal. Assert that the returned error matches
ErrEmptySources via ErrorIs, while preserving the existing empty-sources setup
and test intent.
In `@pkg/config/load_profile_test.go`:
- Line 211: Update the comment for TestParseViperProfilesFromEnv_Quirks to end
with a period, without changing its wording or surrounding test code.
In `@website/docs/cli/configuration/configuration.mdx`:
- Around line 32-33: Update the configuration source description to explicitly
document precedence: command-line flags such as --config and --config-path
override their environment variable equivalents, followed by config files and
then defaults.
---
Outside diff comments:
In `@pkg/config/load_config_args.go`:
- Around line 51-58: Update the config-loading flow around
mergeConfigFromDirectories and profilesBasePathConfigDir to return and apply the
directory provenance of profiles.base_path from the last merged --config-path
configuration that declares it, overriding earlier --config provenance. Extend
coverage to verify mixed --config and --config-path inputs resolve profiles from
the later declaration.
In `@pkg/config/load.go`:
- Around line 398-407: The fallback logic around
getConfigSelectionFromFlagsOrEnv must run when any selection field is missing,
not only when all three are empty. Read the fallback once, then independently
populate each empty AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, and
AtmosBasePath field while preserving caller-provided values; add regression
coverage for every partial-selection combination.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63dd978a-6688-4746-9e8d-ea2299308980
📒 Files selected for processing (27)
.claude/skills/field-test/SKILL.mdcmd/config/operations.gocmd/config/operations_test.gointernal/exec/describe_workflows_test.gointernal/exec/validate_schema.gointernal/exec/validate_schema_test.gointernal/exec/vendor_utils.gointernal/exec/vendor_utils_test.gointernal/exec/workflow_utils.gopkg/config/config_edit.gopkg/config/import_base_path_test.gopkg/config/load.gopkg/config/load_config_args.gopkg/config/load_error_paths_unix_test.gopkg/config/load_profile_test.gopkg/config/load_test.gopkg/config/multifile_array_merge_test.gopkg/config/profiles.gopkg/flags/global_registry.gopkg/flags/global_registry_test.gopkg/mcp/config/config.gopkg/mcp/config/config_test.gopkg/schema/schema.gowebsite/docs/cli/commands/config/config-delete.mdxwebsite/docs/cli/commands/config/config-format.mdxwebsite/docs/cli/commands/config/config-set.mdxwebsite/docs/cli/configuration/configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/exec/workflow_utils.go
- pkg/config/profiles.go
- pkg/schema/schema.go
- cmd/config/operations.go
- internal/exec/vendor_utils.go
Comment on lines
+1376
to
+1380
| name: "ErrEmptySources", | ||
| opts: &executeVendorOptions{ | ||
| vendorConfigFileName: vendorConfigFileName, | ||
| atmosConfig: atmosConfig, | ||
| atmosVendorSpec: schema.AtmosVendorSpec{Imports: []string{}, Sources: []schema.AtmosVendorSource{}}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise ErrEmptySources instead of the missing-definition guard.
ExecuteAtmosVendorInternal returns ErrMissingVendorConfigDefinition before import processing when both Sources and Imports are empty. This row duplicates the first row and never covers ErrEmptySources.
Provide an accepted non-empty import configuration that resolves to zero sources. Assert ErrorIs(err, ErrEmptySources).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/vendor_utils_test.go` around lines 1376 - 1380, Update the
“ErrEmptySources” test case in the vendor execution tests so its atmosVendorSpec
includes a valid, non-empty Imports configuration that resolves to zero sources,
avoiding the missing-definition guard in ExecuteAtmosVendorInternal. Assert that
the returned error matches ErrEmptySources via ErrorIs, while preserving the
existing empty-sources setup and test intent.
| } | ||
|
|
||
| // TestParseViperProfilesFromEnv_Quirks tests the parseViperProfilesFromEnv function | ||
| // TestParseViperProfilesFromEnv_Quirks tests the FixViperEnvStringSliceQuirk function |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a period to the comment.
The comment does not end with a period. This can fail the configured Go comment lint rule.
As per coding guidelines, “All comments must end with periods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/load_profile_test.go` at line 211, Update the comment for
TestParseViperProfilesFromEnv_Quirks to end with a period, without changing its
wording or surrounding test code.
Source: Coding guidelines
CodeRabbit findings on the previous commit, verified and fixed: - displayPath()'s relPath() treated any path whose relative form merely started with the substring ".." as escaping cwd, so an in-directory file literally named e.g. "..vendor.yaml" would incorrectly leak its absolute path. Now checks for ".." as a complete path segment, with a regression test. - field-test skill: fixed the branch-targeting algorithm to resolve the actual PR base (gh pr view --json baseRefName) instead of the upstream tracking branch, which for a pushed feature branch produces an empty diff against itself; also now inspects staged/unstaged/untracked changes, not just committed history. Fixed the Phase 1 guidance that could be read as "skip internal/exec/" when the branch's diff actually touches it. - configuration.mdx: split the flags/env-var precedence entries and stated explicitly that a flag always wins over its env var equivalent. - vendor_utils_test.go: the "ErrEmptySources" test case actually exercised ErrMissingVendorConfigDefinition (same empty Sources+Imports triggers the earlier guard first) -- verified ErrEmptySources is structurally unreachable via ExecuteAtmosVendorInternal's public path given processVendorImports' per-level non-empty-content invariant, documented why, and removed the duplicate/misleading case rather than leave it mislabeled. - The load_profile_test.go missing-period finding was already resolved (the comment reads as one grammatically complete, period-terminated sentence spanning two lines) and the patch-scoped lint gate already passes at 0 issues, so left as-is. Also closes the Codecov patch-coverage gate (83.33% -> ~94%, threshold 85%) with real tests for the newly-added code, not coverage theater: - cmd/config/operations.go: config get's InitCliConfig failure path (a malformed --config file). - pkg/config/config_edit.go: ResolveConfigOverride's three branches directly. - pkg/config/load.go / load_config_args.go: declaresProfilesBasePath's branches (malformed YAML, non-mapping profiles, mapping without base_path) via a direct table test. - pkg/config/profiles.go: splitCliConfigPath's separators-only-no-content edge. - pkg/config/utils.go: a genuine (non-ErrFailedToFindImport) glob syntax error in FindAllStackConfigsInPaths[ForStack], previously untested because the only existing test used the "matched nothing" tolerated case. - internal/exec/vendor_utils.go: getVendorDirToUse/resolveVendorConfigFilePath directly, plus ErrDuplicateImport in processVendorImports. Remaining gaps were investigated and left deliberately uncovered, each for a stated reason rather than silently: two filepath.Abs error branches in AtmosConfigAbsolutePaths (config.go) are unreachable because atmosBasePathAbs is already guaranteed absolute by that point in the function (same as six untested sibling branches above them predating this PR); a handful of load.go/load_config_args.go lines are pre-existing code that only appear as "added" because wrapping the old flow in a new if/else shifted their indentation; and two permission-denied-style branches (vendor_utils.go, workflow_utils.go) are the same class of cross-platform-fragile os.Chmod scenario this repo already accepts as untested elsewhere (TestReadWorkDirConfig_GetwdError). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/exec/vendor_utils_test.go (1)
1211-1249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven cases for the path-resolution scenarios.
Both test functions define multiple scenarios with repeated subtest setup and assertions. Use a test-case slice and a loop for each function.
As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exec/vendor_utils_test.go` around lines 1211 - 1249, Convert TestGetVendorDirToUse and TestResolveVendorConfigFilePath_CheckGlobalConfig to table-driven tests using case slices and subtest loops. Keep each existing scenario’s AtmosConfiguration setup, invocation, and expected path unchanged while moving the repeated assertions into the loop.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/field-test/SKILL.md:
- Around line 23-25: Update the field-test change-inspection instructions to
include the full committed diff, not only its stat summary, and explicitly read
the contents of every untracked path returned by git ls-files --others
--exclude-standard. Preserve inspection of staged and unstaged changes via git
diff HEAD so target inference covers all changed implementation files.
In `@cmd/config/operations_test.go`:
- Around line 243-257: Update TestConfigGetCommand_InitCliConfigError to create
and use cmd.NewTestKit(t) before invoking configGetCmd.RunE, ensuring the Cobra
command test isolates and cleans shared root-command state while preserving the
existing malformed-config assertions.
In `@internal/exec/vendor_utils_test.go`:
- Around line 1212-1218: Update the vendor path tests around getVendorDirToUse
to use OS-native paths: create temporary roots with t.TempDir(), construct paths
via filepath.Join, and derive expected values from those constructed paths.
Ensure the absolute Vendor.BasePath case exercises filepath.IsAbs correctly on
every platform, including Windows.
---
Nitpick comments:
In `@internal/exec/vendor_utils_test.go`:
- Around line 1211-1249: Convert TestGetVendorDirToUse and
TestResolveVendorConfigFilePath_CheckGlobalConfig to table-driven tests using
case slices and subtest loops. Keep each existing scenario’s AtmosConfiguration
setup, invocation, and expected path unchanged while moving the repeated
assertions into the loop.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c80a02a-96b0-4ee4-a482-6a3fd1d373bf
📒 Files selected for processing (10)
.claude/skills/field-test/SKILL.mdcmd/config/operations_test.gointernal/exec/validate_schema.gointernal/exec/validate_schema_test.gointernal/exec/vendor_utils_test.gopkg/config/base_path_resolution_test.gopkg/config/config_edit_test.gopkg/config/multifile_array_merge_test.gopkg/config/profiles_test.gowebsite/docs/cli/configuration/configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/exec/validate_schema.go
- pkg/config/profiles_test.go
- internal/exec/validate_schema_test.go
Comment on lines
+243
to
+257
| func TestConfigGetCommand_InitCliConfigError(t *testing.T) { | ||
| dir := t.TempDir() | ||
| badFile := filepath.Join(dir, "bad.yaml") | ||
| require.NoError(t, os.WriteFile(badFile, []byte("settings:\n enabled: [true\n"), 0o644)) // unterminated flow sequence | ||
|
|
||
| viper.Reset() | ||
| t.Cleanup(viper.Reset) | ||
|
|
||
| origArgs := os.Args | ||
| t.Cleanup(func() { os.Args = origArgs }) | ||
| os.Args = []string{"atmos", "--config", badFile, "config", "get", "settings.enabled"} | ||
|
|
||
| err := configGetCmd.RunE(configGetCmd, []string{"settings.enabled"}) | ||
| require.Error(t, err) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Isolate command state with cmd.NewTestKit(t).
Create cmd.NewTestKit(t) before invoking configGetCmd.RunE. This test executes a Cobra command and can otherwise retain shared command state across tests.
As per coding guidelines: “Always use cmd.NewTestKit(t) in command tests to isolate and clean root-command state.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/config/operations_test.go` around lines 243 - 257, Update
TestConfigGetCommand_InitCliConfigError to create and use cmd.NewTestKit(t)
before invoking configGetCmd.RunE, ensuring the Cobra command test isolates and
cleans shared root-command state while preserving the existing malformed-config
assertions.
Source: Coding guidelines
Verified against current code, two real findings fixed and one skipped as mechanically inapplicable: - field-test skill: the branch-diff inspection was still incomplete -- `--stat` alone never shows diff content, and untracked files never appear in `git diff HEAD` or a `--stat` summary at all, only their bare paths via `git status --porcelain`. Now reads the full `git diff <base>...HEAD` content and explicitly enumerates + reads untracked files via `git ls-files --others --exclude-standard`, so a brand-new implementation file can't go completely unread. - vendor_utils_test.go: TestResolveVendorConfigFilePath_CheckGlobalConfig's "absolute Vendor.BasePath" subtest used a hardcoded "/abs/vendor" string literal, which filepath.IsAbs only treats as absolute on POSIX -- on Windows it would take the wrong branch (no drive letter/UNC prefix) and fail. Switched to an OS-native absolute path built from t.TempDir(). - Skipped: "create cmd.NewTestKit(t) before invoking configGetCmd.RunE" in cmd/config/operations_test.go. Verified this is not applicable: NewTestKit is declared in cmd/testkit_test.go (package cmd, a _test.go file, so not importable cross-package at all), and cmd/config could not import package cmd regardless -- cmd/root.go already imports cmd/config, so the reverse import would be a circular dependency and fail to build. The test already follows this same file's established isolation pattern (viper.Reset() + os.Args save/restore via t.Cleanup) for the state it actually depends on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/field-test/SKILL.md:
- Around line 23-29: Update the base-resolution flow in the field-test
instructions before invoking git diff <base>...HEAD: validate that the selected
PR base or main fallback resolves to an existing Git ref, preferring
origin/<base>, <base>, or main as appropriate. Distinguish an unfetched PR
branch from lookup failure, and stop requesting user input when no usable base
ref exists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc30a384-22d2-4555-992f-6289609ee9c1
📒 Files selected for processing (2)
.claude/skills/field-test/SKILL.mdinternal/exec/vendor_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_utils_test.go
CodeRabbit review on PR #2875: the branch-name resolved via `gh pr view --json baseRefName` / `gh repo view --json defaultBranchRef` was never checked against what's actually available in the current checkout before being handed to `git diff <base>...HEAD`. A shallow clone, detached HEAD, or worktree with a narrow fetch refspec can know a branch's NAME without having its commits, so the skill could silently attempt (and fail) a diff against an unresolvable ref instead of falling back or asking. Added an explicit verification step: try `origin/<resolved-name>`, `<resolved-name>`, `origin/main`, `main` in order via `git rev-parse --verify --quiet <candidate>^{commit}`, take the first that resolves, and stop to ask the user only if none do -- rather than running the diff against an unverified ref and surfacing a raw git error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
internal/exec,pkg/vendoring,cmd/, etc. callingInitCliConfig(schema.ConfigAndStacksInfo{}, false)) now fall back to parsing--config/--config-path/--base-pathfromos.Args/env instead of silently discarding the selection made at startup.--configfile that sets a conflicting value for an array-typed key (e.g.stacks.included_paths) no longer aborts stack discovery for entries that still legitimately match; a real "nothing matched at all" case now returns a distinct error instead.atmos config getnow reports the effective, fully-merged configuration for the invocation (all--configfiles,--config-pathdirs, and profiles applied) instead of reading a single physical file.VendorDirAbsolutePath/WorkflowsDirAbsolutePathare now precomputed once (mirroring the existing top-levelbase_pathresolution), so vendor/workflow path joins no longer re-derive a possibly still-relativeBasePath.why
atmos --config <file> terraform plan/testwas failing withfailed to find importeven thoughatmos --config <file> list stacksworked with the identical flag, because a downstreamInitCliConfigre-invocation lost the--configselection mid-command.--configfiles with a conflicting array value madestacks.included_pathsunusable for stack discovery, whileatmos config getmisleadingly reported the config as unchanged.references
Closes #2867
Closes #2868
Summary by CodeRabbit
New Features
atmos config getnow reads the fully merged configuration, including multiple config files, paths, and profiles.--configfiles are supported, with later files taking precedence.Bug Fixes
Documentation