Repository navigation
[codex] Fix remote Git stack imports - #2528
Merged
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
May 26, 2026 20:56
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 26, 2026 20:56
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:
📝 WalkthroughWalkthroughUnifies local and remote import handling via RemoteImportMatch/Resolve (Git subdir discovery, per-file caching), adds ResolveNested for nested_imports: remote with base-path inference, integrates matches into the stack processor, validates nested_imports, and adds tests, CLI fixtures, and docs. ChangesRemote Import Resolution and Integration
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant RemoteImporter as RemoteImporter.Resolve
participant MatchCache as matchCache
participant GitDownloader as Git downloader
participant GlobResolver as resolveStackFiles
participant FileCache as memCache/FileCache
Caller->>RemoteImporter: Resolve(uri)
RemoteImporter->>MatchCache: check cached matches
alt cache hit
MatchCache-->>RemoteImporter: cached []RemoteImportMatch
RemoteImporter-->>Caller: return matches
else cache miss
RemoteImporter->>GitDownloader: download/checkout git source (if Git)
GitDownloader-->>RemoteImporter: local source dir
RemoteImporter->>GlobResolver: resolveStackFiles(subdir)
GlobResolver-->>RemoteImporter: []file paths
RemoteImporter->>FileCache: cacheFile(key, content) per file
RemoteImporter->>MatchCache: store cloned matches
RemoteImporter-->>Caller: return matches
end
sequenceDiagram
participant Processor as ProcessImportSection
participant Remote as ResolveRemoteImport
participant LocalGlob as u.GetGlobMatches
participant Matches as importMatches[] (RemoteImportMatch)
Processor->>Remote: remote import URI
Remote-->>Processor: []RemoteImportMatch
Processor->>LocalGlob: local glob pattern
LocalGlob-->>Processor: []string paths
Processor->>Matches: convert local paths -> RemoteImportMatch (Path)
Processor->>Matches: unified loop iterates RemoteImportMatch (match.Path / match.Key)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (1)
tests/cli_remote_imports_test.go (1)
41-47: 💤 Low valueConsider adding a git availability check.
The test uses
exec.Command("git", ...)which assumes git is installed. While git is typically available in development environments, consider adding a skip check at the test start:if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") }This would gracefully handle environments without git rather than failing hard.
🤖 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 `@tests/cli_remote_imports_test.go` around lines 41 - 47, Add a git availability check to avoid hard failures when git is missing: in the test entry (or at the top of the helper runRemoteImportsGit), call exec.LookPath("git") and if it returns an error call t.Skip("git not available") so tests are skipped instead of failing; update runRemoteImportsGit (or the test that calls it) to perform this check before running exec.Command("git", ...) and then proceed as before if git is found.
🤖 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 `@pkg/stack/imports/remote_test.go`:
- Around line 66-68: gitFileURI currently builds a file:// URL with raw OS path
which makes Windows backslashes get encoded; update gitFileURI to first convert
the path to an absolute, slash-normalized form (use filepath.Abs +
filepath.ToSlash) and, for Windows drive-letter paths (e.g., "C:/..."), ensure
the URL Path begins with a leading "/" (so it becomes "/C:/...") before
constructing (&url.URL{Scheme: "file", Path: path}).String(); this will produce
a valid file:///C:/... URI on Windows and keep POSIX behavior unchanged.
In `@pkg/stack/imports/remote.go`:
- Around line 248-255: The temp directory for fetching is currently derived only
from uriToTempName(originalURI), causing races when Resolve is called
concurrently; change the tempDir creation to produce a unique per-call directory
(e.g., use os.MkdirTemp or filepath.Join(r.cache.BaseDir(), fmt.Sprintf("%s-%s",
uriToTempName(originalURI), uuidOrTimestamp))) and use that returned path for
both the call to r.downloader.Fetch and the deferred os.RemoveAll so only this
invocation's directory is removed; update any code that assumes the original
non-unique path (references: tempDir, uriToTempName(originalURI),
r.downloader.Fetch) accordingly.
---
Nitpick comments:
In `@tests/cli_remote_imports_test.go`:
- Around line 41-47: Add a git availability check to avoid hard failures when
git is missing: in the test entry (or at the top of the helper
runRemoteImportsGit), call exec.LookPath("git") and if it returns an error call
t.Skip("git not available") so tests are skipped instead of failing; update
runRemoteImportsGit (or the test that calls it) to perform this check before
running exec.Command("git", ...) and then proceed as before if git is found.
🪄 Autofix (Beta)
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
Run ID: c39e966f-240e-4f1d-bedf-72ac69ab7244
📒 Files selected for processing (4)
internal/exec/stack_processor_utils.gopkg/stack/imports/remote.gopkg/stack/imports/remote_test.gotests/cli_remote_imports_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/stack/imports/remote_test.go (1)
58-64: 💤 Low valueConsider CI environment compatibility.
These tests invoke the
gitbinary directly. While pragmatic for testing real git behavior, they'll skip or fail in environments without git installed.If CI stability becomes a concern, consider wrapping these tests with a skip check:
func skipWithoutGit(t *testing.T) { t.Helper() if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") } }🤖 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/stack/imports/remote_test.go` around lines 58 - 64, The tests call the external git binary from runGit which will fail in CI machines that lack git; add a pre-check helper (e.g., skipWithoutGit) that uses exec.LookPath("git") and t.Skip when not found, and invoke that helper at the start of Test functions (or inside runGit) so tests gracefully skip instead of failing in environments without git; update runGit or each test that uses runGit to call skipWithoutGit(t) before executing git commands.
🤖 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.
Nitpick comments:
In `@pkg/stack/imports/remote_test.go`:
- Around line 58-64: The tests call the external git binary from runGit which
will fail in CI machines that lack git; add a pre-check helper (e.g.,
skipWithoutGit) that uses exec.LookPath("git") and t.Skip when not found, and
invoke that helper at the start of Test functions (or inside runGit) so tests
gracefully skip instead of failing in environments without git; update runGit or
each test that uses runGit to call skipWithoutGit(t) before executing git
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c478dada-a983-4ee2-8b12-8410cc55055b
📒 Files selected for processing (3)
pkg/stack/imports/remote.gopkg/stack/imports/remote_test.gotests/cli_remote_imports_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2528 +/- ##
==========================================
+ Coverage 78.54% 78.61% +0.06%
==========================================
Files 1144 1145 +1
Lines 109984 110311 +327
==========================================
+ Hits 86387 86721 +334
+ Misses 18807 18777 -30
- Partials 4790 4813 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 27, 2026 14:18
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/stack/imports/remote_nested.go (1)
19-23: ⚡ Quick winUse
r.atmosConfiginstead ofnilin perf.Track.The receiver has access to
r.atmosConfig(used at line 128 inremoteStacksBasePath). Passing it ensures consistent tracking.Suggested fix
func (r *RemoteImporter) ResolveNested(uri, nestedImports string) ([]RemoteImportMatch, error) { - defer perf.Track(nil, "imports.RemoteImporter.ResolveNested")() + defer perf.Track(r.atmosConfig, "imports.RemoteImporter.ResolveNested")()🤖 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/stack/imports/remote_nested.go` around lines 19 - 23, The perf tracking call in RemoteImporter.ResolveNested currently passes nil which breaks consistency with other calls using the importer config; replace the defer perf.Track(nil, "imports.RemoteImporter.ResolveNested")() with a call that passes the importer config (use r.atmosConfig) so it matches how remoteStacksBasePath and other methods report metrics; update the defer to defer perf.Track(r.atmosConfig, "imports.RemoteImporter.ResolveNested")().pkg/stack/imports/remote_test.go (1)
290-314: ⚡ Quick winAdd an explicit prerequisite sub-test for git env propagation.
This test depends on subprocess env behavior (
GIT_CONFIG_GLOBAL,GIT_CONFIG_NOSYSTEM,HOME) but doesn’t assert that prerequisite explicitly before the main assertion. Add a small precheck sub-test that validatesgit configsees the expected rewrite first.Proposed test shape.
func TestRemoteImporter_Resolve_GitHubShorthandSubdir(t *testing.T) { repoDir := initGitRepo(t, map[string]string{ "stacks/dev.yaml": "vars:\n shorthand: true\n", }) @@ t.Setenv("HOME", t.TempDir()) + + t.Run("prerequisite: git sees insteadOf rewrite", func(t *testing.T) { + cmd := exec.Command("git", "config", "--global", "--get-regexp", "^url\\..*\\.insteadOf$") + out, err := cmd.CombinedOutput() + require.NoError(t, err, string(out)) + assert.Contains(t, string(out), "https://github.com/acme/infrastructure") + }) atmosConfig := &schema.AtmosConfiguration{} importer := newTestRemoteImporter(t, atmosConfig)As per coding guidelines, "
**/*_test.go: Add prerequisite sub-tests for subprocess behavior: when a test depends on implicit env propagation, add an explicit sub-test confirming the behavior before the main test runs.".🤖 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/stack/imports/remote_test.go` around lines 290 - 314, Add a small prerequisite sub-test at the start of TestRemoteImporter_Resolve_GitHubShorthandSubdir that validates the git subprocess sees the configured rewrite (the GIT_CONFIG_GLOBAL/GIT_CONFIG_NOSYSTEM/HOME envs set in the test). Specifically, before calling importer.Resolve, run a git subprocess (e.g., exec.Command("git","config","--global","--get","insteadOf", "https://github.com/acme/infrastructure") or "git config --global --list") with the same environment and assert its output contains the expected file://... rewrite (use gitFileURI(repoDir) to build the expected value); keep the check as a t.Run sub-test so failures clearly indicate env propagation issues. Ensure this runs prior to invoking newTestRemoteImporter/importer.Resolve.
🤖 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 `@pkg/stack/imports/remote_test.go`:
- Around line 27-53: Replace the manual stub type stubRemoteDownloader in
remote_test.go with a mockgen-generated mock for the downloader interface:
generate a mock (e.g., mock_downloader.NewMockDownloader) for the interface that
declares Fetch/FetchAnd* methods, create a gomock controller in the test,
instantiate the generated mock, and configure only the expected Fetch
interaction via mock.EXPECT().Fetch(...) with appropriate matchers for src,
dest, mode (downloader.ClientMode) and timeout to return the desired error or
nil; remove the manual Fetch/FetchAnd* implementations and update the test to
use the generated mock instance.
---
Nitpick comments:
In `@pkg/stack/imports/remote_nested.go`:
- Around line 19-23: The perf tracking call in RemoteImporter.ResolveNested
currently passes nil which breaks consistency with other calls using the
importer config; replace the defer perf.Track(nil,
"imports.RemoteImporter.ResolveNested")() with a call that passes the importer
config (use r.atmosConfig) so it matches how remoteStacksBasePath and other
methods report metrics; update the defer to defer perf.Track(r.atmosConfig,
"imports.RemoteImporter.ResolveNested")().
In `@pkg/stack/imports/remote_test.go`:
- Around line 290-314: Add a small prerequisite sub-test at the start of
TestRemoteImporter_Resolve_GitHubShorthandSubdir that validates the git
subprocess sees the configured rewrite (the
GIT_CONFIG_GLOBAL/GIT_CONFIG_NOSYSTEM/HOME envs set in the test). Specifically,
before calling importer.Resolve, run a git subprocess (e.g.,
exec.Command("git","config","--global","--get","insteadOf",
"https://github.com/acme/infrastructure") or "git config --global --list") with
the same environment and assert its output contains the expected file://...
rewrite (use gitFileURI(repoDir) to build the expected value); keep the check as
a t.Run sub-test so failures clearly indicate env propagation issues. Ensure
this runs prior to invoking newTestRemoteImporter/importer.Resolve.
🪄 Autofix (Beta)
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
Run ID: ddcb6582-41ec-4530-bebd-f6e8efbb7f86
📒 Files selected for processing (11)
docs/prd/import-adapter-registry.mdexamples/remote-stack-imports/README.mdinternal/exec/stack_processor_utils.gointernal/exec/stack_processor_utils_test.gopkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/schema/schema.gopkg/stack/imports/remote.gopkg/stack/imports/remote_nested.gopkg/stack/imports/remote_test.gotests/cli_remote_imports_test.gowebsite/docs/stacks/imports.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/stack/imports/remote.go
Andriy Knysh (aknysh)
approved these changes
May 28, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.220.0. |
This branch was successfully deployed
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
//subdirimports by cloning the repository as a directory, resolving files, no-extension YAML variants, explicit globs, and recursive YAML directory imports.<original-uri>#<relative-file>keys and update stack processing/tests to consume those keys for imports and provenance.why
git cloneto drop the repository name and targethttps://github.com/<owner>/.references
go test ./pkg/stack/imports ./pkg/downloader,go test ./tests -run RemoteStackImports, andgo test ./internal/exec -run '^$'.custom-gcllint binary.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests