iframe-proxy

bilby91 · 2026-05-13T13:21:30Z

Summary

Fix #52: feature-contributed mounts (and env, lifecycle commands, …) now get host-context substitution before reaching Docker.

Previously ResolveBytes only substituted variables in the user's devcontainer.json. Fields folded in afterwards by MergeMetadata from feature / base-image metadata kept their literal \${devcontainerId}, \${localWorkspaceFolder}, \${localEnv:*} tokens and flowed straight into ContainerCreate. The docker-in-docker feature's dind-var-lib-docker-\${devcontainerId} volume mount tripped Docker's volume name validator.

MergeMetadata now takes a SubstitutionContext and resolves layer-contributed strings before folding them in. Substitution copies — input layers are not mutated, so cached feature metadata stays clean. Call sites (applyMetadataMerge in up.go, Attach) build the context from cfg + localEnv.

Test plan

Unit: TestMergeMetadata_SubstitutesLayerStrings covers mount sources, env values, and lifecycle commands with \${devcontainerId} / \${localWorkspaceFolder} / \${localEnv:*}
Unit: TestMergeMetadata_SubstitutionDoesNotMutateLayers pins the no-input-mutation invariant (feature metadata is often shared / cached)
Integration (build tag integration, requires Docker): TestFeatureMountSubstitution reproduces the docker-in-docker mount shape with a local feature, asserts Up succeeds and the inspected container mount path embeds the substituted volume name

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Metadata substitution now consistently resolves placeholders like ${devcontainerId}, ${localEnv:...}, and ${localWorkspaceFolder} in mounts, env vars, and lifecycle commands so feature-provided values are correctly applied at workspace startup.
Tests
- Added/updated unit and integration tests to verify substitution behavior and prevent regressions.

Feature-supplied mounts, env, and lifecycle commands now get host-context substitution. Previously only the user's devcontainer.json fields were resolved by ResolveBytes; feature/base-image metadata folded in by MergeMetadata kept their literal ${devcontainerId}, ${localWorkspaceFolder}, ${localEnv:*} tokens and reached ContainerCreate unchanged, causing Docker to reject mounts like dind-var-lib-docker-${devcontainerId} as having invalid volume name characters. MergeMetadata now accepts a SubstitutionContext and resolves layer- contributed strings before folding them into cfg. Callers (Up's applyMetadataMerge, Attach) build the context from cfg + localEnv. Fixes #52. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-05-13T13:21:43Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 646581c9-7dfa-443a-80b8-a14e1bff46dc

📥 Commits

Reviewing files that changed from the base of the PR and between 9d4310e and e3afe20.

📒 Files selected for processing (3)

config/parse_test.go
config/resolve_test.go
eventbus_test.go

✅ Files skipped from review due to trivial changes (3)

eventbus_test.go
config/resolve_test.go
config/parse_test.go

📝 Walkthrough

Walkthrough

Adds placeholder substitution during MergeMetadata by accepting a SubstitutionContext, substituting layer string fields before merging, and threading localEnv through attach/up. Validated by updated unit tests and an integration test.

Changes

Feature mount variable substitution

Layer / File(s)	Summary
Substitution infrastructure in MergeMetadata `config/merge_metadata.go`	`MergeMetadata` signature updated to accept `SubstitutionContext` parameter; new `substituteLayers`, `substituteLayer`, and `substituteLifecycleCommand` helpers resolve placeholders in user/container names, env maps, mounts (source/target), capability/security slices, and lifecycle commands before merge logic proceeds.
Wiring localEnv through orchestration pipeline `attach.go`, `up.go`	`localEnv` is computed early from `opts.LocalEnv` or `os.Environ()` in `attach.AttachWith`; `up.applyMetadataMerge` signature updated to accept `localEnv` map and builds `SubstitutionContext` with `LocalEnv` field; all three call sites (`attachExisting`, `createFresh`, `createFreshCompose`) pass `opts.LocalEnv` to enable metadata merge substitution.
Unit tests for metadata merge and substitution `config/merge_metadata_test.go`	All existing `MergeMetadata` tests updated to pass empty `SubstitutionContext{}`; two new tests verify that `${devcontainerId}` and `${localEnv:USER}` placeholders are correctly substituted in mounts, environment variables, and lifecycle hook commands, and that input layer slices are not mutated.
Integration test for feature mount substitution `test/integration/feature_mount_substitution_test.go`	New test `TestFeatureMountSubstitution` reproduces issue `#52`: provisions a workspace with a local feature declaring a named volume using `${devcontainerId}` in the mount source, starts the container, and verifies the placeholder is resolved to the expected `dc-feat-vol-<workspaceID>` substring with no remaining `${` markers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Feature-contributed mounts skip variable substitution — ${devcontainerId} reaches Docker literally #52: This PR implements the fix for feature mount source template substitution by adding SubstitutionContext parameter to MergeMetadata and threading LocalEnv through orchestration.

Possibly related PRs

crunchloop/devcontainer#21: Both PRs modify the config.MergeMetadata pipeline; #21 introduced the core merge logic, and this PR extends it with placeholder substitution support via SubstitutionContext and LocalEnv threading.

Poem

🐰 A rabbit sees placeholders dance,
${devcontainerId} gets its chance,
SubstitutionContext hops through the map,
LocalEnv helps close the gap,
Mounts blossom — no ${ in sight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/feature-mount-substitution

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

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

up.go (1)
263-277: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize localEnv before metadata substitution in Up paths.

applyMetadataMerge currently uses opts.LocalEnv as-is. When nil, ${localEnv:*} from feature/base metadata will not resolve from host env, which conflicts with the UpOptions.LocalEnv contract (nil => use os.Environ()) and differs from AttachWith behavior.
Suggested fix
func applyMetadataMerge(cfg *config.ResolvedConfig, baseLayers []config.FeatureMetadata, localEnv map[string]string) {
+	if localEnv == nil {
+		localEnv = environAsMap(os.Environ())
+	}
 	chain := make([]config.FeatureMetadata, 0, len(baseLayers)+len(cfg.Features))
 	chain = append(chain, baseLayers...)
 	for _, f := range cfg.Features {
 		if f.AlreadyInstalled {
 			continue
 		}
 		chain = append(chain, f.Metadata)
 	}
 	subCtx := config.SubstitutionContext{
 		LocalWorkspaceFolder:     cfg.LocalWorkspaceFolder,
 		ContainerWorkspaceFolder: cfg.ContainerWorkspaceFolder,
 		DevcontainerID:           cfg.DevcontainerID,
 		LocalEnv:                 localEnv,
 	}
 	config.MergeMetadata(cfg, subCtx, chain)
 	cfg.Finalize()
}
Also applies to: 475-475, 863-879

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f31bd3dd-e0b5-473a-af10-4790707b6cba

📥 Commits

Reviewing files that changed from the base of the PR and between 6db715d and 9d4310e.

📒 Files selected for processing (5)

attach.go
config/merge_metadata.go
config/merge_metadata_test.go
test/integration/feature_mount_substitution_test.go
up.go

After `if x == nil { t.Fatal(...) }`, staticcheck doesn't model t.Fatal/t.Fatalf as terminating, so subsequent dereferences trip SA5011. Add an explicit return so the analyzer sees the post-nil code as unreachable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai Bot reviewed May 13, 2026

View reviewed changes

coderabbitai Bot approved these changes May 13, 2026

View reviewed changes

bilby91 merged commit 42aa6d6 into main May 13, 2026
5 checks passed

bilby91 mentioned this pull request May 13, 2026

docs: add v0.1.3 changelog entry #54

Merged

bilby91 deleted the fix/feature-mount-substitution branch May 14, 2026 13:04

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

config: substitute variables in feature-contributed metadata#53

config: substitute variables in feature-contributed metadata#53
bilby91 merged 2 commits into
mainfrom
fix/feature-mount-substitution

bilby91 commented May 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 13, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

bilby91 commented May 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bilby91 commented May 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 13, 2026 •

edited

Loading