Actionable comments posted: 7

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/contributor/recipe.md`:
- Around line 152-158: Resolve the merge conflict in the criteria table by
removing the conflict markers and merging both valid updates into a single
Markdown row set. Keep the `service` values aligned with the API contract by
preserving `ocp`, and keep the `accelerator` values aligned with the latest
support by including `l40s`; verify the `CriteriaServiceType` and
`CriteriaAcceleratorType` entries remain consistent with the documented schema.

In `@docs/user/api-reference.md`:
- Around line 121-127: Resolve the merge conflict in the API reference table by
removing the conflict markers and choosing the entries that match the API
schema. In the table for the documented accelerator/service fields, keep the
`service` list aligned with `api/aicr/v1/server.yaml` by retaining `ocp`, and
keep the `accelerator` list consistent with the supported values by including
`l40s` from the feature branch. Use the `service` and `accelerator` rows in the
affected table as the anchors when updating the docs.

In `@docs/user/cli-reference.md`:
- Around line 389-395: Resolve the merge conflict in the CLI reference table by
removing the conflict markers and merging both intended updates. Keep
`--service` aligned with `api/aicr/v1/server.yaml` by preserving `ocp`, and
merge the `--accelerator` values so the list includes the new `l40s` entry
alongside the existing GPU types. Verify the final table formatting in
`docs/user/cli-reference.md` remains valid after the conflict is removed.

In `@pkg/recipe/doc.go`:
- Line 29: The godoc in doc.go is inconsistent because the detailed accelerator
list under the Criteria types section still omits CriteriaAcceleratorL40S even
though the summary comment already mentions l40s. Update the comment block
around the CriteriaAcceleratorType / CriteriaAcceleratorL40 bullet list to
include CriteriaAcceleratorL40S alongside CriteriaAcceleratorL40, keeping the
documented accelerator set aligned with the API changes.

In `@tests/uat/oci/run`:
- Around line 125-130: The helm-diff installation step is unpinned, so UAT runs
can change when upstream releases change. Update the install logic in the run
script’s helm-diff plugin block to request a specific helm-diff version instead
of the default latest release, and apply the same version pinning pattern in the
matching helm-diff install blocks in tests/uat/aws/run and tests/uat/gcp/run so
all UAT environments behave consistently.

In `@tests/uat/oci/tests/cuj1-training/assert-recipe.yaml`:
- Around line 36-38: Update the CUJ1 training assertion fixture to verify the
OKE cert-manager addon is explicitly disabled, not just present in
componentRefs. In assert-recipe.yaml, extend the existing cert-manager check so
the rendered recipe/manifest is validated against the disabled state for
cert-manager, using the cert-manager and componentRefs assertions together to
catch any regression that re-enables the managed addon.

In `@tests/uat/oci/tests/l40s-training-config.yaml`:
- Around line 77-81: The validate agent block only sets tolerations, so the
validation Jobs can still schedule onto the system pool; update
spec.validate.agent to also mirror the GPU node selector used by
spec.snapshot.agent and keep the existing namespace/tolerations in place. Use
the agent stanza under spec.validate as the target and align it with the
nodeSelector settings from spec.snapshot.agent so validator jobs are constrained
to the L40S GPU pool.

---

Outside diff comments:
In @.claude/skills/aicr-analyzing-snapshots/SKILL.md:
- Around line 152-158: Remove the merge conflict markers from the Markdown table
in the snapshot analysis skill document and reconcile the branch/base content
into valid table rows. While fixing the table, make sure the supported service
list stays aligned with the service enum in api/aicr/v1/server.yaml by
preserving the ocp entry alongside the other supported services.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/uat/oci/tests/cuj1-training/chainsaw-test.yaml`:
- Around line 21-29: Update the CUJ1 Chainsaw UAT to exercise the new Oracle
Linux OKE path instead of the old Ubuntu selector. In the test spec’s
descriptive text and any recipe/bundle generation steps that still reference
`ubuntu`, change them to `ol` and ensure the `generate recipe`/`generate bundle`
flow explicitly passes `--os ol` so it matches the new OKE overlays.
Double-check the CUJ1 scenario steps and any related assertions in
`chainsaw-test.yaml` so the test validates the OKE/L40S/training path with
Oracle Linux throughout.

---

Outside diff comments:
In `@pkg/recipe/metadata_test.go`:
- Around line 2193-2210: TestNFDTopologyUpdater_OverlayCoverage is missing the
new OKE/L40S cases, so add coverage entries for the new l40s-oke-training and
l40s-oke-inference leaves in the existing criteria table. Update the table in
metadata_test.go alongside the other real-cluster GPU leaves so the overlay
inheritance path for CriteriaServiceOKE with CriteriaAcceleratorL40S and the
training/inference intents is exercised and regressions in the new leaf coverage
are caught.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/fingerprint/from_measurements.go`:
- Around line 323-333: Add a regression test for the raw-OCID compatibility path
in TestFromMeasurements_ServiceDetection so normalizeProviderID is covered for
older measurements that still carry a bare ocid1.* providerID. Extend the
existing table-driven cases with a raw OCI OCID input and assert it resolves to
the oke service type, alongside the already-normalized provider name cases. Use
the normalizeProviderID helper and the TestFromMeasurements_ServiceDetection
table as the main anchors when updating the test.

The L40S + ol OS enum core is well done — criteria/oskind/fingerprint (correct L40S-before-L40 SKU ordering), OpenAPI, CLI, and thorough table-driven tests all line up, and the OKE-requires-ol model mirrors the GKE-cos precedent cleanly.

The blocker is that the diff carries three changes unrelated to L40S that look like collateral from a stale/conflicted branch (mergeable_state is dirty):

1. Deletes the Rekor supply-chain monitor (.github/workflows/rekor-monitor.yaml, added in #1480) plus its docs — silently removes a security control.
2. Reverts #1495 by re-adding the single-shot verifyClusterPolicyReady check that was removed for flaking the deployment gate.
3. New overlays use the pre-#1499 apiVersion aicr.nvidia.com/v1alpha1 instead of the aicr.run/v1alpha2 every other overlay uses.

Please rebase cleanly onto main so these three drop out, then keep only the L40S/ol changes. Two smaller items: the risk assessment labels the OKE os: ol requirement "non-breaking/additive," but it's a behavior change (OKE queries without --os now return no match — worth restating as such); and the unrelated UAT evidence-repo rename (aws/gcp) should ideally land separately. One doc nit inline. Happy to re-review once it's rebased.

Cross-review: requesting changes

This PR rolls back three recently-merged changes and is incompatible with the current main API version. The rollbacks are reversed in the branch's own commits (its parent already contains #1495, #1480, and #1479), so a plain rebase will not restore them.

Blocking

1. Re-adds the single-shot verifyClusterPolicyReady deployment check that #1495 removed. It samples ClusterPolicy status.state once with no retry, while the chainsaw check validate-cluster-policy-ready already polls the same state for ~5m. On a freshly-installed gpu-operator the operator-validator init containers take ~2.5m to reconcile, so the Go check records a false failure before chainsaw polls ready — re-introducing the deployment-gate flake #1495 fixed. Drop the Go re-add; keep the polling assertion as the sole source.

2. All five newly added AICR documents use apiVersion: aicr.nvidia.com/v1alpha1, which current main rejects (expected "aicr.run/v1alpha2"). This fails TestAllMetadataFilesHaveRequiredFields and CLI validation.

3. Deletes .github/workflows/rekor-monitor.yaml and its runbook in docs/contributor/maintaining.md (added in #1480) with no replacement, dropping hourly Rekor transparency-log consistency + release-identity monitoring. This appears out of scope for an L40S feature PR — please restore it unless the removal is intentional and tracked elsewhere.

4. The UAT evidence push target reverts the #1478/#1479 stable-repo + :run-${RUN_ID} tagging scheme, creating a new untagged GHCR repository per run. Two effects: (a) the configured reference now lacks an explicit tag, so the summary no longer identifies the bundle — main's #1498 digest-based summary must be preserved through the rebase to fix that; and (b) per-run repositories proliferate regardless, which the digest summary does not address.

Non-blocking (both genuine medium-severity defects)

5. parseProvider drops the oci:// → oke mapping in favor of raw-OCID detection only; an oci://… providerID now resolves to service: any. Keep the change additive.

6. The OCI UAT runner references .github/workflows/uat-oci.yaml, which the PR never adds, and nothing else invokes tests/uat/oci, so the advertised OKE L40S UAT runs in no CI pipeline.

Doc/enum drift not attached inline (outside modified hunks): docs/contributor/index.md:182 — the pkg/recipe/oskind "single source of truth" enumeration omits ol.

Approving — all four outstanding review concerns are addressed at a8c2342:

• OKE no-OS query / partial recipe: oke.yaml is restored as an OS-agnostic parent (service: oke, no os: field), with the new oke-ol* Oracle Linux variants living alongside it rather than replacing it. A no---os OKE query matches the full overlay chain again.
• Base-name breakage: oke, oke-training, and oke-inference base names are back, so external --data overlays referencing them continue to resolve.
• helm-diff supply chain: both tests/uat/aws/run and tests/uat/gcp/run now install from the signed release tarball; --verify=false is gone.
• Stale comment: the verifyClusterPolicyReady reference in validators/deployment/expected_resources.go was removed.

CI is green and the refactor (keep OS-agnostic parents + add oke-ol* variants) is the right shape.

Two non-blocking follow-ups before merge:

• The four threads are still formally open in the UI (#2/#4 auto-flipped to outdated; the two oke-ol.yaml threads target a still-present line) — please mark them resolved.
• Branch is behind main; rebase/update before merging.

Re-review: requesting changes (round 4)

The earlier missing-OS L40S case, cert-manager, provider normalization, restored OKE parent identities, and the documentation issues are fixed. But tracing actual resolution output (not just structure) surfaces new regressions from the oke-ol* parallel chain + the OS guard.

Blocking

1. --os ol weakens the GB200 K8s floor — the generic oke-ol-training (>= 1.30) overrides gb200-oke-training (>= 1.34), dropping the GB200 DRA/NVLS prerequisite (inline on recipes/overlays/oke-ol-training.yaml).
2. The signed-tarball helm-diff install may fail on a clean Helm 4 runner without an imported keyring (inline on tests/uat/aws/run) — please confirm on a fresh runner.
3. The OS guard now rejects valid OS-agnostic OKE queries because the generic oke-ol* overlays contribute ol for every accelerator (inline on pkg/recipe/metadata_store.go).

Should fix

4. The guard's success shortcut ignores platform/intent, so a platform query can still pass with a partial recipe (inline on pkg/recipe/metadata_store.go).
5. The guard violates the public OS-optional contract and leaks CLI --os wording into API/SDK errors (inline on pkg/recipe/metadata_store.go).

Root cause for 1/3/4: oke-ol* duplicate their parents' content as a parallel generic chain. Making them thin descendants that add only OL-specifics (no re-declared K8s.server.version, and not generic-accelerator) would resolve 1 and 3 together.

Approving. The blocking GB200 K8s-floor regression is fixed — verified by building this head and resolving --service oke --accelerator gb200 --intent training --os ol, which now yields K8s.server.version >= 1.34 (L40S correctly stays >= 1.30, and the no-OS GB200 query fail-closes).

Three non-blocking follow-ups remain (fine as separate issues, not gating this PR):

• Confirm the helm-diff signed-tarball install succeeds on a clean Helm 4 runner (may need an imported keyring) — CI will surface it if not.
• The OS-required guard's success check keys on service+accelerator only, so a platform/intent query can still pass with a partial recipe.
• The guard returns CLI-flavored --os wording via the shared MetadataStore used by the HTTP server/SDK, and returns HTTP 400 for requests the OpenAPI/godoc still document as OS-optional — reconcile wording/contract.