Skip to content

feat: List images through containerd on Linux - #124

Merged
ananos merged 4 commits into
mainfrom
feat/images
Oct 22, 2026
Merged

ananos merged 4 commits into
mainfrom
feat/images

Conversation

@cmainas

@cmainas cmainas commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

On Linux, hull is to be a containerd client: images, containers and tasks
live in containerd, and the VMs boot through urunc. The Linux build so far
was a stub that printed a greeting. This adds the containerd connection and
the first verb that uses it, hull images, along with CI that tests the
Linux build against a real containerd.

There is no store directory on Linux, and no --store-dir. A containerd
namespace is the isolation boundary instead: --namespace, then
$HULL_NAMESPACE, then hull. The name is used as given, so
ctr -n hull images ls shows what hull sees. The socket comes from
--containerd-address, then $HULL_CONTAINERD_ADDRESS, then
/run/containerd/containerd.sock for root and
$XDG_RUNTIME_DIR/containerd/containerd.sock, where rootless containerd puts
it, for anyone else.

Connect fails fast and says why. client.New does not dial, so Connect
asks the daemon for its version before returning. containerd's dialer
retries a missing socket until its timeout and then reports only the
timeout, so Connect checks for the socket first and names the missing
path. It accepts the address with or without a unix:// scheme, which
containerd's dialer cannot handle itself.

hull images prints the same table and JSON as on macOS. containerd keeps
one record per name, and the name may point at a multi-platform index, so
each record is read through to one platform manifest. Choosing that
platform is the subtle part. ctr and containerd's transfer service fetch
the manifest and config of every platform in an index, but layers for only
the platform requested, so a present manifest does not mean a present image.
DescribeImage ranks platforms whose layers are all present first, the
host's ahead of the rest, and falls back to an incomplete one only when no
platform is complete. Index entries that are not images, BuildKit's
attestation manifests and OCI artifacts with an artifactType, are never
candidates: their layers are never unpacked, so on a containerd that discards
unpacked layers they could otherwise be the only complete entries left. An
image whose content cannot be read is still listed, with a warning.

Related issues

Refs: #105

Changes

  • cmd/hull/main_linux.go: the Linux command tree, with --debug,
    --namespace and --containerd-address, and hull images as its only verb.
  • internal/containerd (new, Linux only): socket and namespace resolution,
    Connect, and DescribeImage with the platform choice. It sits outside the
    CLI because it is containerd logic, not output, and other verbs that
    describe a stored image can use it.
  • cmd/hull/imagelist.go, cmd/hull/jsonout.go: the image table and JSON
    writers lose their darwin build tag, so both builds print the same output.
    No behavior change on macOS.
  • go.mod: adds the containerd v2.3.5 client. Minimal version selection raises
    runtime-spec to v1.3.0 and a few indirect modules.
  • Tests against a live daemon, with no registry involved: images are written
    into containerd through the client (internal/containerd/containerdtest),
    then described through the image service and the gRPC content store, and
    listed by hull images run through the real command tree (newApp),
    flags included.
  • CI: a unit-linux job runs the root module's tests under -race against
    containerd v2.3.5, downloaded from its release and checked against the
    published checksum. HULL_TEST_REQUIRE_CONTAINERD turns a skipped daemon
    test into a failure.
  • CI: the lint job runs golangci-lint for Linux as well as darwin. Its
    separate go vet for Linux goes, since golangci-lint's govet runs the same
    analyzers.
  • docs/build.md: what the Linux build does now, how it finds containerd, and
    how namespaces and platforms work.

