WIP: add pkg-diff feature by dcermak · Pull Request #50 · dcermak/skiff

dcermak · 2026-02-26T09:57:05Z

No description provided.

Copilot

Pull request overview

Adds a new OCI image “resolver” that can read container filesystem contents directly from layer tars (without full extraction) and wires in an initial pkg_diff CLI command that uses Syft catalogers to enumerate packages from an image.

Changes:

Introduce OCIResolver (Syft file.Resolver implementation) that builds an in-memory path index from OCI layer tar streams, including basic whiteout handling and symlink resolution.
Add a new pkg_diff CLI command (currently prints packages from one image) and register it in the main CLI.
Add resolver tests plus a local test image Containerfile and Makefile target to build it; update Go deps to include Syft/Stereoscope/Doublestar.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`pkg/oci_resolver.go`	New `OCIResolver` implementation for indexing and resolving file locations/contents/metadata from OCI layers.
`pkg/oci_resolver_test.go`	Adds resolver tests (remote image fetch + local-image whiteout checks) and a `TestMain` that reexecs into a user namespace.
`pkg/TestImage.containerfile`	Defines a local image used for whiteout behavior testing.
`cmd/skiff/pkg-diff.go`	Adds `pkg_diff` command using Syft’s RedHat DB cataloger (currently only catalogs one image).
`cmd/skiff/main.go`	Registers the new `pkgDiffCommand`.
`Makefile`	Adds `test-image` target and makes `unit-tests` depend on it; expands tests to `./...`.
`go.mod`	Bumps Go version and adds Syft/Stereoscope/Doublestar + indirect dependency updates.
`go.sum`	Updates checksums for the new/updated module graph.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T10:03:03Z

cmd/skiff/pkg-diff.go

