feat(devcontainers): implement features package for OCI and local resolution#319
Merged
feloy merged 7 commits intoopenkaiden:mainfrom Apr 23, 2026
Merged
feat(devcontainers): implement features package for OCI and local resolution#319feloy merged 7 commits intoopenkaiden:mainfrom
feloy merged 7 commits intoopenkaiden:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
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 53 minutes and 12 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 (12)
📝 WalkthroughWalkthroughAdds a new package Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FromMap
participant Feature as Feature (local / OCI)
participant OCI as OCI Registry
participant FS as FileSystem
participant Meta as Metadata Parser
participant Order
Client->>FromMap: FromMap(featuresMap, workspaceDir)
FromMap-->>Client: []Feature, userOptions
loop parallel per feature
Client->>Feature: Download(ctx, destDir)
alt local feature
Feature->>FS: Resolve & copy workspace path -> destDir
else OCI feature
Feature->>OCI: Parse ref, fetch manifest (401→auth challenge)
OCI-->>Feature: Bearer token (if needed)
Feature->>OCI: Download layer blobs
Feature->>FS: Extract tar/gzip -> destDir (sanitize paths, reject symlinks)
end
FS->>Meta: Read devcontainer-feature.json
Meta-->>Feature: FeatureMetadata
Feature-->>Client: FeatureMetadata
end
Client->>Order: Order(feats, metadata)
Order->>Order: Build graph from InstallsAfter (strip versions)
Order->>Order: Kahn topological sort (stable)
Order-->>Client: ordered []Feature
loop for ordered features
Client->>Meta: metadata[feat.ID()].Options().Merge(userOptions)
Meta-->>Client: normalized options map
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/working-with-devcontainers/SKILL.md:
- Around line 87-90: The fenced code block showing mappings for "install-tools"
→ "INSTALL_TOOLS" and "go.version" → "GO_VERSION" is missing a language tag;
update that fenced block to include the text language tag (use ```text) so the
block becomes a ```text fenced block containing the two lines referencing
"install-tools" / INSTALL_TOOLS and "go.version" / GO_VERSION, satisfying the
markdown guideline that all fenced code blocks include a language tag.
In `@pkg/devcontainers/features/features_test.go`:
- Around line 397-413: The test TestOrder_InstallsAfterExternalFeatureIgnored
assumes that an installsAfter reference to a feature not present in the input
list is ignored, but the spec/PR requires missing dependency references to cause
an error; update the behavior and test accordingly: modify the implementation of
features.Order to validate each FeatureMetadata.installsAfter entry (and
similarly installsBefore if present) and return an error when any referenced
feature ID is not present in the provided feats list, and then change
TestOrder_InstallsAfterExternalFeatureIgnored to expect an error from
features.Order (use the fakeMetadata.installsAfter reference as-is) rather than
a successful ordering; ensure the error message clearly identifies the missing
dependency and surface it from Order so the test can assert non-nil err.
In `@pkg/devcontainers/features/features.go`:
- Around line 76-77: The error branch that currently uses strings.HasPrefix(id,
"http://") || strings.HasPrefix(id, "https://") mislabels any HTTP(S) input as a
"Tgz URI"; update the check and message: change the condition to only treat
actual .tgz archives (e.g., strings.HasSuffix(id, ".tgz")) and/or revise the
fmt.Errorf call so it says that direct HTTP(S) feature URIs are not supported
(reference the id variable, the strings.HasPrefix/HasSuffix check, and the
fmt.Errorf call in that case branch). Ensure the branch behavior matches the new
check so plain HTTP URLs get the correct message and true .tgz URIs are handled
appropriately.
In `@pkg/devcontainers/features/local.go`:
- Around line 54-89: The copy logic currently follows symlinks (WalkDir treats
them as non-directories and copyFile opens targets), allowing files outside the
feature tree to be copied; update copyDir and copyFile to explicitly detect and
skip symlinks: inside copyDir's WalkDirFunc check d.Type()&fs.ModeSymlink (or
call os.Lstat on path) and return nil to skip symlink entries, and in copyFile
re-check using os.Lstat(path) to refuse/skip if the source is a symlink before
calling os.Open; keep creating directories and copying regular files unchanged.
In `@pkg/devcontainers/features/metadata.go`:
- Around line 74-128: Defaults are currently written into result with
fmt.Sprintf and skip the type/enum checks; change the initial "Apply defaults
first" loop to validate and normalize spec.Default using the same logic as the
userOptions handling (use normalizeKey, o.specs, spec.Default, result) — i.e.,
for spec.Type == "boolean" accept bool or string "true"/"false" and write
"true"/"false", for "string" ensure the default is a string and validate against
spec.Enum if present, and return an error if the default is invalid; factor or
reuse the switch used for userOptions to avoid duplication so invalid defaults
fail early instead of being silently formatted.
In `@pkg/devcontainers/features/oci.go`:
- Around line 316-346: The extraction currently trusts tar.TypeSymlink entries
and later file writes can follow those symlinks (hdr.Name, target, os.Symlink,
os.OpenFile), allowing escapes from destDir; reject or validate symlinks by: for
tar.TypeSymlink, disallow absolute Linkname or compute resolved :=
filepath.Clean(filepath.Join(filepath.Dir(target), hdr.Linkname)) and ensure
strings.HasPrefix(resolved, filepath.Clean(destDir)+string(os.PathSeparator));
if it fails, return an error instead of creating the symlink; additionally,
before writing regular files (tar.TypeReg) call os.Lstat on target (and/or each
path component) and reject if any component is a symlink to prevent following
filesystem symlinks when creating the file.
🪄 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: 388187a1-5abf-4bc9-bb2e-46d1b7278204
📒 Files selected for processing (9)
.agents/skills/working-with-devcontainers/SKILL.mdAGENTS.mdMakefilepkg/devcontainers/features/features.gopkg/devcontainers/features/features_test.gopkg/devcontainers/features/local.gopkg/devcontainers/features/metadata.gopkg/devcontainers/features/oci.gopkg/devcontainers/features/oci_integration_test.go
serbangeorge-m
approved these changes
Apr 22, 2026
Contributor
serbangeorge-m
left a comment
serbangeorge-m
left a comment
There was a problem hiding this comment.
changes related to integration tests look ok
feloy
added a commit
to feloy/kdn
that referenced
this pull request
Apr 22, 2026
…iden#319 - Add text language tag to fenced code block in SKILL.md - Clarify HTTP(S) feature source error message in FromMap - Update Order docstring: installsAfter soft-ignore is intentional per spec - Reject symlinks and non-regular files in local feature copyDir - Validate defaults with same type/enum rules as user options in Merge - Replace symlink creation with error in extractTarEntries; add safeTarTarget helper for robust path-containment checks - Update oci_test.go to expect error for symlink tar entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/devcontainers/features/features_test.go (1)
604-625:⚠️ Potential issue | 🟠 MajorAlign the missing-dependency behavior with the PR objective.
This test codifies ignoring an
installsAfterreference that is not infeats, but the linked objectives sayOrdershould error on missing references. Either updateOrderand this test to expect an error, or update the objective/spec docs if soft-ignore is intentional.Proposed test expectation if missing references must error
-func TestOrder_InstallsAfterExternalFeatureIgnored(t *testing.T) { +func TestOrder_MissingInstallsAfterDependencyReturnsError(t *testing.T) { t.Parallel() - // Per the Dev Container spec, installsAfter is a soft ordering hint. - // References to features not present in feats (e.g. features from a - // different layer or not selected by the user) must be silently ignored, - // not treated as errors. This is distinct from a missing metadata entry - // (which is always an error because it means a feature in feats was not - // downloaded before Order was called). feats := []features.Feature{&fakeFeature{id: "A"}} metadata := map[string]features.FeatureMetadata{ "A": &fakeMetadata{installsAfter: []string{"external-feature"}}, } - ordered, err := features.Order(feats, metadata) - if err != nil { - t.Fatalf("Order: %v", err) - } - if len(ordered) != 1 || ordered[0].ID() != "A" { - t.Errorf("ordered = %v, want [A]", idsOf(ordered)) + _, err := features.Order(feats, metadata) + if err == nil { + t.Fatal("expected error for missing installsAfter dependency, got nil") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainers/features/features_test.go` around lines 604 - 625, The test TestOrder_InstallsAfterExternalFeatureIgnored currently expects Order to silently ignore an installsAfter reference not present in feats, but the PR objective states features.Order should error on missing references; update the test to assert that features.Order returns a non-nil error for this case (and optionally that the error message mentions the missing dependency like "external-feature") instead of succeeding, using the existing feats slice and metadata (fakeFeature / fakeMetadata) to trigger the missing-reference path; ensure the test checks err != nil and fails if Order returns nil so it aligns with the intended behavior of features.Order.pkg/devcontainers/features/local.go (1)
83-97:⚠️ Potential issue | 🟠 MajorRe-check the source before opening and handle file close errors.
WalkDirrejects symlink entries, butcopyFilestill openssrcwithout anLstatre-check, and both deferred closes are flagged byerrcheck. Re-check regular-file status immediately before opening and propagate write-side close errors.Proposed fix
-func copyFile(src, dst string) error { +func copyFile(src, dst string) (err error) { + info, err := os.Lstat(src) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("symlinks are not supported in local features: %s", src) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("unsupported non-regular file in local feature: %s", src) + } + in, err := os.Open(src) if err != nil { return err } - defer in.Close() + defer func() { + if closeErr := in.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) if err != nil { return err } - defer out.Close() + defer func() { + if closeErr := out.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() _, err = io.Copy(out, in) return err }Run the lint target to confirm
errcheckis clean:#!/bin/bash # Description: Verify static checks after handling Close errors. make vet🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainers/features/local.go` around lines 83 - 97, In copyFile, re-check the source file with os.Lstat(src) and ensure it's a regular file before opening (so symlinks/rejected WalkDir entries are caught) and update the function to capture and propagate Close errors instead of ignoring them: replace simple defers for in.Close() and out.Close() with deferred closures that check the returned error (e.g., if cerr := in.Close(); err == nil { err = cerr }) and do the same for out.Close() (making sure you preserve and return any earlier copy error but prefer to return a close error if it occurs), keeping the io.Copy(out, in) logic and variable names (src, dst, in, out) intact.
🤖 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/devcontainers/features/local.go`:
- Around line 40-42: The Download method in localFeature builds srcDir with
filepath.Join which can remain relative; update localFeature.Download to convert
the joined/cleaned path to an absolute host path by calling filepath.Abs on
srcDir (after computing srcDir := filepath.Clean(filepath.Join(f.dir,
filepath.FromSlash(f.id)))); handle the error from filepath.Abs and return it if
non-nil, then use the absolute path for the subsequent copy/reading operations
so the feature source cannot change based on cwd (referencing
localFeature.Download, srcDir, and filepath.Abs).
In `@pkg/devcontainers/features/oci_integration_test.go`:
- Around line 53-54: Tests currently call feats[i].Download with
context.Background(), which can hang; replace those calls by creating a bounded
context via context.WithTimeout (e.g., 30s) and pass that ctx into every
Download call (for the occurrences around Download in oci_integration_test.go at
the spots using feats[0].Download and the other mentioned locations), remember
to call cancel() via defer immediately after creating the context to avoid
leaks, and propagate the new ctx into all Download invocations in the test.
In `@pkg/devcontainers/features/oci.go`:
- Around line 250-272: In downloadAndExtractLayer, compute and verify the
returned blob's digest (expected "sha256:<hex>" from the digest param) before
extracting: stream the response into a SHA-256 hasher while writing to a
temporary file or buffering (e.g., use io.TeeReader or write to tmp file),
compare hex(sum) prefixed with "sha256:" to the digest parameter, return an
error on mismatch, and only call extractTar (or a modified extractTar that reads
from the verified temp file) after verification; update downloadAndExtractLayer
and extractTar usage accordingly to ensure extraction happens from a verified
blob and avoid extracting on hash failure.
- Around line 141-162: The code currently calls resp.Body.Close(), gzip reader
Close, and file Close without handling returned errors which trips errcheck;
update all such calls: for response bodies (resp) you can explicitly discard
errors with `_ = resp.Body.Close()` where safe (e.g., after handling the error
branch in the manifest fetch block around the `resp` variable and similar blocks
at the other resp closes), but for resources where buffered writes may fail (the
gzip reader variable, e.g., `gz`, and file handles used for writing, e.g.,
`dstFile`/`f`) must check the Close() error and propagate it (return wrapped
fmt.Errorf or `err` up) — update Close usage in the functions that perform gzip
extraction and file writes (the gzip reader close near the creation of the gzip
reader and the file Close calls after writing) to capture and handle errors
instead of ignoring them.
---
Duplicate comments:
In `@pkg/devcontainers/features/features_test.go`:
- Around line 604-625: The test TestOrder_InstallsAfterExternalFeatureIgnored
currently expects Order to silently ignore an installsAfter reference not
present in feats, but the PR objective states features.Order should error on
missing references; update the test to assert that features.Order returns a
non-nil error for this case (and optionally that the error message mentions the
missing dependency like "external-feature") instead of succeeding, using the
existing feats slice and metadata (fakeFeature / fakeMetadata) to trigger the
missing-reference path; ensure the test checks err != nil and fails if Order
returns nil so it aligns with the intended behavior of features.Order.
In `@pkg/devcontainers/features/local.go`:
- Around line 83-97: In copyFile, re-check the source file with os.Lstat(src)
and ensure it's a regular file before opening (so symlinks/rejected WalkDir
entries are caught) and update the function to capture and propagate Close
errors instead of ignoring them: replace simple defers for in.Close() and
out.Close() with deferred closures that check the returned error (e.g., if cerr
:= in.Close(); err == nil { err = cerr }) and do the same for out.Close()
(making sure you preserve and return any earlier copy error but prefer to return
a close error if it occurs), keeping the io.Copy(out, in) logic and variable
names (src, dst, in, out) intact.
🪄 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: 6fefdb36-be9e-441a-a5e4-a452b94e1c43
📒 Files selected for processing (10)
.agents/skills/working-with-devcontainers/SKILL.mdAGENTS.mdMakefilepkg/devcontainers/features/features.gopkg/devcontainers/features/features_test.gopkg/devcontainers/features/local.gopkg/devcontainers/features/metadata.gopkg/devcontainers/features/oci.gopkg/devcontainers/features/oci_integration_test.gopkg/devcontainers/features/oci_test.go
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/working-with-devcontainers/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- pkg/devcontainers/features/features.go
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
pkg/devcontainers/features/features_test.go (1)
606-627:⚠️ Potential issue | 🟠 MajorAlign this test with the missing-reference requirement.
The PR objective says
Ordershould error on missing references, but this test locks in silently ignoring aninstallsAftertarget that is absent fromfeats. If soft-ignore is intentional, update the objective/spec; otherwise expect an error here and updateOrderaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainers/features/features_test.go` around lines 606 - 627, The test TestOrder_InstallsAfterExternalFeatureIgnored currently asserts that features.Order silently ignores an installsAfter reference to "external-feature" even when that feature is not present in feats; update the test to reflect the PR objective that Order should error on missing references by calling features.Order(feats, metadata) and asserting that err is non-nil (and optionally that the error message mentions the missing installsAfter target or missing metadata), and adjust assertions so ordered is not considered successful; keep references to the test name, the metadata map using fakeMetadata with installsAfter, and the features.Order function to locate the change.
🤖 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/devcontainers/features/features_test.go`:
- Around line 288-295: The test checks error contents after calling
meta.Options().Merge but uses t.Error before accessing err.Error(), which can
lead to a nil-panic; change those failing-path assertions to call t.Fatal (or
t.Fatalf) immediately when err == nil so the test stops before any err.Error()
usage—update occurrences around meta.Options().Merge(...) where the pattern is
`if err == nil { t.Error(...) } if !strings.Contains(err.Error(), "...") { ...
}` to instead `if err == nil { t.Fatal(...) }` (and use t.Fatalf when formatting
the message) for the blocks at the shown location and the other similar blocks
(around lines noted in the review).
- Around line 676-689: The test currently ignores errors from tar/gzip Close and
response writes which can hide failures; update the test to check and handle
errors returned by tw.Close(), tw2.Close(), gw.Close() and any w.Write(...) used
by the HTTP handler (or explicitly use io.Copy to io.Discard and check its
error) so failures surface and the produced layer bytes are valid; locate the
creation/closing of tw, tw2 and gw in features_test.go and the HTTP handler
write path and add error checks (fail the test with t.Fatalf or t.Fatal on
non-nil errors) after each Close and after writing the response body.
In `@pkg/devcontainers/features/local_test.go`:
- Around line 46-51: The test currently calls t.Error("expected error...") then
proceeds to use err.Error(), which will panic if err is nil; change the failing
assertions to t.Fatal (e.g., replace t.Error("expected error for non-existent
source directory, got nil") with t.Fatal(...)) so the test stops immediately
when err is nil before any err.Error() call, and make the same change for the
second occurrence around the block that starts at the other check (lines
referenced by the second occurrence around feats[0].Download and the subsequent
err.Error() usage).
- Around line 93-95: The test currently hardcodes "/etc/hosts" when creating a
symlink (os.Symlink(..., filepath.Join(featureDir, "z-link"))), which breaks
cross-platform runs; instead create a real temporary target inside the test's
temp workspace (use t.TempDir() or the existing workspaceDir), join its name
with filepath.Join to produce the target path, and call os.Symlink(targetPath,
filepath.Join(featureDir, "z-link")); ensure you use filepath.Join for the host
filesystem path and avoid any literal "/etc/hosts" strings so the test is
cross-platform and self-contained.
In `@pkg/devcontainers/features/local_unix_test.go`:
- Around line 62-67: Test currently calls feats[0].Download and uses t.Error on
nil err, which allows execution to continue and then calls err.Error() causing a
panic; change the failure at the check for nil error to t.Fatal so the test
stops immediately when Download unexpectedly succeeds, i.e., replace the
t.Error("expected error...") with t.Fatal("expected error...") in the test that
calls feats[0].Download to avoid calling err.Error() when err is nil.
In `@pkg/devcontainers/features/local.go`:
- Around line 86-100: The copyFile function currently defers in.Close() and
out.Close(), which hides close errors (especially delayed write errors from
out.Close()); change it to explicitly close files after io.Copy and propagate
any close errors: after calling io.Copy(out, in) capture its error into err,
then call closeErr := out.Close() and if err == nil set err = closeErr, then
call inCloseErr := in.Close() and if err == nil set err = inCloseErr; return
err. Use the existing symbols copyFile, in, out and io.Copy to locate and update
the cleanup so close failures are not ignored.
In `@pkg/devcontainers/features/oci_test.go`:
- Around line 574-579: The test calls extractTar and then uses t.Error before
inspecting err.Error(), which can dereference nil; change the failure handling
to fail fast by using t.Fatal (or t.Fatalf) when err is nil, e.g., in the test
around the call to extractTar(...) and the subsequent checks replace
t.Error("expected error...") with a t.Fatal or t.Fatalf so the test stops if err
is nil, then safely proceed to assert the error string contains "creating gzip
reader" from err.Error(); reference the extractTar call and the err variable in
oci_test.go when making this change.
- Around line 189-192: The httptest handler's response writes (e.g.,
fmt.Fprint/w.Write/io.WriteString in the srv handler closure) should explicitly
discard errors when the write failure is non-actionable (use _ = ...) and all
tar/gzip finalizing Close calls (e.g., calls on tar.NewWriter.Close and
gzip.NewWriter.Close in the tests) must have their returned errors checked and
handled (fail the test with t.Fatalf or t.Errorf) so the archive bytes are
guaranteed finalized; update each occurrence (handler writes near the srv
handler and every tar.Writer/gzip.Writer Close call) to either explicitly ignore
the error or check it and fail the test accordingly.
In `@pkg/devcontainers/features/oci.go`:
- Around line 280-286: The defer currently calls os.Remove(tmpPath) and ignores
its error which fails errcheck; update the cleanup to explicitly discard the
error so linters are satisfied — e.g., replace the plain defer
os.Remove(tmpPath) with an explicit discard wrapper (referencing tmpPath and
os.Remove) such as a deferred anonymous function that calls os.Remove(tmpPath)
and assigns its result to the blank identifier to explicitly ignore the error.
---
Duplicate comments:
In `@pkg/devcontainers/features/features_test.go`:
- Around line 606-627: The test TestOrder_InstallsAfterExternalFeatureIgnored
currently asserts that features.Order silently ignores an installsAfter
reference to "external-feature" even when that feature is not present in feats;
update the test to reflect the PR objective that Order should error on missing
references by calling features.Order(feats, metadata) and asserting that err is
non-nil (and optionally that the error message mentions the missing
installsAfter target or missing metadata), and adjust assertions so ordered is
not considered successful; keep references to the test name, the metadata map
using fakeMetadata with installsAfter, and the features.Order function to locate
the change.
🪄 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: 0363132d-d1fe-4bbe-ac6c-9f379e7bd08b
📒 Files selected for processing (6)
pkg/devcontainers/features/features_test.gopkg/devcontainers/features/local.gopkg/devcontainers/features/local_test.gopkg/devcontainers/features/local_unix_test.gopkg/devcontainers/features/oci.gopkg/devcontainers/features/oci_test.go
feloy
force-pushed
the
features-pkg
branch
2 times, most recently
from
9394774 to
7de9916
Compare
feloy
added a commit
to feloy/kdn
that referenced
this pull request
Apr 23, 2026
…iden#319 Production fixes: - copyFile uses named return to propagate Close errors (errcheck + delayed write errors from out.Close() now surface correctly) - defer os.Remove(tmpPath) explicitly discards error to satisfy errcheck Test fixes across features_test.go, oci_test.go, oci_integration_test.go, local_test.go, and local_unix_test.go: - t.Error → t.Fatal before err.Error() calls (prevent nil-panic) - tw/gw Close calls checked with t.Fatalf (archive bytes must be valid) - Handler fmt.Fprint/w.Write explicitly discarded with _, _ = (errcheck) - Symlink test target replaced from /etc/hosts with a temp file (cross-platform) - Integration tests use context.WithTimeout(2m) instead of context.Background() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…olution Implements pkg/devcontainers/features as specified in issue openkaiden#302: - Feature, FeatureMetadata, and FeatureOptions interfaces (all unexported impls) - OCI registry download: anonymous Bearer auth, manifest fetch, gzip/plain tar extraction, path-traversal sanitisation - Local file tree download for ./… and ../… IDs - FromMap classifies IDs and returns a deterministically sorted slice - Order runs Kahn's topological sort on installsAfter; matches versionless installsAfter values against versioned feature IDs by stripping the tag - Unit tests covering all acceptance criteria; integration tests against the real ghcr.io registry (build tag: integration) - /working-with-devcontainers skill with spec coverage table - Extend make test-integration target to ./... to include the new tests Closes openkaiden#302 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Add targeted tests to cover previously untested branches in metadata.go and oci.go, raising overall package coverage from 61.6% to 90.4%. - metadata.go: InstallsAfter(), all Merge() branches (unknown option, bool user option true/false, bool string coercion, invalid bool string, wrong type for boolean, unsupported option type), parseMetadata() invalid JSON - oci.go: parseOCIRef(), parseWWWAuthenticate(), fetchBearerToken(), fetchManifest() including 401 auth flow, extractTar() gzip/plain auto-detection, extractTarEntries() dir/symlink/traversal paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…iden#319 - Add text language tag to fenced code block in SKILL.md - Clarify HTTP(S) feature source error message in FromMap - Update Order docstring: installsAfter soft-ignore is intentional per spec - Reject symlinks and non-regular files in local feature copyDir - Validate defaults with same type/enum rules as user options in Merge - Replace symlink creation with error in extractTarEntries; add safeTarTarget helper for robust path-containment checks - Update oci_test.go to expect error for symlink tar entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…ners skill Per the Dev Container spec, installsAfter is a soft ordering hint — references to features not present in feats are silently ignored, not errors. This was already the implemented behavior but the SKILL.md and the test comment did not make the distinction explicit vs. missing metadata entries (which are always errors). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
- localFeature.Download: resolve srcDir to an absolute path via filepath.Abs to prevent cwd-relative path confusion - downloadAndExtractLayer: buffer blob to a temp file, verify its SHA-256 digest before extraction to detect corrupt/tampered blobs; reject unknown digest algorithms upfront - oci.go Close() calls: propagate gzip reader close errors via named return; check file close errors after writes; explicitly discard response body close errors with _ = to satisfy errcheck - safeTarTarget: allow "." (archive root entry) to map to destDir, fixing extraction of real OCI feature archives that include a ./ dir entry; reject absolute paths and ".." traversals as before - Update affected tests to supply real SHA-256 digests for test blobs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds tests for the previously uncovered error paths in local.go: - Download returns error when destDir exists as a regular file - Download wraps copyDir errors with "copying feature from" context - copyDir rejects symlinks and non-regular files (named pipes, etc.) - copyFile returns error on unreadable source or read-only destination - ../ relative paths resolve correctly against the workspace config dir - Feature directories with subdirectories are copied recursively Moves local.go coverage from ~70% to 90%+ across all three functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…iden#319 Production fixes: - copyFile uses named return to propagate Close errors (errcheck + delayed write errors from out.Close() now surface correctly) - defer os.Remove(tmpPath) explicitly discards error to satisfy errcheck Test fixes across features_test.go, oci_test.go, oci_integration_test.go, local_test.go, and local_unix_test.go: - t.Error → t.Fatal before err.Error() calls (prevent nil-panic) - tw/gw Close calls checked with t.Fatalf (archive bytes must be valid) - Handler fmt.Fprint/w.Write explicitly discarded with _, _ = (errcheck) - Symlink test target replaced from /etc/hosts with a temp file (cross-platform) - Integration tests use context.WithTimeout(2m) instead of context.Background() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
benoitf
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements pkg/devcontainers/features as specified in issue #302:
Closes #302
@serbangeorge-m I have added a few integration tests and modified the make test-integration command to cover them. Please check I didn't break anything
Can be tested on PR #323 which includes this PR