Initialize dockdash

[alongubkin]

No description provided.

[linear]

ALIEN-7 Initialize Dockdash

[cursor]

Cmd is cleared when only entrypoint is overridden

Medium Severity

When a user sets entrypoint but not cmd, the builder unconditionally clears cmd to an empty vec via the else branch (proc_config.set_cmd(Some(vec![]))). This silently drops any cmd inherited from the base image, which is standard Docker behavior to preserve. A user setting only an entrypoint would lose the base image's default command arguments.

Additional Locations (1)

• src/image.rs#L1362-L1367

 

[alongubkin]

Already fixed in commit 39fae85 — cmd is now set to None (not empty vec) when only entrypoint is overridden.

[cursor]

Division by zero when total push size is zero

Low Severity

The expression total_push_size_bytes / (1024 * 1024) in the tracing info! macro uses integer division. While not a division-by-zero risk itself, when total_push_size_bytes is less than 1 MB the logged value will silently be 0, which could be confusing during debugging. This is minor but worth noting.

 

[alongubkin]

Not fixing — it's a log line. Logging "0 MB" for sub-MB images is fine.

[cursor]

Manifest pull-and-cache logic duplicated four times

Low Severity

The pattern of pulling a manifest from the registry, serializing it with serde_json::to_vec, writing it to the cache via cache.put_blob, and handling both serialization and cache-write errors is copy-pasted nearly identically across four branches: the deserialization-failure fallback, the cache-miss path, the cache-error path inside PullPolicy::Missing, and the PullPolicy::Always path. Extracting this into a small helper (e.g., pull_and_cache_manifest) would eliminate the redundancy and reduce the risk of these copies diverging during future maintenance.

Additional Locations (2)

• src/image.rs#L1139-L1161
• src/image.rs#L1162-L1185

 

[alongubkin]

Acknowledged — the duplication exists across different cache/pull policy branches with slightly different error handling. Not refactoring in this PR.

[cursor]

ImageBuilder build method is excessively long

Low Severity

The ImageBuilder::build() method spans nearly 490 lines, mixing multiple distinct concerns: cache lookup, registry authentication, manifest pulling and caching, config blob fetching, layer blob fetching, image configuration construction (both base-image and scratch paths), OCI tar archive assembly, and output path handling. Decomposing it into smaller focused functions (e.g., resolve_base_image, build_config, assemble_archive) would greatly improve readability and testability.

 

[alongubkin]

Agree it's long, but splitting it up for its own sake isn't worth the churn right now.

[alongubkin]

@greptile

[greptile-apps]

Greptile Summary

This PR introduces the full initial dockdash crate: OCI image build/push/extract APIs, a two-level content-addressable BlobCache (input-metadata key + content-hash key), zstd layer compression, whiteout-aware extraction, per-registry auth scoping, and project scaffolding (CI, release workflow, example, README). Many issues from earlier review rounds have been fixed (duplicated constant, duplicate error message, missing whiteout handling, leftover debug eprintln!).

Two correctness issues remain:

• Manifest cache key missing platform (src/image.rs:1099): The key manifest-v1:{ref} does not encode the target OS/arch. When the same base image is built for two different platforms with PullPolicy::Missing, the second build retrieves the first platform's resolved manifest from cache, producing an image labelled for platform B but backed by platform A's base layers.
• data() input cache key captures only content size (src/layer.rs:440): Because mtime is always 0 for in-memory data entries, the input-based cache key cannot distinguish two calls that write different bytes to the same path at the same size. A fixed-size config blob with changing values (common in templating scenarios) will silently return a stale cached layer.

Additionally, the CI workflow runs cargo test --lib, which correctly excludes the integration tests (they require a live Docker daemon), but this means regressions in the full build→push→pull→run pipeline are never caught automatically.

Confidence Score: 3/5

• Safe to merge for single-platform use cases; multi-platform builds and fixed-size in-memory data layers will silently produce incorrect results until the cache key bugs are fixed.
• The codebase is well-structured with solid error handling, good test coverage, and most prior issues resolved. Two logic bugs lower the score: the manifest cache key omitting platform can silently build wrong images for multi-platform workflows, and the data() cache key relying solely on size can return stale layers. Both bugs are latent (triggered only in specific usage patterns) but could cause silent correctness failures in production.
• src/image.rs (manifest cache key, line 1099) and src/layer.rs (data() cache key, line 440) require fixes before multi-platform or fixed-size in-memory layer use cases are safe.

Important Files Changed

Filename	Overview
src/image.rs	Core image build/push/extract logic. Contains two logic bugs: manifest cache key omits platform (wrong base layers for multi-platform builds with PullPolicy::Missing), and unpack_in Ok(false) path-traversal skip is silently discarded. The whiteout handling is correctly implemented and the determine_registry_auth function correctly scopes env-var credentials to Docker Hub only.
src/layer.rs	Layer builder with zstd compression, two-level caching (input-key and content-key), and directory walking for cache metadata. The directory() method now correctly records per-file metadata. The data() method only records size (mtime is always 0), so two calls with the same path/size/mode but different content produce the same input cache key — a false-hit correctness issue.
src/blobcache.rs	Clean content-addressable blob cache backed by cacache. Correct fallback to /tmp when home directory is not writable. Unit tests cover put/get/remove lifecycle.
src/error.rs	Structured error enum with thiserror. The Fromio::Error impl now correctly uses a fixed contextual message ("An I/O error occurred") rather than duplicating the source's Display string.
tests/build_push_tests.rs	Comprehensive integration tests covering build, push, pull, extract, and cache behaviour. Note: the CI workflow uses cargo test --lib, so these integration tests are never executed in CI (they require a Docker daemon). This means regressions in the integration path will not be caught automatically.
.github/workflows/ci.yml	Standard CI pipeline (check, fmt, clippy, test, docs). Uses cargo test --lib to exclude integration tests that require Docker — intentional but means integration paths are untested in CI.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ImageBuilder
    participant BlobCache
    participant OciClient
    participant Registry

    Caller->>ImageBuilder: build(base_image, platform, layers)
    ImageBuilder->>BlobCache: get_blob("manifest-v1:{ref}")
    alt Cache hit
        BlobCache-->>ImageBuilder: cached manifest (may be wrong platform!)
    else Cache miss
        ImageBuilder->>OciClient: pull_image_manifest(ref, platform_resolver)
        OciClient->>Registry: GET /v2/.../manifests/{ref}
        Registry-->>OciClient: image index
        OciClient-->>ImageBuilder: resolved platform manifest
        ImageBuilder->>BlobCache: put_blob("manifest-v1:{ref}", manifest)
    end
    loop Each base layer
        ImageBuilder->>BlobCache: get_blob(layer_digest)
        alt Cache miss
            ImageBuilder->>OciClient: pull_blob(layer_digest)
            OciClient->>Registry: GET /v2/.../blobs/{digest}
            Registry-->>OciClient: layer bytes
            OciClient-->>ImageBuilder: layer data
            ImageBuilder->>BlobCache: put_blob(layer_digest, data)
        end
    end
    ImageBuilder->>ImageBuilder: assemble OCI archive (base + new layers)
    ImageBuilder-->>Caller: Image

    Caller->>Image: push(target_ref, options)
    Image->>OciClient: auth(registry)
    OciClient->>Registry: POST /v2/.../auth
    loop Each layer
        Image->>OciClient: mount_blob(target, source=target, digest)
        alt Mount succeeds (layer exists)
            OciClient-->>Image: 201 Created
        else Mount fails
            Image->>OciClient: push_blob(layer data)
            OciClient->>Registry: POST /v2/.../blobs/uploads/
            Registry-->>OciClient: upload URL
            OciClient->>Registry: PUT data
            Registry-->>OciClient: 201 Created
        end
    end
    Image->>OciClient: push_blob(config)
    Image->>OciClient: push_manifest(manifest)
    OciClient->>Registry: PUT /v2/.../manifests/{tag}
    Registry-->>OciClient: 201 Created
    Image-->>Caller: pushed image ref

Loading

_{Last reviewed commit: 4523e38}

[alongubkin]

@greptile

[cursor]

Cursor Bugbot has reviewed your changes and found 5 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

[cursor]

Integer division truncates push size to zero in logs

Low Severity

total_push_size_mb = total_push_size_bytes / (1024 * 1024) uses integer division, so any push under 1 MB logs total_push_size_mb = 0, which is misleading. This is a usize division, so the result is always truncated. For small images this gives no useful information in the tracing output.

 

[alongubkin]

Duplicate of earlier comment. Not fixing — it's a log line.

[cursor]

Layer extraction lacks whiteout file handling for OCI

Medium Severity

The extract method applies layers sequentially but doesn't handle OCI whiteout files (.wh.* prefixed entries). In OCI images, these marker files signal file deletions from prior layers. Without processing them, extracted filesystems will contain stale files that the image intended to remove, producing an incorrect merged filesystem.

 

[cursor]

Scratch image rootfs type uses "layers" instead of "layers"

Medium Severity

When building from scratch, the RootFsBuilder sets .typ("layers") but the OCI image spec requires the type field to be the string "layers". While the string literal happens to be correct, this path is only exercised for scratch builds with custom layers. However, if no layers are provided, diff_ids will be empty, which may cause issues with some container runtimes expecting at least one layer.

 

[alongubkin]

The comment contradicts itself — it says the string literal is correct, then flags it anyway. No issue here.

[cursor]

Cache fallback creates nested redundant directory path

Low Severity

In BlobCache::new(), the writability check calls create_dir_all on the parent of the cache path, but if that succeeds, the actual cache directory itself isn't created until init(). However, the real issue is that if the home directory exists but the cache subdirectory specifically is unwritable (e.g., permissions), the check on the parent will succeed but later init() will fail when trying to create the actual cache dir, producing a confusing error instead of falling back to /tmp.

 

[alongubkin]

Theoretical edge case. The /tmp fallback already provides graceful degradation for the common failure modes.

[cursor]

Cache writability check misses actual target directory failures

Low Severity

BlobCache::new() tests writability by creating the parent of the cache path (~/.dockdash/cache), not the full path (~/.dockdash/cache/blobs). If the parent is creatable but the final directory cannot be created (e.g., a file exists at that path), the /tmp fallback is skipped and init() returns a hard error instead of gracefully degrading.

 

[alongubkin]

Same as above — the fallback to /tmp handles this.

[alongubkin]

@greptile

[alongubkin]

@greptile