Skip to content

feat(config): enforce phase-aware variable substitution scoping#249

Merged
skevetter merged 2 commits into
mainfrom
ce37-819c-ws-a-varsub-scope
May 6, 2026
Merged

feat(config): enforce phase-aware variable substitution scoping#249
skevetter merged 2 commits into
mainfrom
ce37-819c-ws-a-varsub-scope

Conversation

@skevetter

@skevetter skevetter commented May 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements two-pass variable substitution per the devcontainer spec (Variables in devcontainer.json):

  • Pre-container fields (containerEnv) now only resolve local-scoped variables (devcontainerId, localEnv, localWorkspaceFolder, localWorkspaceFolderBasename). Container-scoped variables (${containerWorkspaceFolder}, ${containerWorkspaceFolderBasename}, ${containerEnv:VAR}) are preserved as literals since the container doesn't exist yet at evaluation time.
  • Post-container fields (remoteEnv, lifecycle commands, etc.) continue to resolve all variables including container-scoped ones.
  • Adds restrictedReplace() wrapper function that intercepts container-scoped variable resolution while delegating everything else to the full replacer.
  • Adds comprehensive unit tests (containerEnv scoping, remoteEnv scoping, mixed config) and an E2E test verifying runtime behavior.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed variable substitution in devcontainer configurations to properly respect scope boundaries, ensuring container-scoped variables are correctly preserved during configuration processing.
  • Tests

    • Added comprehensive test coverage for variable substitution scope handling across multiple substitution phases.

Per the devcontainer spec, properties evaluated before container creation
(containerEnv) should not have access to containerWorkspaceFolder or
containerWorkspaceFolderBasename — those variables only exist after the
container is running. This implements two-pass substitution: pre-container
fields use a restricted replacer that preserves container-scoped variables
as literals, while post-container fields get full variable resolution.
@netlify

netlify Bot commented May 6, 2026

Copy link
Copy Markdown

Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit 7b1ad5f
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/69fb76cb4e55a5000873580d

@github-actions github-actions Bot added the size/l label May 6, 2026
@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

This PR implements phase-aware variable substitution for devcontainer configuration. A two-pass substitution mechanism ensures container-scoped variables (containerEnv) are preserved as literals during pre-container field resolution, then fully substituted afterward. Changes include core substitution logic, unit tests, and end-to-end validation.

Changes

Variable Substitution Scoping

Layer / File(s) Summary
Constants & Types
pkg/devcontainer/config/substitute.go, pkg/devcontainer/config/substitute_test.go
Introduces containerEnvField constant and test constants for workspace variables; adds scopeTestConfig type and scopeTestCtx helper for scoped substitution testing.
Core Two-Pass Logic
pkg/devcontainer/config/substitute.go
Refactors Substitute function to perform restricted pre-container field substitution in the first pass, then full substitution for remaining fields, with results merged back.
Helper Functions
pkg/devcontainer/config/substitute.go
Extends replaceWithContext to recognize containerEnvField and return literal matches; adds restrictedReplace wrapper to preserve container-scoped variables during pre-container substitution.
Unit Tests
pkg/devcontainer/config/substitute_test.go
Adds TestSubstituteContainerEnvScoping, TestSubstituteRemoteEnvScoping, TestSubstituteMixedScoping, and TestRestrictedReplacePreservesContainerVars; includes assertContainerEnv and assertRemoteEnv helpers.
Documentation & E2E Tests
pkg/devcontainer/config.go, e2e/tests/up/varsub_scope.go, e2e/tests/up/testdata/docker-varsub-scope/.devcontainer.json
Adds documentation comment on phase-aware scoping semantics; introduces end-to-end test suite validating runtime vs. substitution-time resolution across container and remote environments.

Sequence Diagram

sequenceDiagram
    participant Caller as Caller
    participant Substitute as Substitute()
    participant PreReplace as restrictedReplace
    participant FullReplace as fullReplace
    participant Merge as Merge Results

    Caller->>Substitute: Call Substitute(config, context)
    activate Substitute
    Substitute->>PreReplace: First pass: preContainerFields with restricted replacer
    activate PreReplace
    PreReplace-->>Substitute: Returns containerEnv (preserved as literals)
    deactivate PreReplace
    Substitute->>FullReplace: Second pass: all fields with full replacer
    activate FullReplace
    FullReplace-->>Substitute: Returns fully substituted config
    deactivate FullReplace
    Substitute->>Merge: Merge pre-container results back into full results
    activate Merge
    Merge-->>Substitute: Final config with correct scoping
    deactivate Merge
    Substitute-->>Caller: Return substituted config
    deactivate Substitute
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: enforcing phase-aware variable substitution scoping in the devcontainer config system, which is the core focus across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@skevetter
skevetter marked this pull request as ready for review May 6, 2026 17:19
@skevetter
skevetter enabled auto-merge (squash) May 6, 2026 17:19
@skevetter
skevetter merged commit 8f74302 into main May 6, 2026
54 checks passed
@skevetter
skevetter deleted the ce37-819c-ws-a-varsub-scope branch May 6, 2026 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant