test(image): cover ACR referrer discovery at the regctl boundary by johnsonshi · Pull Request #84 · kvcache-ai/AgentENV

johnsonshi · 2026-07-30T03:25:25Z

Summary

Follow-up for code quality on the review feedback in
#58 (review).

Both non-blocking suggestions from that review are addressed here. This PR changes no
production code — it is test-only.

1. Coverage now starts at the regctl boundary, not at the parser

The review noted that coverage started at parse_overlaybd_referrer, so reintroducing
--filter-artifact-type to the regctl artifact list command would leave tests green while
breaking ACR discovery.

discover_overlaybd_referrer_lists_referrers_unfiltered_and_finds_acr_streaming installs a fake
regctl that records its argv and replays a canned referrers index, then asserts the listing is
unfiltered and that the ACR referrer is returned.

I verified the test actually closes the gap by mutation — reintroducing the flag:

     .arg("list")
+    .arg("--filter-artifact-type")
+    .arg(OVERLAYBD_NATIVE_ARTIFACT_TYPE)
     .arg("--format")

test parse_overlaybd_referrer_picks_matching_artifact ... ok
test parse_overlaybd_referrer_keeps_first_matching_artifact ... ok
test parse_overlaybd_referrer_returns_none_without_match ... ok
test parse_overlaybd_referrer_accepts_acr_artifact_streaming ... ok
test parse_overlaybd_referrer_prefers_containerd_native_over_acr_streaming ... ok
test parse_overlaybd_referrer_ignores_unrelated_artifact_types ... ok
test parse_overlaybd_referrer_selects_first_of_acr_per_platform_referrers ... ok
test discover_overlaybd_referrer_lists_referrers_unfiltered_and_finds_acr_streaming ... FAILED

test result: FAILED. 17 passed; 1 failed

All seven parse-level tests stay green; only the new test fails. That is exactly the regression
the review described.

The same fake-regctl harness also covers the failure path, which previously had no test: a
registry error must surface as an error rather than Ok(None), since Ok(None) would silently
downgrade to a local pull-and-convert.

2. Fixtures use literal wire values

Fixtures now use the literal artifactType strings rather than the implementation constants, so an
unintended edit to a constant is caught instead of being mirrored into the fixture.
overlaybd_referrer_artifact_types_match_published_wire_values additionally pins both constants
and their preference order, so a typo fails with a readable diff rather than only surfacing as a
missed referrer.

3. Fake regctl fixtures are located relative to `$0`

The fake regctl script originally interpolated absolute fixture paths into single-quoted shell
literals. TempDir is created beneath TMPDIR, so a TMPDIR containing an apostrophe or newline
would break the generated script rather than fail cleanly.

The script now derives its own directory from $0 and reaches argv, stdout and stderr
through a quoted shell variable, so no filesystem path is embedded in shell source. This matches
the fully static fake-helper script already used in the ACR client tests
(snapshot::repository::backends::common::acr::client). Verified by running the resolver tests
under TMPDIR="/tmp/it's a dir".

Wire format verified against a real registry

Rather than trusting the fixture, I re-derived it from a live ACR registry with the same command
the code issues:

$ regctl artifact list --format body <myregistry>.azurecr.io/streamtest:v1

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "manifests": [
    { "digest": "sha256:1bdbcce5...", "artifactType": "application/vnd.azure.artifact.streaming.v1",
      "annotations": { "streaming.format": "overlaybd", "streaming.version": "v1",
                       "streaming.platform.os": "linux", "streaming.platform.arch": "amd64" } },
    { "digest": "sha256:3a5ae954...", "artifactType": "application/vnd.azure.artifact.streaming.v1",
      "annotations": { "streaming.format": "overlaybd", "streaming.version": "v1",
                       "streaming.platform.os": "linux", "streaming.platform.arch": "arm64" } }
  ]
}

The artifactType literal matches byte-for-byte, so the fixture is faithful.

On platform selection, measured against the same registry:

subject passed to `regctl artifact list`	referrers returned
`streamtest:v1` — multi-arch tag, resolves to an index	2 — `amd64` + `arm64`
`streamtest@sha256:c4116d4d…` — the `linux/amd64` child	1 — `amd64` only

resolve_fetched_manifest passes fetched.selected_image_ref into discovery, and oci_image
pins that to the platform-resolved manifest digest via
image_ref_with_digest(&current_ref, &manifest_digest). Discovery therefore never receives an
index, and the subject it does receive carries exactly one referrer, for the right architecture.
parse_overlaybd_referrer_selects_first_of_acr_per_platform_referrers keeps the two-referrer
payload as a defensive lock on first-match selection.

The discovery path these tests cover was also exercised end to end against a live Premium ACR
registry and an AKS cluster, where a sandbox booted from an ACR artifact-streaming image — though
that exercises the production change from #58 rather than anything in this test-only PR.

Test results

cargo fmt --check clean, cargo clippy --all-targets -- -D warnings clean.

	passed	failed
`origin/main` (`8174bd9`)	649	3
this branch	653	3

The 3 failures are pre-existing and identical on unmodified main (snapshot::p2p::tests::local_overlaybd_layers_*
and …::acr::source_image::tests::plans_descriptorless_sparse_overlaybd_delta_for_dense_export);
they appear environment-specific to my aarch64 container and are unrelated to this change. Net
+4 tests, no regressions.

Follow-up to the review on kvcache-ai#58, which noted that coverage started at `parse_overlaybd_referrer`, leaving the referrer *query* unverified: reintroducing `--filter-artifact-type` to `regctl artifact list` would pin discovery to a single artifactType and silently drop ACR streaming referrers while every parse-level test stayed green. Add a fake `regctl` that records its argv and replays a canned response, then assert `discover_overlaybd_referrer` lists referrers unfiltered and returns the ACR referrer. Confirmed by mutation: reintroducing the filter flag leaves all seven `parse_overlaybd_referrer` tests green and fails only the new test. Also cover the regctl failure path, which had no test: a registry error must surface as an error rather than `Ok(None)`, which would silently downgrade to a local pull-and-convert. Use literal artifactType wire values in the fixtures instead of the implementation constants, so an unintended edit to a constant is caught instead of being mirrored into the fixture, and pin both constants and their preference order in a dedicated test. Add a multi-arch case taken from a real ACR response: ACR attaches one streaming referrer per platform, all sharing the same artifactType, so several matches is the normal case for a multi-arch subject rather than an oddity.

github-actions · 2026-07-30T03:28:57Z

✅ OpenCodeReview: No comments generated. Looks good to me.

github-actions · 2026-07-30T03:28:58Z

+        assert_eq!(
+            parse_overlaybd_referrer(&body.to_string()).expect("parse"),
+            Some((
+                "sha256:1bdbcce5b02202d0c4c204da05e6ffbb8605bb178f441ff2990b9924c046a69f"
+                    .to_string(),
+                "application/vnd.azure.artifact.streaming.v1"
            ))
        );


[bug · medium]
This test codifies manifest-order selection even though the descriptors identify different platforms. try_resolve_overlaybd_referrer already has the requested arch, but discovery/parsing does not receive it, and the selected referrer manifest is not subsequently checked against its streaming.platform.* annotations. If ACR returns these descriptors in the opposite order, an arm64 resolve can use the amd64 artifact (or vice versa). Pass the target platform into discovery/parsing and prefer the descriptor whose streaming.platform.os/streaming.platform.arch match it; add a test where the matching descriptor is not first.

github-actions · 2026-07-30T03:28:58Z

+            write!(
+                file,
+                "#!/bin/sh\nprintf '%s\\n' \"$@\" > '{argv}'\ncat '{stdout}'\ncat '{stderr}' >&2\nexit {exit_code}\n",
+                argv = argv_path.display(),
+                stdout = stdout_path.display(),
+                stderr = stderr_path.display(),
+            )


[security · low]
These paths are inserted directly into single-quoted shell literals. A TMPDIR containing an apostrophe will break the generated script and can cause unintended shell interpretation, so the tests are fragile under a valid test environment configuration. Avoid interpolating paths into shell source: have the script derive its directory from $0 and access argv, stdout, and stderr relative to that directory (with normal double-quoted shell expansions), or use a helper that performs proper shell escaping.

…viour The per-platform fixture comment claimed the multi-referrer path is the normal case. Verifying against a live ACR registry shows otherwise: referrers queried on a multi-arch tag return one per platform, but `resolve_fetched_manifest` never passes a tag or index. It hands `FetchedManifest::selected_image_ref` to discovery, which oci_image pins to the platform-resolved manifest digest, and that subject carries exactly one referrer. Reword the comment so it documents the fixture as a defensive lock on first-match selection rather than the common path. No behaviour or assertion changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ecb58e-c794-4a64-acd3-e00d8e81cbea

github-actions · 2026-07-30T05:07:58Z

+            write!(
+                file,
+                "#!/bin/sh\nprintf '%s\\n' \"$@\" > '{argv}'\ncat '{stdout}'\ncat '{stderr}' >&2\nexit {exit_code}\n",
+                argv = argv_path.display(),
+                stdout = stdout_path.display(),
+                stderr = stderr_path.display(),
+            )


[security · medium]
The fixture paths are inserted directly into single-quoted shell literals. TempDir is created beneath an externally controlled temporary directory, so a valid TMPDIR containing ' (or a newline) can break the generated script and potentially inject shell commands. Avoid embedding filesystem paths: have the script derive its directory from $0 and access the adjacent fixture files through quoted shell variables, as the existing shared fake-regctl fixture does.

Suggestion:

Suggested change

write!(

file,

"#!/bin/sh\nprintf '%s\\n' \"$@\" > '{argv}'\ncat '{stdout}'\ncat '{stderr}' >&2\nexit {exit_code}\n",

argv = argv_path.display(),

stdout = stdout_path.display(),

stderr = stderr_path.display(),

)

write!(

file,

"#!/bin/sh\ndir=\"$(cd \"$(dirname \"$0\")\" && pwd)\"\nprintf '%s\\n' \"$@\" > \"$dir/argv\"\ncat \"$dir/stdout\"\ncat \"$dir/stderr\" >&2\nexit {exit_code}\n",

)

The fake regctl script interpolated absolute fixture paths into single-quoted shell literals. TempDir is created beneath TMPDIR, so a TMPDIR containing an apostrophe or newline would break the generated script rather than fail cleanly. Derive the fixture directory from $0 inside the script and access argv, stdout and stderr through a quoted shell variable, so no filesystem path is embedded in shell source. This matches the fully static fake-helper script already used in the ACR client tests. Verified by running the resolver tests under TMPDIR="/tmp/it's a dir". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ecb58e-c794-4a64-acd3-e00d8e81cbea

github-actions Bot reviewed Jul 30, 2026

View reviewed changes

johnsonshi marked this pull request as draft July 30, 2026 04:42

github-actions Bot reviewed Jul 30, 2026

View reviewed changes

johnsonshi marked this pull request as ready for review July 30, 2026 05:51

johnsonshi marked this pull request as draft July 30, 2026 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(image): cover ACR referrer discovery at the regctl boundary - #84

test(image): cover ACR referrer discovery at the regctl boundary#84
johnsonshi wants to merge 3 commits into
kvcache-ai:mainfrom
johnsonshi:test/acr-referrer-discovery-coverage

johnsonshi commented Jul 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot Jul 30, 2026

Uh oh!

github-actions Bot Jul 30, 2026

Uh oh!

github-actions Bot Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

johnsonshi commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

1. Coverage now starts at the regctl boundary, not at the parser

2. Fixtures use literal wire values

3. Fake regctl fixtures are located relative to $0

Wire format verified against a real registry

Test results

Uh oh!

github-actions Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

johnsonshi commented Jul 30, 2026 •

edited

Loading

3. Fake regctl fixtures are located relative to `$0`

github-actions Bot commented Jul 30, 2026 •

edited

Loading