Merged
Conversation
ws.Config.UserEnvProbe was parsed but never applied: Exec passed the raw cmd to the runtime, so PATH additions injected by the user's rc files (nvm, asdf, feature-installed binaries in ~/.local/bin) were invisible to library callers. Discovered downstream in crunchloop/dap. Adopt devpod's probe-once-and-inject model rather than @devcontainers/cli's per-exec shell wrapping: probe with bash -lic (or the configured variant) once after Up's lifecycle and on Attach, parse the resulting env, and merge it under every Engine.Exec — layered as probed < cfg.RemoteEnv < caller's opts.Env, with devpod's PATH-aware merge (/sbin stripped for non-root). Adds ExecOptions.SkipUserEnvProbe for callers that want a clean env (internal probes, raw-env helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds user environment probing during Up/Attach to capture shell-modified env into Workspace.probedEnv and conditionally merges that plus remoteEnv into Exec calls; per-call opt-out via ExecOptions.SkipUserEnvProbe. ChangesUser Environment Probing Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant Container
participant Workspace
Client->>Engine: Up / AttachWith
Engine->>Container: perform lifecycle
Engine->>Container: spawn probe shell
Container-->>Engine: env bytes
Engine->>Workspace: set ws.probedEnv
Client->>Engine: Exec (may SkipUserEnvProbe)
Engine->>Container: exec with merged or caller env
Container-->>Engine: command output
Engine-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issuesPossibly related PRsPoem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@exec.go`:
- Around line 31-36: The comment for SkipUserEnvProbe is inaccurate: when Exec
uses SkipUserEnvProbe it actually skips the entire merge path (so RemoteEnv is
not merged) rather than only leaving RemoteEnv merging; update the comment near
the Exec implementation and the SkipUserEnvProbe field to reflect that behavior
by stating that setting SkipUserEnvProbe causes Exec to bypass probing and skip
merging probedEnv and RemoteEnv into the process environment, and mention
probedEnv remains empty for lifecycle scripts — reference the SkipUserEnvProbe
field, Exec function, RemoteEnv and probedEnv symbols when editing the doc to
ensure correct, unambiguous behavior is documented.
In `@userenvprobe.go`:
- Around line 89-90: The parsing currently mutates env values by calling
strings.TrimSpace on each fragment (line := strings.TrimSpace(s[start:i]));
remove the TrimSpace call and use the raw slice string(s[start:i]) so
leading/trailing spaces in values are preserved, optionally skipping truly empty
fragments (e.g., if len(s[start:i]) == 0) before processing; keep the same start
= i + 1 update and downstream logic that consumes variable name/value from the
preserved line.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3440d639-c677-403a-91a5-e30b6e9de63c
📒 Files selected for processing (7)
attach.goengine_test.goexec.gotest/integration/userenvprobe_test.goup.gouserenvprobe.goworkspace.go
- SkipUserEnvProbe also bypasses RemoteEnv merging (not just probedEnv); doc previously implied otherwise. - parseEnvironBytes no longer TrimSpaces entries — env values may legitimately carry leading/trailing whitespace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@userenvprobe.go`:
- Around line 166-167: The root-detection is too narrow: update the if condition
that currently checks remoteUser == "root" (used when deciding whether to keep
/sbin segments before inserting into probedTokens) to treat all root
representations as root by including remoteUser == "0", remoteUser == ""
(container default), and the ContainerUser constant; i.e., change the check in
the block that references remoteUser, sbinSegment, probedTokens, insertAt and
slices.Insert to consider remoteUser == "root" || remoteUser == "0" ||
remoteUser == "" || remoteUser == ContainerUser so /sbin is preserved for all
root cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ce5a5e2-cd80-484f-97ce-bd90b41d5b42
📒 Files selected for processing (2)
exec.gouserenvprobe.go
Treat "0" and "" as root in addition to "root". Many minimal images (alpine, bash:*-alpine) run as root by default but report empty remoteUser via effectiveUser; the narrow check stripped /sbin entries that the user actually has access to. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Summary
ws.Config.UserEnvProbewas parsed but never applied —Engine.Execpassed the raw cmd to the runtime, so PATH additions from the user's rc files (nvm, asdf, feature-installed~/.local/bin) were invisible to library callers. Discovered downstream incrunchloop/dap.@devcontainers/cli's per-exec shell wrapping: probe withbash -lic(or the configured variant) once afterUp's lifecycle and onAttach, then merge the captured env under everyEngine.Exec.probedEnv<cfg.RemoteEnv< caller'sopts.Env, with devpod's PATH-aware merge (/sbinstripped for non-root).ExecOptions.SkipUserEnvProbefor callers that want a clean env (internal probes, raw-env helpers).Why probe-once instead of per-exec shell wrapping
bashstartup or.bashrcparse.exec "$@"trick)..bashrcis edited at runtime — re-Up/re-Attachre-probes.Test plan
go test -short ./...— existing unit tests pass (one assertion adjusted: Up now performs an extra probe exec).go test -tags=integration ./test/integration/...— 5 new integration tests, all green:TestUserEnvProbe_PathFromBashrc—postCreateCommandwrites/etc/profile.d/*.sh; default-probe Exec seesEXTRA_PATH.TestUserEnvProbe_None—userEnvProbe: \"none\"skips injection.TestUserEnvProbe_SkipOption— per-Exec opt-out works even with default probe.TestUserEnvProbe_NoBashFallback— alpine (no bash) doesn't break Up; Exec still works (graceful fallback).TestUserEnvProbe_RemoteEnvMerged—remoteEnvreaches Exec.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests