feat(config): parse and resolve devcontainer secrets property#83
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 53 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant ConfigFile as Config File
participant Parser as Config Parser
participant LifecycleEnv as Lifecycle Env Resolver
participant HostEnv as Host Environment
participant RemoteEnv as Remote Environment
ConfigFile->>Parser: Parse devcontainer.json
Parser->>Parser: Extract Secrets declarations
Parser->>LifecycleEnv: Pass MergedConfig with Secrets
LifecycleEnv->>LifecycleEnv: Build merged remote environment
LifecycleEnv->>HostEnv: Resolve each secret via os.Getenv
HostEnv-->>LifecycleEnv: Return secret values
LifecycleEnv->>RemoteEnv: Apply resolved secrets to env
LifecycleEnv-->>Parser: Return lifecycleEnv with secrets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (1)
pkg/devcontainer/config/config.go (1)
113-116: Consider renamingDocumentationUrltoDocumentationURL.Go style (and common linters like revive/golint
var-naming) recommends capitalizing initialisms such asURL. The JSON tag keeps the wire format unchanged, so this is purely a Go-side identifier rename.Proposed rename
type SecretConfig struct { Description string `json:"description,omitempty"` - DocumentationUrl string `json:"documentationUrl,omitempty"` + DocumentationURL string `json:"documentationUrl,omitempty"` }The test in
pkg/devcontainer/config/parse_test.go(line 43) would need the matching rename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/config/config.go` around lines 113 - 116, Rename the SecretConfig field DocumentationUrl to DocumentationURL to follow Go initialism conventions; update the struct in SecretConfig (change DocumentationUrl → DocumentationURL) but keep the json tag `documentationUrl` unchanged so wire format is preserved, and update any references in tests and code (e.g., pkg/devcontainer/config/parse_test.go and any uses of SecretConfig.DocumentationUrl) to the new identifier.
🤖 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/devcontainer/config/config.go`:
- Around line 107-110: MergeConfiguration currently copies
DevContainerConfigBase (which contains Secrets) from the user config once and
never re-merges imageMetadataEntries, causing feature-provided secrets to be
dropped; update MergeConfiguration in pkg/devcontainer/config/merge.go to merge
Secrets the same way RemoteEnv/ContainerEnv are merged by calling mergeMaps for
the Secrets map on each imageMetadataEntries iteration (use the same last-wins
merge policy as the other env maps) and add a brief comment documenting the
chosen policy so feature-contributed secrets are not silently ignored.
In `@pkg/devcontainer/setup/lifecyclehooks.go`:
- Around line 46-51: The current loop over mergedConfig.Secrets uses os.Getenv
and skips host-provided empty-string values and logs nothing when a secret is
missing; change it to use os.LookupEnv to distinguish unset vs empty-string so
intentionally-empty secrets are propagated according to policy, and add a
debug/warn log (without printing secret values) when LookupEnv reports the
variable is not present; update the loop that writes into env (referencing
mergedConfig.Secrets and env) to call os.LookupEnv and call the package logger
(or processLogger) to record unresolved secret names only.
- Around line 44-56: The current code applies mergedConfig.Secrets after calling
mergeRemoteEnv, letting host secrets override author-specified RemoteEnv; change
the order so secrets are merged into probedEnv before calling mergeRemoteEnv
(i.e., apply host secrets to the probedEnv map, then call
mergeRemoteEnv(probedEnv,...)) so mergeRemoteEnv can correctly let
mergedConfig.RemoteEnv override secrets; update any test (e.g.,
TestResolveLifecycleEnvIncludesSecrets) to include a collision case asserting
author RemoteEnv wins, and reference mergeRemoteEnv, mergedConfig.Secrets, and
lifecycleEnv when making the change.
---
Nitpick comments:
In `@pkg/devcontainer/config/config.go`:
- Around line 113-116: Rename the SecretConfig field DocumentationUrl to
DocumentationURL to follow Go initialism conventions; update the struct in
SecretConfig (change DocumentationUrl → DocumentationURL) but keep the json tag
`documentationUrl` unchanged so wire format is preserved, and update any
references in tests and code (e.g., pkg/devcontainer/config/parse_test.go and
any uses of SecretConfig.DocumentationUrl) to the new identifier.
🪄 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: 6c3cea80-2034-4f1e-b60f-5688490606d1
📒 Files selected for processing (4)
pkg/devcontainer/config/config.gopkg/devcontainer/config/parse_test.gopkg/devcontainer/setup/lifecyclehooks.gopkg/devcontainer/setup/lifecyclehooks_test.go
| // Secrets declared by the dev container. Keys are secret names; values describe the secret. | ||
| // At runtime, each key is looked up in the host environment and, if set, injected into lifecycle commands. | ||
| Secrets map[string]SecretConfig `json:"secrets,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Secrets is not merged across ImageMetadata entries.
rg -nP -C3 '\bSecrets\b' --type=goRepository: devsy-org/devsy
Length of output: 12220
🏁 Script executed:
cat -n pkg/devcontainer/config/merge.goRepository: devsy-org/devsy
Length of output: 8867
🏁 Script executed:
# Search for ImageMetadata struct definition and its fields
rg -n "type ImageMetadata struct" -A 50 --type=goRepository: devsy-org/devsy
Length of output: 1896
🏁 Script executed:
# Look for merge tests related to Secrets or features
fd -e "*_test.go" -x grep -l "MergeConfiguration\|merge.*[Ss]ecrets" {} \;Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Find all merge-related test files
fd -e "merge*test.go" --type=fRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# List all test files in config package
fd -e "*test.go" pkg/devcontainer/config/Repository: devsy-org/devsy
Length of output: 460
🏁 Script executed:
# List all test files in config package
fd . 'pkg/devcontainer/config/' -e '*test.go'Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Search for test files in the config directory
ls -la pkg/devcontainer/config/ | grep testRepository: devsy-org/devsy
Length of output: 263
🏁 Script executed:
# Search for MergeConfiguration in test files
rg -n "MergeConfiguration" pkg/devcontainer/config/ --type=goRepository: devsy-org/devsy
Length of output: 119
🏁 Script executed:
# Search for MergeConfiguration usage across the codebase
rg -n "MergeConfiguration" --type=goRepository: devsy-org/devsy
Length of output: 490
🏁 Script executed:
# Search for feature-related content and secrets in the codebase
rg -n "feature" -i pkg/devcontainer/config/merge.goRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Search for devcontainer spec references or feature documentation
rg -n "feature.*secret|secret.*feature" -i --type=goRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Check for any comments in merge.go about missing functionality or TODOs
rg -n "TODO|FIXME|XXX|BUG" pkg/devcontainer/config/merge.goRepository: devsy-org/devsy
Length of output: 96
🏁 Script executed:
# Check the devcontainer spec comment in config.go for Secrets context
rg -n -B5 "Secrets.*secret" pkg/devcontainer/config/config.goRepository: devsy-org/devsy
Length of output: 511
🏁 Script executed:
# Check how imageMetadataEntries are populated/created to understand if features can declare secrets
rg -n "imageMetadata|ImageMetadata" pkg/devcontainer/ -A3 | head -80Repository: devsy-org/devsy
Length of output: 5049
🏁 Script executed:
# Find FeatureConfigToImageMetadata function
rg -n "FeatureConfigToImageMetadata" -A 20 --type=goRepository: devsy-org/devsy
Length of output: 3279
🏁 Script executed:
# Check the FeatureConfig struct definition
rg -n "type FeatureConfig struct" -A 30 --type=goRepository: devsy-org/devsy
Length of output: 2881
Secrets from feature/image metadata are dropped during merge.
Per MergeConfiguration in pkg/devcontainer/config/merge.go, the DevContainerConfigBase (which includes Secrets) is copied from the user config once at initialization (line 31) and never re-merged from the imageMetadataEntries. Since ImageMetadata embeds DevContainerConfigBase, any secrets declared in feature metadata are silently ignored.
The codebase already uses mergeMaps() for other maps like RemoteEnv and ContainerEnv. Apply the same pattern to Secrets to ensure feature-contributed secrets are properly merged (e.g., using a last-wins or first-wins policy with clear documentation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/config/config.go` around lines 107 - 110, MergeConfiguration
currently copies DevContainerConfigBase (which contains Secrets) from the user
config once and never re-merges imageMetadataEntries, causing feature-provided
secrets to be dropped; update MergeConfiguration in
pkg/devcontainer/config/merge.go to merge Secrets the same way
RemoteEnv/ContainerEnv are merged by calling mergeMaps for the Secrets map on
each imageMetadataEntries iteration (use the same last-wins merge policy as the
other env maps) and add a brief comment documenting the chosen policy so
feature-contributed secrets are not silently ignored.
| env := mergeRemoteEnv(mergedConfig.RemoteEnv, probedEnv, remoteUser) | ||
|
|
||
| // Resolve declared secrets from the host environment. | ||
| for name := range mergedConfig.Secrets { | ||
| if val := os.Getenv(name); val != "" { | ||
| env[name] = val | ||
| } | ||
| } | ||
|
|
||
| return lifecycleEnv{ | ||
| remoteUser: remoteUser, | ||
| workspaceFolder: setupInfo.SubstitutionContext.ContainerWorkspaceFolder, | ||
| remoteEnv: mergeRemoteEnv(mergedConfig.RemoteEnv, probedEnv, remoteUser), | ||
| remoteEnv: env, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
devcontainer.json secrets spec precedence remoteEnv
💡 Result:
In devcontainer.json, there is a "secrets" property for declaring recommended secrets, primarily used in GitHub Codespaces to prompt users to set user-specific secrets. These are not stored in the file but reference secrets provided by the implementing tool/service (e.g., Codespaces secrets). Secrets are handled similarly to "remoteEnv": implementing tools inject them as environment variables for the runtime environment (VS Code server, terminals, tasks, etc.) after the container's ENTRYPOINT. The devcontainers CLI supports passing secrets via --secrets-file for commands like "up", applying them like remoteEnv. Precedence: devcontainer.json takes precedence over image/feature metadata for properties like remoteEnv and containerEnv (per-variable last value wins, devcontainer.json last when order matters). Secrets follow similar logic if contributed by features/images. No native Docker buildkit --secret support in devcontainer.json; use workarounds like docker-compose build.secrets or build.options. Example declaration: { "secrets": { "MY_SECRET": { "description": "Personal API key" } }, "remoteEnv": { "MY_SECRET": "${secret:MY_SECRET}" } }
Citations:
- 1: https://github.com/devcontainers/spec/blob/main/docs/specs/devcontainer-reference.md
- 2: https://code.visualstudio.com/docs/devcontainers/devcontainerjson-reference
- 3: Declarative secrets devcontainers/spec#198
- 4: https://containers.dev/implementors/spec
- 5: https://docs.github.com/en/codespaces/setting-up-your-project-for-codespaces/configuring-dev-containers/specifying-recommended-secrets-for-a-repository
- 6: https://containers.dev/implementors/json_schema
- 7: Implementing secret support proposal in devcontainers CLI devcontainers/cli#507
- 8: https://www.spec.ab.ca
- 9: https://www.techcommunity.microsoft.com/category/azure
🏁 Script executed:
# First, let's look at the full function and surrounding context
cat -n pkg/devcontainer/setup/lifecyclehooks.go | head -80Repository: devsy-org/devsy
Length of output: 2961
🏁 Script executed:
# Look for test cases related to secret handling
fd -t f lifecyclehooks_test.goRepository: devsy-org/devsy
Length of output: 104
🏁 Script executed:
# Search for how secrets are used elsewhere in the codebase
rg -i "secrets" pkg/devcontainer/setup/ -A 3 -B 3 --max-count=50Repository: devsy-org/devsy
Length of output: 1754
🏁 Script executed:
# Check for documentation or spec about secret precedence
fd -t f -i "spec\|readme\|doc" pkg/devcontainer/Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# View the full test case for secret handling
cat -n pkg/devcontainer/setup/lifecyclehooks_test.go | grep -A 40 "TestResolveLifecycleEnvIncludesSecrets"Repository: devsy-org/devsy
Length of output: 1350
🏁 Script executed:
# Find the mergeRemoteEnv function definition
rg "func mergeRemoteEnv" pkg/devcontainer/setup/ -A 20Repository: devsy-org/devsy
Length of output: 1673
🏁 Script executed:
# Check the Secrets configuration in the config package
rg "Secrets\s*map\[string\]" pkg/devcontainer/config/ -B 2 -A 2Repository: devsy-org/devsy
Length of output: 494
🏁 Script executed:
# Search for any comments or TODOs about secret precedence
rg -i "secret.*precedence\|precedence.*secret\|secret.*override\|override.*secret" pkg/devcontainer/Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Check if there are any related GitHub issues or discussions referenced
rg -i "secret.*remoteenv\|remoteenv.*secret" pkg/devcontainer/ -B 3 -A 3Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Look at git history or comments around the secret handling code
rg "Resolve declared secrets" pkg/devcontainer/ -B 5 -A 10Repository: devsy-org/devsy
Length of output: 1111
Secrets applied after mergeRemoteEnv create inconsistent precedence — author-specified values may be unexpectedly overwritten.
Secrets from the host environment (lines 47–50) are injected after mergeRemoteEnv(), which means a secret name matching a key in mergedConfig.RemoteEnv will overwrite the author-explicitly-set value. However, mergeRemoteEnv() itself states: "remoteEnv should always override probedEnv as it has been specified explicitly by the devcontainer author." This creates conflicting precedence: author-set RemoteEnv is prioritized over probed env, but then overridden by secrets. Either the order should be reversed (secrets first, then RemoteEnv), or the behavior should be clarified with a warning when a collision occurs. The existing test (TestResolveLifecycleEnvIncludesSecrets) does not cover the collision scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/setup/lifecyclehooks.go` around lines 44 - 56, The current
code applies mergedConfig.Secrets after calling mergeRemoteEnv, letting host
secrets override author-specified RemoteEnv; change the order so secrets are
merged into probedEnv before calling mergeRemoteEnv (i.e., apply host secrets to
the probedEnv map, then call mergeRemoteEnv(probedEnv,...)) so mergeRemoteEnv
can correctly let mergedConfig.RemoteEnv override secrets; update any test
(e.g., TestResolveLifecycleEnvIncludesSecrets) to include a collision case
asserting author RemoteEnv wins, and reference mergeRemoteEnv,
mergedConfig.Secrets, and lifecycleEnv when making the change.
| // Resolve declared secrets from the host environment. | ||
| for name := range mergedConfig.Secrets { | ||
| if val := os.Getenv(name); val != "" { | ||
| env[name] = val | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty-string host values silently skip declared secrets; also consider logging unresolved secrets.
Two concerns on this block:
if val := os.Getenv(name); val != ""conflates "unset" with "set to empty string".os.LookupEnvis the idiomatic way to distinguish the two; whether an intentionally-empty secret should be propagated is a policy decision worth making explicit.- When a declared secret is missing from the host environment, nothing is logged. Given these are declared secrets users opted into, a debug/warn log (without the value) would aid troubleshooting when lifecycle commands fail due to missing env vars.
Suggested adjustment
- // Resolve declared secrets from the host environment.
- for name := range mergedConfig.Secrets {
- if val := os.Getenv(name); val != "" {
- env[name] = val
- }
- }
+ // Resolve declared secrets from the host environment.
+ for name := range mergedConfig.Secrets {
+ if val, ok := os.LookupEnv(name); ok {
+ env[name] = val
+ } else {
+ log.Debugf("declared secret %q is not set in host environment; skipping", name)
+ }
+ }Note: the existing test asserts that an absent secret is not present in remoteEnv, which the suggested change preserves. The val != "" check test case isn't covered — worth adding if you keep that semantic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/setup/lifecyclehooks.go` around lines 46 - 51, The current
loop over mergedConfig.Secrets uses os.Getenv and skips host-provided
empty-string values and logs nothing when a secret is missing; change it to use
os.LookupEnv to distinguish unset vs empty-string so intentionally-empty secrets
are propagated according to policy, and add a debug/warn log (without printing
secret values) when LookupEnv reports the variable is not present; update the
loop that writes into env (referencing mergedConfig.Secrets and env) to call
os.LookupEnv and call the package logger (or processLogger) to record unresolved
secret names only.
Split TestSecretsRoundTrip into TestParseSecrets and TestSecretsRoundTrip to reduce complexity. Add nolint:cyclop directive for test assertions and nosec G306 for test file writes (consistent with existing tests).
1 participant
Summary
Secrets map[string]SecretConfigfield toDevContainerConfigBasefor parsing the devcontainer specsecretspropertySecretConfigstruct withDescriptionandDocumentationUrlfieldsSummary by CodeRabbit