Checklist

  • make test passes
  • make macos builds hull, vz-runner and hvi, if Go, Swift or Rust code changed
  • I have added or updated tests covering the change
  • I have run the e2e harnesses (test/*.py) for changes touching boot, console, or the run path
  • I have updated the affected docs (README, docs/)

The Linux build of hull was a stub that printed a greeting. It now has
the command tree of the darwin CLI, with --debug, --version and errors
printed as "error: ..." with exit status 1, so verbs can come over from
the darwin CLI one at a time.

There is no --store-dir. On Linux, images, containers and tasks are to
live in containerd, which leaves hull no directory to own.

`hull images` is registered with its macOS flags but refuses for now. On
Linux it lists what containerd holds, and hull cannot reach containerd
yet.

No Linux runner ran the root module's tests, so neither the untagged
tests in cmd/hull nor any linux-tagged code was tested on Linux, and
golangci-lint only ran for darwin. A unit-linux job now runs the tests
natively under the race detector, and the lint job lints for Linux as
well. Its separate `go vet` for Linux goes, since golangci-lint's govet
runs the same analyzers.

Refs: #105
Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
On Linux, hull keeps its images, containers and tasks in containerd.
Connect gives the Linux commands a client to it, scoped to one
namespace. The package builds for Linux only: hull on macOS keeps its
images in its own store and never talks to containerd.

The namespace is hull's isolation boundary on Linux, the role
--store-dir plays on macOS: two namespaces on one daemon see none of
each other's state. ResolveNamespace takes an explicit name, then
$HULL_NAMESPACE, then "hull". The name is used as given rather than
derived, so `ctr -n hull` shows what hull sees. A name containerd would
reject is refused before dialing.

ResolveAddress takes an explicit socket, then $HULL_CONTAINERD_ADDRESS,
then a default by user. Root gets /run/containerd/containerd.sock, which
is also where a rootless containerd appears inside rootlesskit's
namespaces. Other users get $XDG_RUNTIME_DIR/containerd/containerd.sock,
where a rootless containerd puts it on the host. Connect takes the
address with or without a unix:// scheme: containerd's dialer adds the
scheme itself and fails to parse an address that already has it.

client.New does not dial, so Connect asks the daemon for its version
before returning, and a daemon that is down fails there, with the
address in the error. containerd's dialer retries a socket that does not
exist until its timeout and then reports only the timeout, so Connect
checks for the socket first and names it.

The containerd v2 client is a new dependency. Minimal version selection
raises runtime-spec to v1.3.0, and a few indirect modules with it.

The unit-linux job starts containerd v2.3.5, the version the
brig-standalone-linux bundle ships, from its release tarball, on a socket
the job's user owns. Tests that need a daemon skip without one unless
HULL_TEST_REQUIRE_CONTAINERD is set. The job sets it, so it cannot pass
with those tests skipped. containerdtest.DaemonAddress applies that rule,
so tests in any package get it the same way.

Refs: #105
Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
`hull images` on Linux should print exactly what it prints on macOS, but
the table and JSON writers, and the digest, size and age formatters they
use, lived in darwin-only files. imagelist.go and jsonout.go now hold
them without a build tag, so both builds share one implementation and
its tests run on Linux as well. Nothing changes on macOS.

Refs: #105
Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>
@cmainas

cmainas commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@claude-fleet review

@claude-fleet

claude-fleet Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review task queued.

Task: 2822e595-f776-40ea-8514-60b122fba09e
Commit: bb72d34
Model: claude-opus-5 (effort medium)
Executor: rp1
Status: done. Review posted.

@claude-fleet claude-fleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the whole diff, ran the package tests, and exercised hull images end to end against a real containerd I started in the workspace. The design is sound and the output matches macOS; two things to fix.

Findings:

  • should_fix, internal/containerd/images.go:129 — pickManifest can pick a buildx attestation manifest, listing the image as unknown/unknown at a few KB
  • should_fix, .github/workflows/ci.yml:275 — the busybox end-to-end step the description promises is not in the diff

Verified:

  • hull images prints the macOS table and JSON against a live daemon: ghcr.io/containerd/busybox:latest, linux/arm64/v8, index digest recorded, sha256:149ff441a10b short digest — cmd/hull/images_linux.go:49-80, cmd/hull/imagelist.go:48-74
  • Size means the same on both builds: sum of compressed layer sizes — internal/containerd/images.go:90-92 against pkg/ociclient/client.go:160-166 (go-containerregistry layer.Size() is the compressed size)
  • Connect names the address and HULL_CONTAINERD_ADDRESS for a missing socket, and also for permission denied on a socket it cannot open — internal/containerd/containerd.go:124-142
  • unix:// prefix is accepted against a live daemon — internal/containerd/containerd.go:120, containerd_test.go:166-173
  • --namespace is a root flag read from the subcommand; hull --namespace att images listed that namespace, ctr -n att images ls agreed — cmd/hull/main_linux.go:69-74, urfave/cli v3 lookupFlag walks Lineage() (command.go:383)
  • Complete-and-matching ranks first, and an index with only one platform's layers is described as that platform — internal/containerd/images.go:139-150, confirmed with a real index
  • The description's claim that jsonout.go/imagelist.go only lose their build tag holds: the moved code is byte-identical to what images.go dropped

Checks run:

  • go test -count=1 ./internal/containerd/ ./cmd/hull/ → ok (daemon tests skipped)
  • HULL_TEST_CONTAINERD_ADDRESS=... HULL_TEST_REQUIRE_CONTAINERD=1 go test ./internal/containerd/ against containerd 2.2.1 → ok, both daemon tests ran
  • GOOS=linux GOARCH=amd64 go vet ./cmd/... ./internal/containerd/... → clean
  • ctr -n allp content fetch --all-platforms docker.io/library/hello-world:latest, then removed every non-attestation manifest's layer blobs → hull images reported platform: "unknown/unknown", size: 4742, digest sha256:8e752a1cddea (the amd64 attestation manifest)
  • No revert check: this adds a feature, it does not fix a bug

Smaller things:

  • internal/containerd/containerd.go:88-93 — ResolveAddress tests geteuid() for root but builds the fallback runtime dir from getuid(). The two differ only for a setuid binary, and TestResolveAddress pins uid to 1000 in every case, so the mix is untested. Pick one, or say in the comment why each is right.
  • internal/containerd/images.go:133-136 — if err != nil || !available { continue } swallows a real content-store error (a corrupt manifest blob fails validateMediaType, which is not NotFound). The user then gets "no manifest of the index is in the content store", which names the wrong problem. Keeping the first non-NotFound error and returning it when no candidate survives would cost two lines.
  • internal/containerd/containerd.go:126-130 — the missing-socket branch assumes the address is a filesystem path. --containerd-address tcp://host:1234 fails with "no socket at tcp://host:1234" before any dial. Narrow, and the direction is safe.
  • .github/workflows/ci.yml:154-171 — the lint job now runs golangci-lint-action twice; the second invocation may fight the first over the action's cache key. Worth watching on the first few runs.

Not checked: go.sum, the indirect-module bumps in go.mod, and whether the golangci-lint linux pass is clean (I ran go vet, not golangci-lint).

Not checked:

  • go.sum and the indirect module bumps in go.mod
  • whether golangci-lint is clean under GOOS=linux (I ran go vet instead)
  • the e2e/boot/darwin CI lanes, which this PR does not touch

Task 2822e595-f776-40ea-8514-60b122fba09e on rp1, model claude-opus-5, effort medium.

matches bool
}
var present []candidate
for _, m := range manifests {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should_fix: pickManifest treats a buildx attestation manifest as a candidate platform. Those entries carry platform: {os: unknown, architecture: unknown} and vnd.docker.reference.type: attestation-manifest, and their in-toto layer is never unpacked, so it is never discarded -- which makes them the only complete manifests in an index whose real platforms have had their layers discarded. complete ranks ahead of matches at images.go:141-147, so the attestation entry wins.

Failure: ctr -n hull content fetch --all-platforms docker.io/library/hello-world:latest, then drop the layer blobs of every real platform (what a containerd with discard_unpacked_layers = true leaves after an all-platforms unpack). hull images then prints docker.io/library/hello-world latest sha256:8e752a1cddea 4.6 KB, and hull images --json gives "platform": "unknown/unknown", "size": 4742 -- the amd64 attestation manifest, not any runnable image. The package doc says this choice is also what pulling and running on Linux will use, so the same pick would later hand a VM an attestation manifest.

Evidence: reproduced against a live containerd 2.2.1 with the binary built from this commit. The attestation entries are real: the hello-world index at sha256:11a3eb1597e8 holds ten of them, one per platform. Ranking is at internal/containerd/images.go:139-150; candidacy is decided at images.go:129-138.

Fix: skip a manifest whose descriptor says os == "unknown" (or that carries the vnd.docker.reference.type annotation) in the loop at line 129, before images.Check.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid and addressed.

Comment thread .github/workflows/ci.yml
"$dir/bin/ctr" --address "$dir/containerd.sock" version
echo "HULL_TEST_CONTAINERD_ADDRESS=$dir/containerd.sock" >> "$GITHUB_ENV"

- name: Unit tests

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should_fix: The description says unit-linux has "an end-to-end step [that] lists an image pulled from ghcr.io/containerd/busybox, which keeps Docker Hub's rate limits out of CI". No such step is in the diff: the job is checkout, setup-go, Start containerd, go test -count=1 -race ./..., and the failure-only log dump. grep -n busybox .github/workflows/ci.yml returns nothing.

Failure: nothing in CI runs hull images against a daemon holding a real image. The daemon tests that do run (TestConnectToDaemon, TestConnectToDaemonWithUnixScheme) only call Version and read DefaultNamespace; DescribeImage is tested only against a local content store, never the proxy store client.ContentStore() returns, and listImages has no test at all. A regression in the ImageService().List -> DescribeImage -> imageRecord path would land green.

Evidence: .github/workflows/ci.yml:214-280 is the whole job; internal/containerd/containerd_test.go:145-173 are the only daemon tests; internal/containerd/images_test.go:88-96 uses plugins/content/local.

Either add the step (I ran the equivalent by hand and it works: ctr -n hull content fetch the image, then HULL_CONTAINERD_ADDRESS=... hull images --json), or drop the sentence from the description.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created tests and fixed text.

@cmainas

cmainas commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@claude-fleet review

@claude-fleet

claude-fleet Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review task queued.

Task: bfdbe1f6-7bd0-4f9c-b777-920f096fc639
Commit: 8a2c5b8
Model: claude-opus-5 (effort medium)
Executor: ravel-clickbench-ubuntu-2
Status: done. Review posted.

@claude-fleet claude-fleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous round at bb72d34: 2 findings.

Previous finding Classification Cited line
internal/containerd/images.go:129 (should_fix): pickManifest treats a buildx attestation manifest as a candidate platform. Those entries carry platform: {os: unknown, architecture: unknown} and vnd.docker.reference.type: attestation-manifest, and their in-toto layer is never unpacked, so it is never discarded -- which makes them the only complete manifests in an index whose real platforms have had their layers discarded. complete ranks ahead of matches at images.go:141-147, so the attestation entry wins. Failure: ctr -n hull content fetch --all-platforms docker.io/library/hello-world:latest, then drop the layer blobs of every real platform (what a containerd with discard_unpacked_layers = true leaves after an all-platforms unpack). hull images then prints docker.io/library/hello-world latest sha256:8e752a1cddea 4.6 KB, and hull images --json gives "platform": "unknown/unknown", "size": 4742 -- the amd64 attestation manifest, not any runnable image. The package doc says this choice is also what pulling and running on Linux will use, so the same pick would later hand a VM an attestation manifest. Evidence: reproduced against a live containerd 2.2.1 with the binary built from this commit. The attestation entries are real: the hello-world index at sha256:11a3eb1597e8 holds ten of them, one per platform. Ranking is at internal/containerd/images.go:139-150; candidacy is decided at images.go:129-138. Fix: skip a manifest whose descriptor says os == "unknown" (or that carries the vnd.docker.reference.type annotation) in the loop at line 129, before images.Check. fixed internal/containerd/images.go:157
.github/workflows/ci.yml:275 (should_fix): The description says unit-linux has "an end-to-end step [that] lists an image pulled from ghcr.io/containerd/busybox, which keeps Docker Hub's rate limits out of CI". No such step is in the diff: the job is checkout, setup-go, Start containerd, go test -count=1 -race ./..., and the failure-only log dump. grep -n busybox .github/workflows/ci.yml returns nothing. Failure: nothing in CI runs hull images against a daemon holding a real image. The daemon tests that do run (TestConnectToDaemon, TestConnectToDaemonWithUnixScheme) only call Version and read DefaultNamespace; DescribeImage is tested only against a local content store, never the proxy store client.ContentStore() returns, and listImages has no test at all. A regression in the ImageService().List -> DescribeImage -> imageRecord path would land green. Evidence: .github/workflows/ci.yml:214-280 is the whole job; internal/containerd/containerd_test.go:145-173 are the only daemon tests; internal/containerd/images_test.go:88-96 uses plugins/content/local. Either add the step (I ran the equivalent by hand and it works: ctr -n hull content fetch the image, then HULL_CONTAINERD_ADDRESS=... hull images --json), or drop the sentence from the description. open .github/workflows/ci.yml:275

Request changes: one new should_fix (go.mod is not tidy, which fails the release and channel workflows), and the stale description sentence from the last round is still there. The attestation-manifest pick is fixed, and I reproduced the fix against a live containerd 2.3.5.

Findings:

  • should_fix, go.mod:42 — github.com/containerd/errdefs is imported directly but listed // indirect; go mod tidy gate in release.yml/channel.yml fails

Previous round:

  • 1 (attestation manifests) — fixed by isImage, internal/containerd/images.go:157-168, called at images.go:130 before images.Check. Your version is better than what I proposed: it also covers OCI artifacts via ArtifactType and keeps the rule in one named predicate with the reasoning next to it.
  • 2 (CI claim) — open, and now only as a description defect: TestImagesCommandAgainstDaemon (cmd/hull/images_linux_test.go:36-98) and TestDescribeImageThroughDaemon (internal/containerd/images_test.go:259-320) close the real gap — the CLI path, ImageService().List/Get and the proxy content store are all exercised against a live daemon, which is what the finding was about. But the "Changes" bullet still says unit-linux has "an end-to-end step [that] lists an image pulled from ghcr.io/containerd/busybox". There is no such step: .github/workflows/ci.yml:275-276 is go test -count=1 -race ./..., and grep -rn busybox .github/workflows/ returns nothing. Replace the sentence with what you actually did (daemon-backed Go tests, gated by HULL_TEST_REQUIRE_CONTAINERD).

Verified:

  • Attestation entries are skipped on real data, not just in the unit test: with containerd 2.3.5 and ctr -n hull content fetch --all-platforms docker.io/library/hello-world:latest, hull images --json gives "platform": "linux/amd64", digest sha256:d1a8d0a4eeb6…, indexDigest sha256:11a3eb1597e8…. After deleting all 24 layer blobs of that index (no real platform complete, attestations intact) the pick is still linux/amd64.
  • Size means the same thing on both platforms — sum of compressed layer sizes: internal/containerd/images.go:92-94 against pkg/ociclient/client.go:160-165.
  • HULL_TEST_REQUIRE_CONTAINERD turns the skip into a failure — internal/containerd/containerdtest/containerdtest.go:43-52.
  • The containerd 2.3.5 tarball checksum in .github/workflows/ci.yml:243 matches the published release (sha256sum -c on the downloaded tarball: OK).
  • Before returning a nil context is safe — urfave/cli v3.10.1 runBefore only assigns when non-nil (command_run.go:400-412).
  • The platform choice ranks complete over matching and falls back correctly — internal/containerd/images.go:139-150, plus the five DescribeImage tests.

Checks run:

  • go test -count=1 -race ./internal/containerd/... ./cmd/hull/ with HULL_TEST_CONTAINERD_ADDRESS pointing at a containerd 2.3.5 started the way unit-linux starts it → ok, daemon tests included.
  • Revert check: isImage replaced with return true → TestDescribeImageSkipsEntriesThatAreNotImages fails all three subtests (Platform = "unknown/unknown", want linux/amd64); file restored, tree clean.
  • go mod tidy -diff → non-empty (the finding).
  • GOOS=darwin GOARCH=arm64 go vet ./cmd/hull/ ./internal/... → clean; gofmt -l → clean.

Smaller things:

  • internal/containerd/images.go:132-135: a transient content-store error (not just not-found) is swallowed by if err != nil || !available { continue }, so a dropped connection mid-listing surfaces as "no image manifest of the index is in the content store". Reasonable for a listing; worth a word in the comment so the next reader does not take the message literally.
  • hull images stats every blob of every candidate platform over gRPC (images.Check), so listing is O(images × platforms × blobs) round trips. Fine at current scale; something to remember if listing ever feels slow.

Not checked:

  • go.sum contents beyond what go mod tidy -diff reports.
  • The unit-linux job as GitHub runs it (no Actions runner here); I reproduced its containerd startup and test command locally instead.

Not checked:

  • go.sum contents beyond the go mod tidy -diff result
  • the unit-linux job as GitHub Actions runs it (no runner available); its containerd startup and test command were reproduced locally instead

Task bfdbe1f6-7bd0-4f9c-b777-920f096fc639 on ravel-clickbench-ubuntu-2, model claude-opus-5, effort medium.

Comment thread .github/workflows/ci.yml
"$dir/bin/ctr" --address "$dir/containerd.sock" version
echo "HULL_TEST_CONTAINERD_ADDRESS=$dir/containerd.sock" >> "$GITHUB_ENV"

- name: Unit tests

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should_fix: Still open since bb72d34. The description says unit-linux has "an end-to-end step [that] lists an image pulled from ghcr.io/containerd/busybox, which keeps Docker Hub's rate limits out of CI". No such step is in the diff: the job is checkout, setup-go, Start containerd, go test -count=1 -race ./..., and the failure-only log dump. grep -n busybox .github/workflows/ci.yml returns nothing.

Failure: nothing in CI runs hull images against a daemon holding a real image. The daemon tests that do run (TestConnectToDaemon, TestConnectToDaemonWithUnixScheme) only call Version and read DefaultNamespace; DescribeImage is tested only against a local content store, never the proxy store client.ContentStore() returns, and listImages has no test at all. A regression in the ImageService().List -> DescribeImage -> imageRecord path would land green.

Evidence: .github/workflows/ci.yml:214-280 is the whole job; internal/containerd/containerd_test.go:145-173 are the only daemon tests; internal/containerd/images_test.go:88-96 uses plugins/content/local.

Either add the step (I ran the equivalent by hand and it works: ctr -n hull content fetch the image, then HULL_CONTAINERD_ADDRESS=... hull images --json), or drop the sentence from the description.

Comment thread go.mod Outdated
github.com/containerd/console v1.0.5 // indirect
github.com/containerd/containerd/api v1.11.1 // indirect
github.com/containerd/continuity v0.5.0 // indirect
github.com/containerd/errdefs v1.0.0 // indirect

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should_fix: github.com/containerd/errdefs is imported directly by this PR but stays in the indirect require block, so go.mod is not tidy at this head.

Failure: release.yml:92-98 and channel.yml:155-165 run go mod tidy and fail on any change to go.mod or go.sum. The next release or channel build stops at that step with go mod tidy changed the module files: M go.mod. ci.yml has no tidy check, so this PR lands green and the failure appears on main.

Evidence: go mod tidy -diff on this commit wants to add github.com/containerd/errdefs v1.0.0 to the direct block and drop the // indirect line here; the direct import is internal/containerd/containerdtest/containerdtest.go:32, used at :126 (errdefs.IsNotFound).

Fix: run go mod tidy and commit the result.

@cmainas

cmainas commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

@claude-fleet review

@claude-fleet

claude-fleet Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review task queued.

Task: 3fea9022-86a8-4eb1-9740-43fb3d583d46
Commit: 12b47ff
Model: claude-opus-5 (effort medium)
Executor: rp2
Status: done. Review posted.

@claude-fleet claude-fleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous round at 8a2c5b8: 3 findings.

Previous finding Classification Cited line
internal/containerd/images.go:157 (should_fix): pickManifest treats a buildx attestation manifest as a candidate platform. Those entries carry platform: {os: unknown, architecture: unknown} and vnd.docker.reference.type: attestation-manifest, and their in-toto layer is never unpacked, so it is never discarded -- which makes them the only complete manifests in an index whose real platforms have had their layers discarded. complete ranks ahead of matches at images.go:141-147, so the attestation entry wins. Failure: ctr -n hull content fetch --all-platforms docker.io/library/hello-world:latest, then drop the layer blobs of every real platform (what a containerd with discard_unpacked_layers = true leaves after an all-platforms unpack). hull images then prints docker.io/library/hello-world latest sha256:8e752a1cddea 4.6 KB, and hull images --json gives "platform": "unknown/unknown", "size": 4742 -- the amd64 attestation manifest, not any runnable image. The package doc says this choice is also what pulling and running on Linux will use, so the same pick would later hand a VM an attestation manifest. Evidence: reproduced against a live containerd 2.2.1 with the binary built from this commit. The attestation entries are real: the hello-world index at sha256:11a3eb1597e8 holds ten of them, one per platform. Ranking is at internal/containerd/images.go:139-150; candidacy is decided at images.go:129-138. Fix: skip a manifest whose descriptor says os == "unknown" (or that carries the vnd.docker.reference.type annotation) in the loop at line 129, before images.Check. fixed internal/containerd/images.go:131
.github/workflows/ci.yml:275 (should_fix): The description says unit-linux has "an end-to-end step [that] lists an image pulled from ghcr.io/containerd/busybox, which keeps Docker Hub's rate limits out of CI". No such step is in the diff: the job is checkout, setup-go, Start containerd, go test -count=1 -race ./..., and the failure-only log dump. grep -n busybox .github/workflows/ci.yml returns nothing. Failure: nothing in CI runs hull images against a daemon holding a real image. The daemon tests that do run (TestConnectToDaemon, TestConnectToDaemonWithUnixScheme) only call Version and read DefaultNamespace; DescribeImage is tested only against a local content store, never the proxy store client.ContentStore() returns, and listImages has no test at all. A regression in the ImageService().List -> DescribeImage -> imageRecord path would land green. Evidence: .github/workflows/ci.yml:214-280 is the whole job; internal/containerd/containerd_test.go:145-173 are the only daemon tests; internal/containerd/images_test.go:88-96 uses plugins/content/local. Either add the step (I ran the equivalent by hand and it works: ctr -n hull content fetch the image, then HULL_CONTAINERD_ADDRESS=... hull images --json), or drop the sentence from the description. fixed cmd/hull/images_linux_test.go:37
go.mod:42 (should_fix): github.com/containerd/errdefs is imported directly by this PR but stays in the indirect require block, so go.mod is not tidy at this head. Failure: release.yml:92-98 and channel.yml:155-165 run go mod tidy and fail on any change to go.mod or go.sum. The next release or channel build stops at that step with go mod tidy changed the module files: M go.mod. ci.yml has no tidy check, so this PR lands green and the failure appears on main. Evidence: go mod tidy -diff on this commit wants to add github.com/containerd/errdefs v1.0.0 to the direct block and drop the // indirect line here; the direct import is internal/containerd/containerdtest/containerdtest.go:32, used at :126 (errdefs.IsNotFound). Fix: run go mod tidy and commit the result. fixed go.mod:10

No longer carried to the next round, settled in rounds 2 and 3:

  • internal/containerd/images.go:157 (should_fix): pickManifest treats a buildx attestation manifest as a candidate platform. Those entries carry platform: {os: unknown, architecture: unknown} and vnd.docker.reference.type: attestation-manifest, and their in-toto layer is never unpacked, so it is never discarded -- which makes them the only complete manifests in an index whose real platforms have had their layers discarded. complete ranks ahead of matches at images.go:141-147, so the attestation entry wins. Failure: ctr -n hull content fetch --all-platforms docker.io/library/hello-world:latest, then drop the layer blobs of every real platform (what a containerd with discard_unpacked_layers = true leaves after an all-platforms unpack). hull images then prints docker.io/library/hello-world latest sha256:8e752a1cddea 4.6 KB, and hull images --json gives "platform": "unknown/unknown", "size": 4742 -- the amd64 attestation manifest, not any runnable image. The package doc says this choice is also what pulling and running on Linux will use, so the same pick would later hand a VM an attestation manifest. Evidence: reproduced against a live containerd 2.2.1 with the binary built from this commit. The attestation entries are real: the hello-world index at sha256:11a3eb1597e8 holds ten of them, one per platform. Ranking is at internal/containerd/images.go:139-150; candidacy is decided at images.go:129-138. Fix: skip a manifest whose descriptor says os == "unknown" (or that carries the vnd.docker.reference.type annotation) in the loop at line 129, before images.Check.

No findings.

Verified the whole diff, ran the suite against a live containerd, and did the revert check for the attestation fix.

Approve: all three previous findings are fixed, no new blocking or should_fix finding.

Verified:

  • pickManifest no longer ranks non-image index entries — isImage rejects artifactType, the vnd.docker.reference.type annotation and os == "unknown", and it gates candidacy before images.Check — internal/containerd/images.go:131, 168-176
  • images.Check's return order is used correctly (available, _, _, missing, err) — core/images/image.go:303 in containerd v2.3.5 vs internal/containerd/images.go:134
  • The comparator's a.matches && guard at images.go:155 is load-bearing, not dead: it is what keeps pref.Less from dereferencing a nil Platform when neither candidate matches
  • Image.Size means the same on both builds — sum of compressed layer sizes: internal/containerd/images.go:91-93 vs pkg/ociclient/client.go:160-166, both into store.ImageMetadata.Size (pkg/store/store.go:43)
  • Linux hull images has the same flag and output surface as macOS — cmd/hull/images_linux.go:31-45 vs cmd/hull/images.go:28-42; --json gets a non-nil empty slice, so an empty namespace prints [] (images_linux.go:63)
  • The lint job's GOOS: darwin is at job level (ci.yml:117-119) and the new step overrides it to linux (ci.yml:270-276), so both build contexts really are linted
  • Description matches the diff at this head: the busybox claim is gone, and the tests it now describes exist — cmd/hull/images_linux_test.go:37 (through newApp, flags included), internal/containerd/images_test.go:257 (image service plus the gRPC content store)

Checks run:

  • git diff 8a2c5b8..HEAD → go.mod only, the tidy fix
  • go mod tidy -diff → no diff
  • go vet ./... (native linux) → clean
  • go test -count=1 ./internal/containerd/... ./cmd/hull/... → ok, ok
  • Same with HULL_TEST_CONTAINERD_ADDRESS pointing at a containerd 2.x daemon I started for this review → ok, ok; the four daemon tests run rather than skip
  • Reverted the || !isImage(m) guard at internal/containerd/images.go:131 → TestDescribeImageSkipsEntriesThatAreNotImages fails all three subtests, each picking the unknown/unknown manifest; restored, tree clean

Smaller things:

  • ResolveAddress decides root with geteuid() (containerd.go:87) but builds the fallback path from getuid() (containerd.go:92). The two disagree only for a setuid binary, which hull is not, but reading one or the other consistently would remove the question.
  • pickManifest runs images.Check per index entry, and over the client's content store each config and layer is a ReaderAt round trip to the daemon. hull images on a namespace with many multi-platform images costs O(images x platforms x layers) RPCs; ctr images ls costs one. Fine at today's scale, worth remembering when hull images gets slow.
  • docs/build.md says make test runs on Linux but does not mention that the daemon tests skip unless HULL_TEST_CONTAINERD_ADDRESS names a socket. One line pointing at it would save the next person the search.

Not checked: golangci-lint itself was not run (no binary here), so whether the two golangci-lint-action steps in one lint job contend over the action's cache is untested; the unit-linux containerd tarball checksum was not fetched or compared.

Not checked:

  • golangci-lint was not run locally (no binary available), so cache contention between the two golangci-lint-action steps in the lint job is untested
  • the containerd 2.3.5 tarball checksum pinned in ci.yml was not fetched or verified
  • go.sum entries were not reviewed line by line beyond confirming go mod tidy -diff is empty

Task 3fea9022-86a8-4eb1-9740-43fb3d583d46 on rp2, model claude-opus-5, effort medium.

@cmainas
cmainas marked this pull request as ready for review October 21, 2026 13:47
@cmainas
cmainas requested a review from ananos October 21, 2026 13:48
`hull images` on Linux lists the images in hull's containerd namespace,
in the same table and JSON form as on macOS. The global --namespace and
--containerd-address flags take precedence over HULL_NAMESPACE and
HULL_CONTAINERD_ADDRESS.

containerd keeps one record per image name, and the name may point at a
multi-arch index, while each macOS record describes one platform's
image. DescribeImage in internal/containerd therefore reads each record
through to one platform manifest. Digest is that manifest's digest,
IndexDigest the index's, Size the sum of its compressed layers, and
Labels and Platform come from its config. It lives outside the CLI
because it is containerd logic, not output, and other verbs that describe
a stored image can use it.

Choosing the platform needs care, because a manifest being present does
not mean its image is. ctr and containerd's transfer service fetch the
manifest and config of every platform in an index but the layers of only
the platform requested, so pulling linux/arm64 on an amd64 host leaves an
amd64 manifest with no layers. pickManifest ranks platforms whose layers
are all present first, the host's ahead of the rest, and settles for an
incomplete one only when none is complete, as on a containerd that
discards layers once it has unpacked them. An image whose manifest cannot
be read is still listed, under the digest its name points at and with a
warning, rather than dropped from the listing.

Entries of an index that are not images never become candidates:
BuildKit's attestation manifests, which carry platform unknown/unknown and
a vnd.docker.reference.type annotation, and OCI artifacts with an
artifactType. Their layers are never unpacked, so on a containerd that
discards unpacked layers they can be the only complete entries left, and
the ranking above would otherwise pick one.

images.Check reports missing content as unavailable, not as an error, so
an error means an entry could not be checked at all: its manifest is
malformed, or the daemon failed to answer. Such an entry is skipped, so
one bad manifest does not stop the rest of the index from being
described, but the first error is kept. If no entry is usable it is
returned instead of "no image manifest", so a daemon failing every check
is reported as failing, and a done context ends the search at once.

The platform choice is tested against an in-process content store and
against a live daemon. The daemon test writes images through the client
and reads them back through the image service and the gRPC content store;
one of them has lost the layers of every platform, so it is described
only if a missing blob comes back from the daemon as not found. A second
test runs `hull images` through newApp, flags included, and checks the
JSON, the table and an empty namespace; listImages writes to the
command's writer so the test can read what it prints.

Refs: #105
Signed-off-by: Charalampos Mainas <cmainas@nofire.ai>

@ananos ananos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @cmainas, looks great!

I ran it against a containerd 2.3.5 on a Linux box (as my user, no sudo):

  • go test -race ./... passes with HULL_TEST_REQUIRE_CONTAINERD=1.
  • hello-world fetched with --all-platforms lists as linux/amd64, and the attestation entries are skipped. alpine fetched with --platform linux/arm64 lists as linux/arm64 on the amd64 host.
  • With busybox's config blob removed, the image is still listed, with a warning.
  • With no flags, hull images found the rootless containerd already running under /run/user/1000, so the $XDG_RUNTIME_DIR default works.
  • hull run alpine now exits 3, which also covers my stub comment on #123 :D

The checksum pinned in unit-linux matches the published containerd-2.3.5-linux-amd64.tar.gz.sha256sum.

On the images.Check errors in ea38aea: the three new tests pass under -race. Against the previous images.go, the failing-store and done-context tests fail with the old "no image manifest" error, so they guard the fix. Skipping a bad entry and keeping the first error makes sense to me. One malformed manifest should not hide the whole image.

Approving.

@ananos
ananos merged commit 24dd9ec into main Oct 22, 2026
14 checks passed
@ananos
ananos deleted the feat/images branch October 22, 2026 11:08
@ananos ananos mentioned this pull request Oct 23, 2026
3 of 5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants