fix(feature): match devcontainer CLI lockfile dependsOn format - #793
Conversation
The lockfile generator emitted `dependsOn` as a JSON object mapping feature IDs to empty option objects. The reference devcontainer CLI emits it as an array of feature IDs in declaration order (`Object.keys(dependsOn)`). Retype LockedFeature.DependsOn to []string and preserve the JSON declaration order of dependsOn keys via a custom FeatureConfig unmarshaler, so generated lockfiles are byte-identical to the CLI. Signed-off-by: Samuel K <skevetter@pm.me>
✅ Deploy Preview for devsydev canceled.
|
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughFeature dependency keys are captured in JSON declaration order, with sorted fallback for programmatic configs. Lockfile entries now store dependencies as string arrays, and lockfile generation uses the ordered keys. Tests cover ordering, fallbacks, empty configs, and serialization. ChangesFeature dependency lockfile representation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant FeatureConfig
participant recordLockEntry
participant LockedFeature
participant WriteLockfile
FeatureConfig->>recordLockEntry: Provide DependsOnKeys()
recordLockEntry->>LockedFeature: Assign dependency string array
LockedFeature->>WriteLockfile: Serialize dependsOn array
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
✅ Deploy Preview for images-devsy-sh canceled.
|
Signed-off-by: Samuel K <skevetter@pm.me>
![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 `@pkg/devcontainer/config/feature.go`:
- Around line 153-154: Update the depends-on ordering logic around
dependsOnOrder so it retains the captured declaration order only when every
captured key still exists in DependsOn, not merely when the lengths match;
otherwise return the existing sorted fallback. Add a regression test that
unmarshals configuration, replaces one dependency key without changing the map
size, and verifies the fallback order is used.
In `@pkg/devcontainer/feature/lockfile.go`:
- Around line 19-22: Update ReadLockfile to decode persisted feature entries
through a flexible JSON representation before constructing LockedFeature.
Normalize legacy object-shaped dependsOn values into the []string form, while
preserving existing array-shaped values, so newLockfileState receives the
canonical representation used for pinning and writing.
🪄 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 Plus
Run ID: 871bb23d-41c2-412c-9496-d8abcbac7456
📒 Files selected for processing (5)
pkg/devcontainer/config/feature.gopkg/devcontainer/config/feature_test.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/lockfile.gopkg/devcontainer/feature/lockfile_test.go
| Version string `json:"version,omitempty"` | ||
| Resolved string `json:"resolved,omitempty"` | ||
| Integrity string `json:"integrity,omitempty"` | ||
| DependsOn []string `json:"dependsOn,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate lockfile readers and JSON decoding paths involving Lockfile types.
ast-grep outline pkg/devcontainer/feature/lockfile.go --items all
ast-grep run --lang go --pattern 'json.Unmarshal($DATA, $TARGET)' pkg
rg -nP --type=go -C3 '\b(?:Lockfile|LockedFeature)\b|json\.NewDecoder\(' pkgRepository: devsy-org/devsy
Length of output: 35212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile.go =="
sed -n '1,260p' pkg/devcontainer/feature/lockfile.go
echo "== call sites for newLockfileState / ReadLockfile / lockfileState =="
rg -n -C 4 'newLockfileState\(|ReadLockfile\(|\.loaded|checkFrozenPrecondition|lockfileMode|needsWrite|lockfileNeedsWrite' pkg/devcontainer pkg -g '*.go' | head -n 200
echo "== inspect devcontainer feature/extend relevant =="
sed -n '480,545p' pkg/devcontainer/feature/extend.goRepository: devsy-org/devsy
Length of output: 21450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go JSON Unmarshal legacy object as []string behavior =="
go version 2>/dev/null || true
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/unmarshal_probe.go" <<'GO'
package main
import (
"encoding/json"
"fmt"
)
type LockedFeature struct {
Version string `json:"version,omitempty"`
Resolved string `json:"resolved,omitempty"`
Integrity string `json:"integrity,omitempty"`
DependsOn []string `json:"dependsOn,omitempty"`
}
type Lockfile struct {
Features map[string]LockedFeature `json:"features"`
}
func main() {
legacy := []byte(`{
"features": {
"ghcr.io/a/f:1": {"resolved": "sha256:00", "dependsOn": "nope"}
}
}`)
var lf Lockfile
err := json.Unmarshal(legacy, &lf)
fmt.Printf("legacy-object-string err=%v lf=%#v\n", err, lf)
legacyMap := []byte(`{
"features": {
"ghcr.io/a/f:1": {"resolved": "sha256:00", "dependsOn": {"dep": true}}
}
}`)
err = json.Unmarshal(legacyMap, &lf)
fmt.Printf("legacy-map err=%v lf=%#v\n", err, lf)
current := []byte(`{
"features": {
"ghcr.io/a/f:1": {"resolved": "sha256:00", "dependsOn": ["dep"]}
}
}`)
err = json.Unmarshal(current, &lf)
fmt.Printf("current-array err=%v lf=%#v\n", err, lf)
}
GO
go run "$tmpdir/unmarshal_probe.go"
echo "== inspect config DependsOn type for feature config keys =="
sed -n '1,60p' pkg/devcontainer/config/feature.go
rg -n -C 3 'DependsOn[A-Za-z0-9_]*|DependsOn' pkg/devcontainer/config pkg/devcontainer/feature/lockfile.go pkg/devcontainer/feature/extend.goRepository: devsy-org/devsy
Length of output: 11881
Normalize legacy dependsOn on lockfile read.
ReadLockfile unmarshals persisted files into LockedFeature, whose DependsOn is now []string; object-shaped dependsOn entries fail to parse during fetch resolution (via newLockfileState). Decode lockfile entries as flexible JSON first/normatively, then normalize to the array shape used for pinning and writing.
🤖 Prompt for 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.
In `@pkg/devcontainer/feature/lockfile.go` around lines 19 - 22, Update
ReadLockfile to decode persisted feature entries through a flexible JSON
representation before constructing LockedFeature. Normalize legacy object-shaped
dependsOn values into the []string form, while preserving existing array-shaped
values, so newLockfileState receives the canonical representation used for
pinning and writing.
Address review feedback: - DependsOnKeys now retains captured declaration order only when every captured key still exists in DependsOn, guarding against post-unmarshal mutation that keeps the map size unchanged; otherwise it uses the sorted fallback. - LockedFeature.UnmarshalJSON normalizes legacy object-shaped dependsOn into the []string form, so pre-fix lockfiles still load and pin correctly. Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
1 participant
Summary
The devcontainer lockfile generator emitted the
dependsOnfield as a JSON object mapping feature IDs to empty option objects:The reference devcontainers CLI (
generateLockfile) emits it as an array of feature IDs in JSON declaration order (Object.keys(dependsOn)):This change makes devsy's generated
devcontainer-lock.jsonbyte-identical to the CLI.Changes
LockedFeature.DependsOnretyped frommap[string]anyto[]string.FeatureConfiggains a customUnmarshalJSONthat captures the declaration order ofdependsOnkeys (a Go map cannot preserve it), exposed viaDependsOnKeys(). Falls back to sorted keys for programmatically-built configs so output is always deterministic.recordLockEntrynow writescfg.DependsOnKeys()directly.The regenerated repo lockfile (array format) ships separately on the devcontainer-features branch (#792).
Summary by CodeRabbit
Bug Fixes
dependsOnas an array, matching expected devcontainer format.Tests