fix(devcontainer): keep compose metadata label valid JSON#771
fix(devcontainer): keep compose metadata label valid JSON#771YassineElbouchaibi wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCompose devcontainer label generation now escapes only dollar signs before writing ChangesCompose label escaping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
2789372 to
5586a66
Compare
Quote-heavy lifecycle commands (e.g. a postStartCommand using a single-quoted grep) caused the generated devcontainer.metadata image label to contain an illegal `\'` JSON escape, because every single quote in the label value was rewritten to `\'\'` before yaml.Marshal. On the next up/attach, GetImageMetadataFromContainer failed to unmarshal the label, swallowed the error, and returned empty metadata, so all customizations.vscode.extensions/settings were silently dropped on Compose-based devcontainers. yaml.Marshal already handles YAML quoting, so the manual single-quote escaping is unnecessary and corrupting. Extract the label escaping into escapeComposeLabelValue, which now only doubles `$` for compose interpolation, and add a regression test that round-trips a quote-heavy postStartCommand plus a vscode extension through the compose override. Fixes skevetter#770 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5586a66 to
a19ad04
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a Compose-only devcontainer bug where devcontainer.metadata labels could be corrupted into invalid JSON when lifecycle commands contained single quotes, causing VS Code customizations (extensions/settings) to be silently dropped on attach.
Changes:
- Extract label escaping into
escapeComposeLabelValue, removing the incorrect single-quote rewriting and keeping only$-doubling to prevent Docker Compose interpolation. - Add regression tests that round-trip quote-heavy metadata through YAML marshal/unmarshal + Compose interpolation and verify JSON validity and customization preservation.
- Add a focused test ensuring literal
$survives the Compose interpolation round-trip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/devcontainer/compose.go | Replaces the prior regex-based escaping (including broken single-quote escaping) with a dedicated helper that only escapes $ for Compose interpolation safety. |
| pkg/devcontainer/compose_label_escape_test.go | Adds regression coverage that reproduces the prior JSON corruption and verifies both quote preservation and $-escaping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #770.
On Docker Compose–based devcontainers,
customizations.vscode.extensions/settingswere silently dropped whenever a lifecycle command contained single quotes (e.g. apostStartCommandusing a single-quotedgrepregex).Root cause: when serializing additional labels into the generated docker-compose override file,
generateDockerComposeUpProjectrewrote every single quote'in the label value to\'\':That injects an illegal
\'escape into thedevcontainer.metadataJSON label. On the nextup/attach,GetImageMetadataFromContainer→json.Unmarshalfails with:The error is logged but swallowed, empty metadata is returned, and
MergeConfigurationdrops all customizations — so extensions/settings never get applied, while the container otherwise comes up fine.The single-quote rewrite is a mis-port of the devcontainers/cli reference, whose JS replacement
'\'\''evaluates to''(YAML single-quote doubling), not the literal characters\'\'. Because devpod writes the override file withyaml.Marshal(which already handles all YAML quoting), the manual single-quote escaping is both unnecessary and corrupting.Change
escapeComposeLabelValue, which now only doubles$so docker compose does not interpolate it ($$→$). Single quotes are left untouched and handled byyaml.Marshal.postStartCommandplus acustomizations.vscode.extensionsentry through the exact pipeline (escape →yaml.Marshal→ compose interpolation →json.Unmarshal) and asserts the customizations survive, plus a test confirming literal$is still preserved for compose.Verification
Before this change the new
TestEscapeComposeLabelValue_PreservesQuoteHeavyMetadatafails with the exactinvalid character '\'' in string escape codeerror; after, it passes and thegolang.goextension survives the round-trip.Notes / follow-up (out of scope here)
This PR fixes the corruption at the source. As defense-in-depth,
metadata.GetImageMetadataFromContainerstill silently swallows any future label-parse failure (logs and returns empty metadata). A follow-up could surface that loudly and/or fall back to the parseddevcontainer.jsoncustomizations. Kept separate to keep this change focused.🤖 Generated with Claude Code
Summary by CodeRabbit
$characters are preserved correctly in label values instead of being treated as variables.