[PR #84] [MERGED] test(image): cover ACR referrer discovery at the regctl boundary #167

New issue

Closed

opened 2026-10-29 14:20:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:20:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/84
Author: @johnsonshi
Created: 7/30/2026
Status: ✅ Merged
Merged: 8/15/2026
Merged by: @yingdi-shan

Base: main ← Head: test/acr-referrer-discovery-coverage

📝 Commits (3)

0accb6c test(image): cover ACR referrer discovery at the regctl boundary
0bb6d2e test(resolver): correct multi-referrer comment to match resolver behaviour
5f5e61c test(resolver): locate fake regctl fixtures relative to $0

📊 Changes

1 file changed (+228 additions, -8 deletions)

View changed files

📝 src/image/resolver.rs (+228 -8)

📄 Description

Summary

Follow-up for code quality on the review feedback in
https://github.com/kvcache-ai/AgentENV/pull/58#pullrequestreview-4805170907.

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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/84 **Author:** [@johnsonshi](https://github.com/johnsonshi) **Created:** 7/30/2026 **Status:** ✅ Merged **Merged:** 8/15/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `test/acr-referrer-discovery-coverage` --- ### 📝 Commits (3) - [`0accb6c`](https://github.com/kvcache-ai/AgentENV/commit/0accb6ccfb2b055b860231b049604ab01f3527af) test(image): cover ACR referrer discovery at the regctl boundary - [`0bb6d2e`](https://github.com/kvcache-ai/AgentENV/commit/0bb6d2e0dd633bf9fb5c4a38e9c63ed41f32b2ce) test(resolver): correct multi-referrer comment to match resolver behaviour - [`5f5e61c`](https://github.com/kvcache-ai/AgentENV/commit/5f5e61c03627a506aff12776ee299f76d4efe17b) test(resolver): locate fake regctl fixtures relative to $0 ### 📊 Changes **1 file changed** (+228 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `src/image/resolver.rs` (+228 -8) </details> ### 📄 Description ## Summary Follow-up for code quality on the review feedback in https://github.com/kvcache-ai/AgentENV/pull/58#pullrequestreview-4805170907. 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: ```diff .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 ``` ```jsonc { "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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:20:33 +00:00