+var pkgDiffCommand = cli.Command{
+	Name:  "pkg_diff",
+	Usage: "Diff the packages of two container images",
+	Arguments: []cli.Argument{
+		&cli.StringArg{Name: "oldImage", UsageText: "Original Container Image ref"},
+		&cli.StringArg{Name: "newImage", UsageText: "New Container Image ref"},
+	},


Command/arg naming is inconsistent with existing commands (layers, top) which use lowercase names and simple argument keys (e.g., url, image). Consider renaming pkg_diff to pkg-diff (or similar) and using lowercase arg names (e.g., old, new) for consistency.

Copilot · 2026-02-26T10:03:03Z

cmd/skiff/pkg-diff.go

+	fmt.Printf("Found %d packages in %s\n", len(pkgs), uri)
+	for _, p := range pkgs {
+		fmt.Printf("- %s %s\n", p.Name, p.Version)
+	}


Other commands write output via the CLI’s writer (c.Writer) rather than directly to stdout. Using fmt.Printf here makes output harder to test and inconsistent with the rest of the CLI. Prefer writing to the command’s writer (and consider returning structured data from getPkgMetadataFromImg).

Copilot · 2026-02-26T10:03:03Z

pkg/oci_resolver_test.go

+func TestOCIResolver(t *testing.T) {
+	// Use a known stable image digest
+	// registry.suse.com/bci/bci-busybox@sha256:7c7d8d3ff8813e62ec46501b458c47cabf818867afd8ade209f729e181a3f7ea
+	uri := "docker://registry.suse.com/bci/bci-busybox@sha256:7c7d8d3ff8813e62ec46501b458c47cabf818867afd8ade209f729e181a3f7ea"
+
+	ctx := context.Background()
+	sysCtx := &types.SystemContext{}
+
+	img, _, err := ImageAndLayersFromURI(ctx, sysCtx, uri)
+	if err != nil {
+		t.Logf("Skipping test: unable to fetch image (network/auth issue?): %v", err)
+		t.SkipNow()
+	}


This test performs a live pull from an external registry. Even with the skip-on-error behavior, it introduces non-determinism and can hang/flake in CI due to network/auth/rate limits. Consider marking it as an integration test (build tag) or skipping when testing.Short() is set, and prefer a local fixture image for unit tests.

Copilot · 2026-02-26T10:03:04Z

pkg/oci_resolver.go

+		if strings.HasPrefix(file, ".wh.") {
+			whiteoutName := strings.TrimPrefix(file, ".wh.")
+			if whiteoutName == ".wh.opq" {
+				// Opaque whiteout: remove all siblings in this directory
+				// We need to find all entries in fsIndex that start with dir
+				for p := range r.fsIndex {


OCI opaque whiteout handling looks incorrect: OCI uses the special filename .wh..wh..opq to mark a directory as opaque, but this code checks whiteoutName == ".wh.opq" after trimming .wh., which won’t match. As a result, opaque directories won’t be applied correctly. Update the condition to detect the OCI opaque marker and apply the opaque semantics accordingly.

Copilot · 2026-02-26T10:03:04Z

pkg/oci_resolver.go

+			} else {
+				// Regular whiteout: remove specific file
+				target := filepath.Join(dir, whiteoutName)
+				delete(r.fsIndex, target)
+			}


Regular whiteouts can delete directories as well as files. Deleting only the exact target path leaves any previously-indexed children (e.g., /dir/file) still present, which is incorrect for a directory whiteout. Consider deleting all indexed paths with target as a prefix (and target itself) when the whiteout refers to a directory.

Copilot · 2026-02-26T10:03:05Z

cmd/skiff/pkg-diff.go

+	Action: func(ctx context.Context, c *cli.Command) error {
+		oldImage := c.StringArg("oldImage")
+		if oldImage == "" {
+			return fmt.Errorf("old image URL is required")
+		}
+		newImage := c.StringArg("newImage")
+		if newImage == "" {
+			return fmt.Errorf("newImage URL is required")
+		}
+
+		sysCtx := types.SystemContext{}
+		return getPkgMetadataFromImg(oldImage, ctx, &sysCtx)
+	},


This command collects newImage but never uses it, so pkg_diff doesn’t actually perform a diff despite its name/usage. Either implement the comparison between oldImage and newImage here or remove the unused argument until the diff logic is available.

Copilot · 2026-02-26T10:03:05Z

pkg/TestImage.containerfile

@@ -0,0 +1,9 @@
+FROM registry.opensuse.org/opensuse/leap:16.0
+
+RUN zypper -n in vim-small; zypper -n clean


Using RUN cmd1; cmd2 causes the layer’s exit status to be that of cmd2, so a failed zypper -n in vim-small can be masked if zypper -n clean succeeds. Use && (or set -e) so the build reliably fails when package installation fails.

Copilot · 2026-02-26T10:03:05Z

Makefile

+test-image:
+	pushd pkg; buildah bud --layers -t skiff-test-image -f TestImage.containerfile .; popd
+
 # Run unit tests
-unit-tests:
-	go test -v ./cmd/skiff/...
+unit-tests: test-image
+	go test -v ./...


This unit-tests target now requires buildah (via test-image) and runs go test ./..., which pulls in network/local-image dependent tests. That’s more of an integration test workflow and is likely to break in CI or on developer machines without buildah. Consider splitting into separate unit-tests (pure go test fast tests) and integration-tests (buildah + image-based tests).

Copilot · 2026-02-26T10:03:05Z

pkg/oci_resolver_test.go

+	// Uses localhost/leap-test-image:latest which must be built locally
+	// Contains:
+	// - /src/data/foo/bar (created)
+	// - /src/data/foo/bar (deleted) -> file whiteout
+	// - /src/data/foo (deleted) -> directory whiteout
+	// - /src/data (remains)
+	uri := "localhost/skiff-test-image:latest"


The comment says this test uses localhost/leap-test-image:latest, but the code uses localhost/skiff-test-image:latest. Update the comment to match the actual image name to avoid confusion when running/maintaining the test.

Copilot · 2026-02-26T10:03:06Z

pkg/oci_resolver_test.go

+func TestOCIResolver_Whiteouts(t *testing.T) {
+	// Uses localhost/leap-test-image:latest which must be built locally
+	// Contains:
+	// - /src/data/foo/bar (created)
+	// - /src/data/foo/bar (deleted) -> file whiteout
+	// - /src/data/foo (deleted) -> directory whiteout
+	// - /src/data (remains)
+	uri := "localhost/skiff-test-image:latest"
+
+	ctx := context.Background()
+	sysCtx := &types.SystemContext{}
+
+	img, _, err := ImageAndLayersFromURI(ctx, sysCtx, uri)
+	if err != nil {
+		t.Fatalf("Failed to load test image %s: %v. Ensure image is built locally.", uri, err)
+	}


This test hard-requires a locally built image and fails the suite if it’s missing. That makes go test ./... brittle outside of the Makefile. Consider skipping (with a clear message) when the image isn’t present, or gate it behind an integration build tag so unit tests remain self-contained.

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T14:37:06Z

pkg/oci_resolver.go

+	var errs error
+	for _, c := range t.closers {
+		if err := c.Close(); err != nil {
+			if errs == nil {
+				errs = err
+			} else {
+				errs = fmt.Errorf("%v; %v", errs, err)
+			}
+		}
+	}
+	return errs


The error aggregation in the Close method can lose error information. When multiple closers fail, only the first and last errors are preserved. Consider using a more robust error aggregation approach like errors.Join (available since Go 1.20) or a similar multi-error pattern to preserve all error details.

Suggested change

var errs error

for _, c := range t.closers {

if err := c.Close(); err != nil {

if errs == nil {

errs = err

} else {

errs = fmt.Errorf("%v; %v", errs, err)

}

}

}

return errs

var allErrs []error

for _, c := range t.closers {

if err := c.Close(); err != nil {

allErrs = append(allErrs, err)

}

}

if len(allErrs) == 0 {

return nil

}

return errors.Join(allErrs...)

Copilot · 2026-02-26T14:37:07Z

cmd/skiff/pkg-diff.go

+)
+
+var pkgDiffCommand = cli.Command{
+	Name:  "pkg_diff",


The command name uses underscore ("pkg_diff") which is inconsistent with the other command "topCommand" and typical CLI conventions that use hyphens. Consider renaming to "pkg-diff" to match CLI naming conventions.

Suggested change

Name: "pkg_diff",

Name: "pkg-diff",

Copilot · 2026-02-26T14:37:07Z

pkg/oci_resolver_test.go

+	resolver, err := NewOCIResolver(ctx, img, sysCtx)
+	if err != nil {
+		t.Fatalf("Failed to create OCIResolver: %v", err)
+	}
+	defer resolver.Close()


Resource leak: The resolver is not closed in the test. Add defer resolver.Close() after line 324 to ensure proper cleanup of resources even if the test fails.

dcermak requested a review from Copilot February 26, 2026 09:57

Copilot started reviewing on behalf of dcermak February 26, 2026 09:57 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

dcermak force-pushed the pkg-diff-feature branch from 97b6b2e to 053a30f Compare February 26, 2026 10:09

WIP: add pkg-diff feature

02039a6

dcermak force-pushed the pkg-diff-feature branch from 053a30f to 02039a6 Compare February 26, 2026 11:22

dcermak requested a review from Copilot February 26, 2026 14:32

Copilot started reviewing on behalf of dcermak February 26, 2026 14:33 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

		@@ -0,0 +1,9 @@
		FROM registry.opensuse.org/opensuse/leap:16.0

		RUN zypper -n in vim-small; zypper -n clean

Conversation

dcermak commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants