Repository navigation
Conversation
…ntimes The codebase assumed macOS always means Docker Desktop, which provides transparent UID mapping and other special behaviors. Colima, Podman, and OrbStack on macOS don't provide these, causing several failures. Three fixes, all applied unconditionally (harmless no-ops where not needed): 1. Configure git safe.directory via GIT_CONFIG_COUNT env vars to prevent "dubious ownership" errors when container user UID differs from mounted workspace file ownership. Uses wildcard (*) matching VS Code devcontainer behavior. 2. Mount ~/.config/gh on all platforms. Previously gated by isLinux, making --gh-creds a silent no-op on macOS. The comment referencing Keychain-based copying was stale — that code never existed. 3. Run UID/GID sync on all platforms. Previously skipped on macOS with the assumption that Docker Desktop handles it automatically. The function already no-ops when UIDs match, so this is safe everywhere.
…times
Colima and Podman only share /Users by default. os.MkdirTemp("", ...)
creates dirs under /var/folders which macOS resolves to /private/var/folders
via symlink — a path the container VM can't access.
Fixes TestE2E_RealWorldNodeJS, TestE2E_CustomMounts,
TestE2E_MountVariableSubstitution, TestE2E_InitializeCommand,
and TestE2E_InitializeCommand_Object.
These commands require root privileges but the docker exec calls were not specifying -u root, causing silent failures on non-Docker-Desktop runtimes where the container user lacks root access.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAlways-mount GitHub CLI config when present, export git safe.directory via environment for containerized git, and perform UID/GID synchronization on all platforms; tests now use home-directory roots and two new E2E tests were added. Changes
Sequence DiagramsequenceDiagram
participant Runner
participant Host as Host Filesystem
participant Container
participant Git as Git (in container)
participant GHCli as GitHub CLI (in container)
Runner->>Host: Check for ~/.config/gh
alt GH config exists
Runner->>Container: Mount ~/.config/gh (all platforms)
end
Runner->>Container: Export GIT_CONFIG_COUNT, GIT_CONFIG_KEY_0, GIT_CONFIG_VALUE_0
Runner->>Container: Start container with mounts & env
alt UID/GID mismatch detected
Runner->>Container: Exec usermod/groupmod/chown as root (-u root)
Container->>Container: Apply UID/GID and ownership changes
end
Git->>Git: Read safe.directory from injected config
GHCli->>GHCli: Use mounted credentials from host
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/runner/runner.go (2)
1835-1841:⚠️ Potential issue | 🟠 MajorDon’t early-return on UID match without validating GID.
The function returns when UID matches, even if GID is still mismatched. That leaves group ownership drift unresolved and breaks the stated UID/GID sync behavior.
✅ Check both UID and GID before no-op
currentUID := strings.TrimSpace(currentUIDStr) - if currentUID == fmt.Sprintf("%d", hostUID) { + currentGIDStr, err := dockerClient.Run("exec", containerID, "id", "-g", username) + if err != nil { + return fmt.Errorf("failed to get current GID for user '%s': %w", username, err) + } + currentGID := strings.TrimSpace(currentGIDStr) + if currentUID == fmt.Sprintf("%d", hostUID) && currentGID == fmt.Sprintf("%d", hostGID) { if verbose { - fmt.Fprintf(os.Stderr, "User '%s' already has correct UID=%d, skipping sync\n", username, hostUID) + fmt.Fprintf(os.Stderr, "User '%s' already has correct UID=%d GID=%d, skipping sync\n", username, hostUID, hostGID) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/runner.go` around lines 1835 - 1841, The early return checks only currentUID and skips syncing even when GID differs; change the no-op to validate both UID and GID before returning by reading/trim-parsing the current GID (analogous to currentUID/currentUIDStr) and comparing it to hostGID as well, and only if currentUID == fmt.Sprintf("%d", hostUID) AND currentGID == fmt.Sprintf("%d", hostGID) (use the same verbose logging path with username and hostUID/hostGID) should the function return nil; otherwise proceed to fix the mismatched group ownership.
858-863:⚠️ Potential issue | 🟠 MajorMount GitHub CLI credentials read-only by default.
This bind mount is writable, which lets container processes modify host
~/.config/ghauth state. For credential mounts, default should be:rounless explicit write access is required.🔒 Suggested hardening
- args = append(args, "-v", fmt.Sprintf("%s:/home/%s/.config/gh", ghConfigPath, devConfig.RemoteUser)) + args = append(args, "-v", fmt.Sprintf("%s:/home/%s/.config/gh:ro", ghConfigPath, devConfig.RemoteUser))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/runner.go` around lines 858 - 863, The bind mount of the GitHub CLI config under the config.Credentials.GH check is currently writable; change the volume string constructed for ghConfigPath so it mounts read-only by default (add :ro to the target spec in the fmt.Sprintf that appends to args for devConfig.RemoteUser). If there is a use-case requiring write access, gate that with an explicit option (e.g., a new flag in config.Credentials or devConfig) and only omit :ro when that flag is set; keep the fileExists check and args append logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runner/e2e_test.go`:
- Around line 3877-3884: The test currently writes a fixed marker filename into
ghConfigDir and may delete real user data via os.Remove or os.RemoveAll; change
it to create an isolated temp config dir (use os.MkdirTemp) and point the test
at that directory (or set XDG_CONFIG_HOME) so markerFile is created inside the
temp dir, write the marker with os.WriteFile as before, and on cleanup only
remove files/dirs the test created (i.e., remove the temp dir unconditionally)
and avoid touching the real ghConfigDir; update logic that references
createdGHDir, markerFile, ghConfigDir and any os.RemoveAll calls to only remove
the temp directory you created.
In `@pkg/runner/runner.go`:
- Around line 1003-1009: The current code appends GIT_CONFIG_VALUE_0="*" which
globally trusts all repos; change the value to the mounted workspace path
instead so only that workspace is marked safe. Update the args append that sets
GIT_CONFIG_VALUE_0 (the line using GIT_CONFIG_VALUE_0=*) to use the workspace
path variable used by the runner (e.g., the workspace or repo path string the
container mounts), ensuring it is properly escaped/quoted; leave
GIT_CONFIG_COUNT and GIT_CONFIG_KEY_0 as-is so the safe.directory entry is added
but scoped to the workspace.
In `@README.md`:
- Line 367: Update the README Requirements section to reflect runtime
compatibility with Colima/Podman instead of or alongside the previous “Docker
Desktop on macOS” wording: locate the Requirements heading and replace the
macOS-specific Docker Desktop line (the text “Docker Desktop on macOS”) with a
statement that lists supported runtimes (e.g., Docker Desktop, Colima, Podman)
and any OS-specific notes; ensure the GitHub CLI path note (`~/.config/gh`)
remains accurate and remove any contradictory phrasing so the Requirements
section matches this PR’s Colima/Podman compatibility goal.
---
Outside diff comments:
In `@pkg/runner/runner.go`:
- Around line 1835-1841: The early return checks only currentUID and skips
syncing even when GID differs; change the no-op to validate both UID and GID
before returning by reading/trim-parsing the current GID (analogous to
currentUID/currentUIDStr) and comparing it to hostGID as well, and only if
currentUID == fmt.Sprintf("%d", hostUID) AND currentGID == fmt.Sprintf("%d",
hostGID) (use the same verbose logging path with username and hostUID/hostGID)
should the function return nil; otherwise proceed to fix the mismatched group
ownership.
- Around line 858-863: The bind mount of the GitHub CLI config under the
config.Credentials.GH check is currently writable; change the volume string
constructed for ghConfigPath so it mounts read-only by default (add :ro to the
target spec in the fmt.Sprintf that appends to args for devConfig.RemoteUser).
If there is a use-case requiring write access, gate that with an explicit option
(e.g., a new flag in config.Credentials or devConfig) and only omit :ro when
that flag is set; keep the fileExists check and args append logic intact.
| markerFile := filepath.Join(ghConfigDir, "packnplay-test-marker") | ||
| require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644)) | ||
| defer os.Remove(markerFile) | ||
| defer func() { | ||
| if createdGHDir { | ||
| os.RemoveAll(ghConfigDir) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Avoid mutating/deleting real user GH config content during test cleanup.
The test uses a fixed marker filename in ~/.config/gh and can delete a pre-existing user file with the same name. Cleanup also removes the whole directory when it was created by the test, which can delete files created during the test window by other processes.
🛠️ Safer marker handling
- markerFile := filepath.Join(ghConfigDir, "packnplay-test-marker")
+ markerFile := filepath.Join(ghConfigDir, fmt.Sprintf("packnplay-test-marker-%d", time.Now().UnixNano()))
require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644))
defer os.Remove(markerFile)
defer func() {
if createdGHDir {
- os.RemoveAll(ghConfigDir)
+ // Remove only if still empty; avoid deleting user-created files.
+ _ = os.Remove(ghConfigDir)
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| markerFile := filepath.Join(ghConfigDir, "packnplay-test-marker") | |
| require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644)) | |
| defer os.Remove(markerFile) | |
| defer func() { | |
| if createdGHDir { | |
| os.RemoveAll(ghConfigDir) | |
| } | |
| }() | |
| markerFile := filepath.Join(ghConfigDir, fmt.Sprintf("packnplay-test-marker-%d", time.Now().UnixNano())) | |
| require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644)) | |
| defer os.Remove(markerFile) | |
| defer func() { | |
| if createdGHDir { | |
| // Remove only if still empty; avoid deleting user-created files. | |
| _ = os.Remove(ghConfigDir) | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/e2e_test.go` around lines 3877 - 3884, The test currently writes a
fixed marker filename into ghConfigDir and may delete real user data via
os.Remove or os.RemoveAll; change it to create an isolated temp config dir (use
os.MkdirTemp) and point the test at that directory (or set XDG_CONFIG_HOME) so
markerFile is created inside the temp dir, write the marker with os.WriteFile as
before, and on cleanup only remove files/dirs the test created (i.e., remove the
temp dir unconditionally) and avoid touching the real ghConfigDir; update logic
that references createdGHDir, markerFile, ghConfigDir and any os.RemoveAll calls
to only remove the temp directory you created.
| // Configure git safe.directory to prevent "dubious ownership" errors | ||
| // when container user UID differs from mounted workspace file ownership | ||
| // (common with Colima, Podman, and other non-Docker-Desktop runtimes). | ||
| // Uses GIT_CONFIG_COUNT mechanism, additive to the mounted read-only .gitconfig. | ||
| args = append(args, "-e", "GIT_CONFIG_COUNT=1") | ||
| args = append(args, "-e", "GIT_CONFIG_KEY_0=safe.directory") | ||
| args = append(args, "-e", "GIT_CONFIG_VALUE_0=*") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the exact content of the file at the specified lines
cat -n pkg/runner/runner.go | sed -n '1000,1020p'Repository: obra/packnplay
Length of output: 1145
🏁 Script executed:
# Find the workingDir variable definition in the file
rg 'workingDir' pkg/runner/runner.go -B 2 -A 2Repository: obra/packnplay
Length of output: 2633
🏁 Script executed:
# Get more context around lines 1003-1009
cat -n pkg/runner/runner.go | sed -n '980,1030p'Repository: obra/packnplay
Length of output: 2591
🌐 Web query:
Git safe.directory GIT_CONFIG_COUNT GIT_CONFIG_KEY configuration environment variables
💡 Result:
GIT_CONFIG_COUNT / GIT_CONFIG_KEY_<n> / GIT_CONFIG_VALUE_<n> let you inject Git config via environment variables (a “command” scope config source). If GIT_CONFIG_COUNT is a positive integer, Git reads zero-indexed pairs GIT_CONFIG_KEY_0+GIT_CONFIG_VALUE_0 … up to COUNT-1. Missing keys/values are errors. These env-provided values override config files, but are overridden by explicit git -c key=value options. [1]
Example (set multiple runtime config entries):
export GIT_CONFIG_COUNT=2
export GIT_CONFIG_KEY_0=safe.directory
export GIT_CONFIG_VALUE_0=/srv/shared/repo
export GIT_CONFIG_KEY_1=core.sshCommand
export GIT_CONFIG_VALUE_1="ssh -i /run/secrets/deploy_key"
git status(“command” scope includes these env vars and -c.) [2]
safe.directory specifics
safe.directoryis a multi-valued config that marks repos as safe even if owned by another user; it’s only honored in protected configuration (system, global, and command scopes). [2][3]- Special forms:
- Set to
*to trust all repositories (disables the check). [3] - Append
/*to trust all repos under a directory. [3] ~/...and%(prefix)/...are interpolated/expanded. [3]- Add an empty
safe.directoryentry to reset/clear the list. [3]
- Set to
A real-world use is setting safe.directory=* for a service via environment variables (e.g., systemd), using the GIT_CONFIG_COUNT/KEY/VALUE mechanism. [4]
Sources:
[1] git-config docs (ENVIRONMENT: GIT_CONFIG_COUNT, GIT_CONFIG_KEY_<n>, GIT_CONFIG_VALUE_<n>), Git SCM. (git-scm.com)
[2] git-config docs (SCOPES / Protected configuration; “command” scope includes env pairs and -c), Git SCM. (git-scm.com)
[3] git-config docs (safe.directory behavior, wildcards, interpolation, reset), Git SCM. (git-scm.com)
[4] Debian bug thread showing systemd Environment=GIT_CONFIG_COUNT=... safe.directory=* usage. (mail-archive.com)
Citations:
- 1: https://git-scm.com/docs/git-config/2.45.3.html?utm_source=openai
- 2: https://git-scm.com/docs/git-config/2.51.0.html
- 3: https://git-scm.com/docs/git-config/2.51.0.html
- 4: https://www.mail-archive.com/debian-bugs-dist%40lists.debian.org/msg1990080.html?utm_source=openai
Scope safe.directory to workspace, not wildcard.
Using safe.directory=* trusts every repository in the container and weakens Git's ownership protection more than necessary. Set it to the mounted workspace path instead:
🎯 Narrower safe.directory configuration
- args = append(args, "-e", "GIT_CONFIG_VALUE_0=*")
+ args = append(args, "-e", fmt.Sprintf("GIT_CONFIG_VALUE_0=%s", workingDir))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Configure git safe.directory to prevent "dubious ownership" errors | |
| // when container user UID differs from mounted workspace file ownership | |
| // (common with Colima, Podman, and other non-Docker-Desktop runtimes). | |
| // Uses GIT_CONFIG_COUNT mechanism, additive to the mounted read-only .gitconfig. | |
| args = append(args, "-e", "GIT_CONFIG_COUNT=1") | |
| args = append(args, "-e", "GIT_CONFIG_KEY_0=safe.directory") | |
| args = append(args, "-e", "GIT_CONFIG_VALUE_0=*") | |
| // Configure git safe.directory to prevent "dubious ownership" errors | |
| // when container user UID differs from mounted workspace file ownership | |
| // (common with Colima, Podman, and other non-Docker-Desktop runtimes). | |
| // Uses GIT_CONFIG_COUNT mechanism, additive to the mounted read-only .gitconfig. | |
| args = append(args, "-e", "GIT_CONFIG_COUNT=1") | |
| args = append(args, "-e", "GIT_CONFIG_KEY_0=safe.directory") | |
| args = append(args, "-e", fmt.Sprintf("GIT_CONFIG_VALUE_0=%s", workingDir)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/runner.go` around lines 1003 - 1009, The current code appends
GIT_CONFIG_VALUE_0="*" which globally trusts all repos; change the value to the
mounted workspace path instead so only that workspace is marked safe. Update the
args append that sets GIT_CONFIG_VALUE_0 (the line using GIT_CONFIG_VALUE_0=*)
to use the workspace path variable used by the runner (e.g., the workspace or
repo path string the container mounts), ensuring it is properly escaped/quoted;
leave GIT_CONFIG_COUNT and GIT_CONFIG_KEY_0 as-is so the safe.directory entry is
added but scoped to the workspace.
There was a problem hiding this comment.
Keeping the wildcard intentionally. This is a sandboxed dev container — scoping to the workspace path would break git submodules, nested repos, or any tools that clone repos elsewhere in the container. The security benefit of restricting safe.directory is negligible in an ephemeral sandbox.
There was a problem hiding this comment.
@schuyler, that makes sense! You're right that in an ephemeral dev container where users may be working with submodules, nested repos, or tools that dynamically clone repositories, the wildcard approach is more practical. The sandbox isolation already provides the security boundary. Thanks for the clarification!
os.MkdirTemp creates with 0700, preventing non-root container users from accessing the directory on Linux (no transparent UID mapping).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/runner/e2e_test.go (1)
3870-3886:⚠️ Potential issue | 🟠 MajorIsolate test credentials config instead of mutating real
~/.config/gh.Line 3879 uses a fixed marker in the real user config, and Line 3884 can remove the whole directory. This can delete real data or race with other processes.
🛠️ Safer approach
- ghConfigDir := filepath.Join(homeDir, ".config", "gh") - createdGHDir := false - if !fileExists(ghConfigDir) { - require.NoError(t, os.MkdirAll(ghConfigDir, 0755)) - createdGHDir = true - } - markerFile := filepath.Join(ghConfigDir, "packnplay-test-marker") - require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644)) - defer os.Remove(markerFile) - defer func() { - if createdGHDir { - os.RemoveAll(ghConfigDir) - } - }() + testHome, err := os.MkdirTemp(homeDir, "packnplay-e2e-home-*") + require.NoError(t, err) + defer os.RemoveAll(testHome) + + oldHome := os.Getenv("HOME") + require.NoError(t, os.Setenv("HOME", testHome)) + defer func() { _ = os.Setenv("HOME", oldHome) }() + + ghConfigDir := filepath.Join(testHome, ".config", "gh") + require.NoError(t, os.MkdirAll(ghConfigDir, 0755)) + + markerFile := filepath.Join(ghConfigDir, fmt.Sprintf("packnplay-test-marker-%d", time.Now().UnixNano())) + require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/e2e_test.go` around lines 3870 - 3886, The test must not mutate the user's real ~/.config/gh; instead create an isolated temporary config dir (use os.MkdirTemp) and point the test to it (set XDG_CONFIG_HOME or adjust ghConfigDir usage) so fileExists, ghConfigDir and markerFile refer to the temp location; write the marker there and defer os.RemoveAll on the temp dir (remove createdGHDir logic and any removal of the real ~/.config/gh), ensuring cleanup always runs and any env var set for the test is restored after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/runner/e2e_test.go`:
- Around line 3870-3886: The test must not mutate the user's real ~/.config/gh;
instead create an isolated temporary config dir (use os.MkdirTemp) and point the
test to it (set XDG_CONFIG_HOME or adjust ghConfigDir usage) so fileExists,
ghConfigDir and markerFile refer to the temp location; write the marker there
and defer os.RemoveAll on the temp dir (remove createdGHDir logic and any
removal of the real ~/.config/gh), ensuring cleanup always runs and any env var
set for the test is restored after the test.
- Use os.Remove instead of os.RemoveAll for gh config dir cleanup - Update Requirements to list Colima and Podman as supported runtimes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/runner/e2e_test.go (1)
3870-3886:⚠️ Potential issue | 🟠 MajorIsolate GH config in a test sandbox; current marker flow can clobber user data.
At Line 3879, the test writes a fixed marker filename in real
~/.config/gh. If that file already exists, it is overwritten and then removed during cleanup.🛠️ Safer test-local GH config approach
- // Create ~/.config/gh with a marker file if it doesn't exist - homeDir, err := os.UserHomeDir() + // Create an isolated test HOME under the real home path so it remains mountable + realHomeDir, err := os.UserHomeDir() require.NoError(t, err) - ghConfigDir := filepath.Join(homeDir, ".config", "gh") - createdGHDir := false - if !fileExists(ghConfigDir) { - require.NoError(t, os.MkdirAll(ghConfigDir, 0755)) - createdGHDir = true - } - markerFile := filepath.Join(ghConfigDir, "packnplay-test-marker") + testHomeDir, err := os.MkdirTemp(realHomeDir, "packnplay-e2e-home-*") + require.NoError(t, err) + defer os.RemoveAll(testHomeDir) + + oldHome := os.Getenv("HOME") + require.NoError(t, os.Setenv("HOME", testHomeDir)) + defer func() { + if oldHome == "" { + _ = os.Unsetenv("HOME") + } else { + _ = os.Setenv("HOME", oldHome) + } + }() + + ghConfigDir := filepath.Join(testHomeDir, ".config", "gh") + require.NoError(t, os.MkdirAll(ghConfigDir, 0755)) + markerFile := filepath.Join(ghConfigDir, fmt.Sprintf("packnplay-test-marker-%d", time.Now().UnixNano())) require.NoError(t, os.WriteFile(markerFile, []byte("gh-creds-test\n"), 0644)) defer os.Remove(markerFile) - defer func() { - if createdGHDir { - os.Remove(ghConfigDir) - } - }() // Create temp dir under user's home so it's accessible inside VM-based Docker // runtimes (Colima, etc.) which may only share /Users, not /var/folders - projectDir, err := os.MkdirTemp(homeDir, "packnplay-e2e-ghcreds-*") + projectDir, err := os.MkdirTemp(realHomeDir, "packnplay-e2e-ghcreds-*") require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/e2e_test.go` around lines 3870 - 3886, The test currently writes a marker into the real ~/.config/gh (variables homeDir, ghConfigDir, markerFile, and fileExists) which can clobber user data; change the test to create a sandboxed config directory (use t.TempDir() or os.MkdirTemp) and set XDG_CONFIG_HOME (or HOME) to that temp dir for the duration of the test, then create the gh subpath and markerFile inside that sandbox instead of using os.UserHomeDir(); ensure cleanup by letting the test temp dir remove itself and restore any modified env var in a deferred function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/runner/e2e_test.go`:
- Around line 3870-3886: The test currently writes a marker into the real
~/.config/gh (variables homeDir, ghConfigDir, markerFile, and fileExists) which
can clobber user data; change the test to create a sandboxed config directory
(use t.TempDir() or os.MkdirTemp) and set XDG_CONFIG_HOME (or HOME) to that temp
dir for the duration of the test, then create the gh subpath and markerFile
inside that sandbox instead of using os.UserHomeDir(); ensure cleanup by letting
the test temp dir remove itself and restore any modified env var in a deferred
function.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdpkg/runner/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
…ile generator Fixes staticcheck QF1012 lint violation.
58a0bbf to
fc7ba31
Compare
Summary
$HOMEfor E2E test temp dirs so they're mountable by all runtimes (Colima only shares/Usersby default, not/var/folders)usermod/groupmod/chownas root inupdateRemoteUserUID(non-Docker-Desktop runtimes don't implicitly run exec as root)~/.config/ghon all platforms, not just LinuxGIT_CONFIG_COUNTenv vars to configuresafe.directory=*and prevent "dubious ownership" errorsupdateRemoteUserUIDsince Docker Desktop isn't the only macOS runtimeTest plan
go test ./pkg/runner/ -run TestE2E_UpdateRemoteUserUID -vgo test ./pkg/runner/ -run TestE2E_GitSafeDirectory -vgo test ./pkg/runner/ -run TestE2E_GHCredsMount -vSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests