From 70b865d75a59b3ab672a03f715c8d12d9fd50b77 Mon Sep 17 00:00:00 2001 From: Mohit Yadav Date: Sat, 25 Jul 2026 19:12:56 +0530 Subject: [PATCH 1/3] fix(recipe,client): reject duplicate ComponentRef names, omit disabled components from SDK facade - PrepareAndValidate now rejects duplicate component-ref names (enabled or disabled) before registry back-fill, since names key value materialization, override routing, validation, filtering, and deployment ordering throughout the bundler/validations/deployer paths. - facadeResultFromInternal now skips disabled component refs, matching the facade's deployable-components contract. Fixes: #1874 --- pkg/client/v1/aicr.go | 3 ++ pkg/client/v1/aicr_internal_test.go | 28 ++++++++++ pkg/recipe/componentref_coherence_test.go | 62 +++++++++++++++++++++++ pkg/recipe/metadata.go | 14 +++++ 4 files changed, 107 insertions(+) diff --git a/pkg/client/v1/aicr.go b/pkg/client/v1/aicr.go index db0775560..a773becc8 100644 --- a/pkg/client/v1/aicr.go +++ b/pkg/client/v1/aicr.go @@ -857,6 +857,9 @@ func facadeResultFromInternal(r *recipe.RecipeResult, name string) *RecipeResult internal: r, } for _, c := range r.ComponentRefs { + if !c.IsEnabled() { + continue + } // Project the chart the component actually deploys: a source-only // Helm ref falls back to the component name (the deployers' // EffectiveChart rule), so SDK consumers never see an empty chart diff --git a/pkg/client/v1/aicr_internal_test.go b/pkg/client/v1/aicr_internal_test.go index c05c24371..5c5342ae8 100644 --- a/pkg/client/v1/aicr_internal_test.go +++ b/pkg/client/v1/aicr_internal_test.go @@ -1368,3 +1368,31 @@ func TestFacadeResultFromInternal_ChartProjection(t *testing.T) { } } } + +// TestFacadeResultFromInternal_OmitsDisabledComponents pins the facade +// contract: only deployable (enabled) components appear in the SDK +// facade result. See #1874. +func TestFacadeResultFromInternal_OmitsDisabledComponents(t *testing.T) { + t.Parallel() + + internal := &recipe.RecipeResult{ + ComponentRefs: []recipe.ComponentRef{ + {Name: "enabled-a", Type: recipe.ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + { + Name: "disabled-b", + Type: recipe.ComponentTypeHelm, + Source: "https://charts.example.com", + Version: "1.0.0", + Overrides: map[string]any{"enabled": false}, + }, + }, + } + + out := facadeResultFromInternal(internal, "test") + if len(out.Components) != 1 { + t.Fatalf("components = %d, want 1", len(out.Components)) + } + if out.Components[0].Name != "enabled-a" { + t.Errorf("got component %q, want enabled-a", out.Components[0].Name) + } +} diff --git a/pkg/recipe/componentref_coherence_test.go b/pkg/recipe/componentref_coherence_test.go index df377bdeb..2273d1c5e 100644 --- a/pkg/recipe/componentref_coherence_test.go +++ b/pkg/recipe/componentref_coherence_test.go @@ -304,3 +304,65 @@ func TestPrepareAndValidate_RejectsReservedDeployerName(t *testing.T) { }) } } + +func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) { + tests := []struct { + name string + refs []ComponentRef + wantErr bool + }{ + { + "adjacent duplicate", + []ComponentRef{ + {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + true, + }, + { + "non-adjacent duplicate", + []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + true, + }, + { + "disabled duplicate still rejected", + []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + { + Name: "a", + Overrides: map[string]any{"enabled": false}, + }, + }, + true, + }, + { + "empty names not flagged", + []ComponentRef{ + {Name: "", Overrides: map[string]any{"enabled": false}}, + {Name: "", Overrides: map[string]any{"enabled": false}}, + }, + false, + }, + { + "unique names ok", + []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &RecipeResult{ComponentRefs: tt.refs} + err := r.PrepareAndValidate() + if (err != nil) != tt.wantErr { + t.Fatalf("PrepareAndValidate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/pkg/recipe/metadata.go b/pkg/recipe/metadata.go index 6c136f5b6..37ac12033 100644 --- a/pkg/recipe/metadata.go +++ b/pkg/recipe/metadata.go @@ -563,6 +563,20 @@ func (r *RecipeResult) PrepareAndValidate() error { fmt.Sprintf("recipe component %q uses the reserved deployer override key as its name; %q is reserved for --set deployer: Argo deployer options", r.ComponentRefs[i].Name, ReservedDeployerKey)) } } + // Reject duplicate component-ref names (enabled or disabled) BEFORE + // registry back-fill. See #1874. + seen := make(map[string]bool, len(r.ComponentRefs)) + for i := range r.ComponentRefs { + name := r.ComponentRefs[i].Name + if name == "" { + continue + } + if seen[name] { + return errors.New(errors.ErrCodeInvalidRequest, + fmt.Sprintf("recipe has duplicate component ref name %q", name)) + } + seen[name] = true + } if err := r.backfillComponentTypes(); err != nil { return err } From 486b11a7e5e9bcfa5e5d30dbf686e0279569214c Mon Sep 17 00:00:00 2001 From: Mohit Yadav Date: Tue, 28 Jul 2026 16:59:36 +0530 Subject: [PATCH 2/3] address review feedback from njhensley on #1917 --- api/aicr/v1/server.yaml | 5 +- docs/integrator/go-library.md | 4 +- docs/integrator/public-api.md | 2 +- docs/user/api-reference.md | 3 + pkg/client/v1/aicr.go | 10 +++ pkg/client/v1/types.go | 4 +- pkg/recipe/componentref_coherence_test.go | 80 +++++++++++++++++------ pkg/recipe/loader_provider_test.go | 40 ++++++++++++ pkg/recipe/metadata.go | 48 ++++++++++---- pkg/recipe/metadata_store.go | 12 ++++ 10 files changed, 169 insertions(+), 39 deletions(-) diff --git a/api/aicr/v1/server.yaml b/api/aicr/v1/server.yaml index 74d9eec62..0daf2972b 100644 --- a/api/aicr/v1/server.yaml +++ b/api/aicr/v1/server.yaml @@ -1629,7 +1629,10 @@ components: properties: name: type: string - description: Component name + description: >- + Component name. Must be unique within componentRefs (across + both enabled and disabled refs); a duplicate name is + rejected with 400. type: type: string enum: [Helm, Kustomize] diff --git a/docs/integrator/go-library.md b/docs/integrator/go-library.md index 6efd4ae82..e63047c4c 100644 --- a/docs/integrator/go-library.md +++ b/docs/integrator/go-library.md @@ -248,7 +248,9 @@ identifiers. When you already hold a `pkg/recipe.AllowLists`, use `ResolveRecipe` takes the stable `RecipeRequest` shape and returns the facade `RecipeResult` — a deliberately small struct exposing the -`Name`, `Version`, and `Components` of the resolved recipe. When you +`Name`, `Version`, and `Components` of the resolved recipe. `Components` +lists enabled (deployable) components only; disabled refs remain visible +via `Resolved().ComponentRefs`. When you already hold an `*aicr.Criteria` value — for example, a REST handler that parsed criteria from an incoming HTTP request and wrapped them with `aicr.WrapCriteria` — use `ResolveRecipeFromCriteria`. It returns the diff --git a/docs/integrator/public-api.md b/docs/integrator/public-api.md index 21e86e105..648c1f946 100644 --- a/docs/integrator/public-api.md +++ b/docs/integrator/public-api.md @@ -64,7 +64,7 @@ aliases — the table below documents which. | `aicr.Phase`, `aicr.PhaseDeployment` / `PhasePerformance` / `PhaseConformance` | string consts | **Facade-owned**. Values match `pkg/validator/v1` constants verbatim for byte-identical wire round-trip. | | `aicr.ReportSummary` | `pkg/validator/ctrf.Summary` | **Facade-owned struct** with the CTRF count fields. | | `aicr.ValidateOption` | `pkg/validator.Option` | **Facade-owned** functional-option type that captures into an internal struct and translates at call time. | -| `aicr.RecipeResult` | `pkg/recipe.RecipeResult` | **Facade-owned struct** exposing `Name`, `Version`, `TranslatedAt`, and `Components`. Call `Resolved()` for the full upstream `*pkg/recipe.RecipeResult` (constraints, deployment order, validation config, metadata). The previous `aicr.Recipe` alias was removed in #1115; `ResolveRecipeFromCriteria` and `ResolveRecipeFromSnapshot` now return `*RecipeResult`. | +| `aicr.RecipeResult` | `pkg/recipe.RecipeResult` | **Facade-owned struct** exposing `Name`, `Version`, `TranslatedAt`, and `Components` (enabled/deployable components only — disabled refs remain visible via `Resolved().ComponentRefs`). Call `Resolved()` for the full upstream `*pkg/recipe.RecipeResult` (constraints, deployment order, validation config, metadata). The previous `aicr.Recipe` alias was removed in #1115; `ResolveRecipeFromCriteria` and `ResolveRecipeFromSnapshot` now return `*RecipeResult`. | | `aicr.AllowLists` | `pkg/recipe.AllowLists` | **Facade-owned struct** with `[]string` fields (Accelerators / Services / Intents / OSTypes). Use `aicr.WrapAllowLists` to lift a `*pkg/recipe.AllowLists`. | | `aicr.Criteria` | `pkg/recipe.Criteria` | **Facade-owned struct** whose enum-typed fields (Service / Accelerator / Intent / OS / Platform) project to plain strings; Nodes stays an `int` per the facade's string/int contract. Use `aicr.WrapCriteria` to lift a `*pkg/recipe.Criteria`. | | `aicr.CriteriaRegistry` | `pkg/recipe.CriteriaRegistry` | Documented transparent alias. Kept as an alias intentionally because the registry is behavior-rich (`ParseService`, `SetStrict`, `Values`, ...) and carries mutable per-`DataProvider` state — wrapping would either break the per-Client identity coupling (copy) or add no isolation win over the alias (pointer). | diff --git a/docs/user/api-reference.md b/docs/user/api-reference.md index 013374f48..4f90f5be6 100644 --- a/docs/user/api-reference.md +++ b/docs/user/api-reference.md @@ -484,6 +484,9 @@ These are the recipe **components** in [`recipes/registry.yaml`](https://github. > carrying surrounding whitespace are rejected — deployers consume them > verbatim. > Incoherent refs are rejected with HTTP 400 naming the component. +> Component ref names must also be unique within a recipe (enabled or +> disabled refs); a duplicate name is rejected with HTTP 400 naming the +> conflicting positions. ```shell # Basic: pipe recipe to bundle diff --git a/pkg/client/v1/aicr.go b/pkg/client/v1/aicr.go index a773becc8..f2a8c02b4 100644 --- a/pkg/client/v1/aicr.go +++ b/pkg/client/v1/aicr.go @@ -1058,6 +1058,16 @@ func (c *Client) BundleComponents(ctx context.Context, r *RecipeResult) ([]Compo return nil, preflightErr } + // Agree with DefaultBundler.Make (bundler.go), which fails closed with + // ErrCodeInvalidRequest when every component is disabled. Without this, + // an all-disabled recipe silently returns an empty, successful bundle + // list here — an SDK caller that loops over the result and deploys + // would silently no-op instead of getting an actionable error. + if len(r.Components) == 0 && r.internal != nil && len(r.internal.ComponentRefs) > 0 { + return nil, errors.New(errors.ErrCodeInvalidRequest, + "recipe has no enabled components") + } + bundles := make([]ComponentBundle, 0, len(r.Components)) for i := range r.Components { // Bail on every iteration so a long recipe doesn't hold diff --git a/pkg/client/v1/types.go b/pkg/client/v1/types.go index 80581188d..9273b86b7 100644 --- a/pkg/client/v1/types.go +++ b/pkg/client/v1/types.go @@ -268,7 +268,9 @@ type RecipeResult struct { // internal RecipeResult currently carries no build timestamp. TranslatedAt time.Time - // Components lists the deployable components in the recipe. + // Components lists the deployable components in the recipe — enabled + // component refs only. Disabled refs are omitted; call Resolved() for + // the full underlying ComponentRefs (enabled and disabled). Components []ComponentRef // internal holds the upstream pkg/recipe.RecipeResult so diff --git a/pkg/recipe/componentref_coherence_test.go b/pkg/recipe/componentref_coherence_test.go index 2273d1c5e..cb5c34106 100644 --- a/pkg/recipe/componentref_coherence_test.go +++ b/pkg/recipe/componentref_coherence_test.go @@ -305,6 +305,14 @@ func TestPrepareAndValidate_RejectsReservedDeployerName(t *testing.T) { } } +// TestPrepareAndValidate_RejectsDuplicateNames verifies the common +// validation boundary fails closed on a recipe with two component refs +// sharing the same non-empty Name. Component names key value +// materialization, override routing, validation, filtering, and deployment +// ordering throughout the bundler/validations/deployer paths, all of which +// assume a unique name -> ref mapping. Disabled refs participate too, so a +// disabled duplicate can't silently mask an enabled ref of the same name +// (or vice versa). See #1874. func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) { tests := []struct { name string @@ -312,48 +320,63 @@ func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) { wantErr bool }{ { - "adjacent duplicate", - []ComponentRef{ + name: "adjacent duplicate", + refs: []ComponentRef{ {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, }, - true, + wantErr: true, }, { - "non-adjacent duplicate", - []ComponentRef{ + name: "non-adjacent duplicate", + refs: []ComponentRef{ {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, }, - true, + wantErr: true, }, { - "disabled duplicate still rejected", - []ComponentRef{ + name: "disabled duplicate still rejected", + refs: []ComponentRef{ {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, - { - Name: "a", - Overrides: map[string]any{"enabled": false}, - }, + {Name: "a", Overrides: map[string]any{"enabled": false}}, }, - true, + wantErr: true, }, { - "empty names not flagged", - []ComponentRef{ - {Name: "", Overrides: map[string]any{"enabled": false}}, - {Name: "", Overrides: map[string]any{"enabled": false}}, + // Both refs are enabled and nameless — the actual shape that + // reaches the deployers if the name=="" exemption were ever + // removed. Disabled nameless refs would pass trivially for an + // unrelated reason (backfillComponentTypes/ValidateCoherence + // skip disabled refs entirely), so this must use enabled ones + // to pin the exemption it's named for. + name: "empty names not flagged", + refs: []ComponentRef{ + {Name: "", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, }, - false, + wantErr: false, }, { - "unique names ok", - []ComponentRef{ + name: "unique names ok", + refs: []ComponentRef{ {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, }, - false, + wantErr: false, + }, + { + // Reserved-key rejection runs before the duplicate check, so + // two refs both named ReservedDeployerKey must surface the + // "reserved" message, never the "duplicate" one. Deliberate + // precedence; pinned here so an inversion is caught. + name: "reserved-key precedence over duplicate check", + refs: []ComponentRef{ + {Name: ReservedDeployerKey, Overrides: map[string]any{"enabled": false}}, + {Name: ReservedDeployerKey, Overrides: map[string]any{"enabled": false}}, + }, + wantErr: true, }, } for _, tt := range tests { @@ -363,6 +386,21 @@ func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) { if (err != nil) != tt.wantErr { t.Fatalf("PrepareAndValidate() error = %v, wantErr %v", err, tt.wantErr) } + if !tt.wantErr { + return + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("expected ErrCodeInvalidRequest, got: %v", err) + } + if tt.name == "reserved-key precedence over duplicate check" { + if !strings.Contains(err.Error(), "reserved") { + t.Errorf("error should mention the reservation, got: %v", err) + } + return + } + if !strings.Contains(err.Error(), "duplicate") { + t.Errorf("error should mention the duplicate, got: %v", err) + } }) } } diff --git a/pkg/recipe/loader_provider_test.go b/pkg/recipe/loader_provider_test.go index 59eea96a1..b0a2bd4da 100644 --- a/pkg/recipe/loader_provider_test.go +++ b/pkg/recipe/loader_provider_test.go @@ -15,9 +15,13 @@ package recipe import ( + stderrors "errors" "os" "path/filepath" + "strings" "testing" + + "github.com/NVIDIA/aicr/pkg/errors" ) // newTestLayeredProvider builds a LayeredDataProvider over a temp dir holding a @@ -128,4 +132,40 @@ func TestLoadFromFileWithProvider(t *testing.T) { t.Errorf("DataProvider() = %v, want nil for nil-dp path", rec.DataProvider()) } }) + + t.Run("hand-authored duplicate componentRef names are rejected", func(t *testing.T) { + // Exercises the most-used boundary — a hand-authored, already- + // hydrated RecipeResult file loaded via LoadFromFileWithProvider, + // which is the shape `aicr bundle -r`/`aicr validate -r` and + // POST /v1/bundle load. See #1874. + layered := newTestLayeredProvider(t) + dir := t.TempDir() + path := filepath.Join(dir, "recipe.yaml") + content := "kind: RecipeResult\n" + + "apiVersion: aicr.run/v1alpha2\n" + + "criteria:\n service: eks\n" + + "componentRefs:\n" + + " - name: gpu-operator\n" + + " type: Helm\n" + + " source: https://charts.example.com\n" + + " version: \"1.0.0\"\n" + + " - name: gpu-operator\n" + + " type: Helm\n" + + " source: https://charts.example.com\n" + + " version: \"1.0.0\"\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write recipe: %v", err) + } + + _, err := LoadFromFileWithProvider(t.Context(), path, "", "vtest", layered) + if err == nil { + t.Fatal("expected an error for duplicate componentRef names, got nil") + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("expected ErrCodeInvalidRequest, got: %v", err) + } + if !strings.Contains(err.Error(), "duplicate") { + t.Errorf("error should mention the duplicate, got: %v", err) + } + }) } diff --git a/pkg/recipe/metadata.go b/pkg/recipe/metadata.go index 37ac12033..068acb431 100644 --- a/pkg/recipe/metadata.go +++ b/pkg/recipe/metadata.go @@ -534,13 +534,13 @@ func (r *RecipeResult) backfillComponentTypes() error { // PrepareAndValidate normalizes a RecipeResult's component refs and rejects // incoherent ones, in the required order: reject refs named with the reserved -// deployer override key (all refs, enabled or disabled), back-fill missing +// deployer override key, reject duplicate non-empty ref names, back-fill missing // types on enabled refs from the registry, canonicalize the type casing, then // validate // coherence (which itself only inspects enabled refs). Boundaries // that produce a RecipeResult WITHOUT running ApplyRegistryDefaults — file load // (LoadFromFileWithProvider) and external adoption (client adoptRecipe) — call -// this single method so the three steps cannot drift or be partially applied +// this single method so the four steps cannot drift or be partially applied // (e.g. validating before canonicalizing would reject legitimate lowercase // types; skipping the back-fill would reject type-less registry refs). The // resolve path (finalizeRecipeResult) instead back-fills via ApplyRegistryDefaults @@ -564,18 +564,9 @@ func (r *RecipeResult) PrepareAndValidate() error { } } // Reject duplicate component-ref names (enabled or disabled) BEFORE - // registry back-fill. See #1874. - seen := make(map[string]bool, len(r.ComponentRefs)) - for i := range r.ComponentRefs { - name := r.ComponentRefs[i].Name - if name == "" { - continue - } - if seen[name] { - return errors.New(errors.ErrCodeInvalidRequest, - fmt.Sprintf("recipe has duplicate component ref name %q", name)) - } - seen[name] = true + // registry back-fill. See validateRefNames for the rationale. + if err := validateRefNames(r.ComponentRefs); err != nil { + return err } if err := r.backfillComponentTypes(); err != nil { return err @@ -584,6 +575,35 @@ func (r *RecipeResult) PrepareAndValidate() error { return r.ValidateCoherence() } +// validateRefNames rejects duplicate non-empty component-ref names (enabled +// or disabled). Component names key value materialization, override +// routing, validation, filtering, and deployment ordering throughout the +// bundler/validations/deployer paths, all of which assume a unique +// name -> ref mapping. AICR-generated recipes never produce duplicates, but +// a hand-authored or externally-adopted recipe can, and a disabled +// duplicate must not be allowed to silently mask an enabled ref of the same +// name (or vice versa) — so ALL refs participate in the check, not just +// enabled ones. Empty names are exempt: they are never used as a lookup +// key. Shared between PrepareAndValidate (the file-load, adopt, and +// bundle/validate -r / POST /v1/bundle boundary) and finalizeRecipeResult +// (the criteria-resolve boundary), so both fail closed on the same +// recipes. See #1874. +func validateRefNames(refs []ComponentRef) error { + seen := make(map[string]int, len(refs)) + for i := range refs { + name := refs[i].Name + if name == "" { + continue + } + if first, ok := seen[name]; ok { + return errors.New(errors.ErrCodeInvalidRequest, + fmt.Sprintf("recipe has duplicate component ref name %q (refs %d and %d)", name, first, i)) + } + seen[name] = i + } + return nil +} + // ValidateCoherence rejects enabled ComponentRefs whose deployment-shape fields // are internally inconsistent (see coherenceProblem), aggregating every // offender into one ErrCodeInvalidRequest so the author sees all problems at diff --git a/pkg/recipe/metadata_store.go b/pkg/recipe/metadata_store.go index 303d4b361..43f9dfc69 100644 --- a/pkg/recipe/metadata_store.go +++ b/pkg/recipe/metadata_store.go @@ -968,6 +968,18 @@ func finalizeRecipeResult(provider DataProvider, criteria *Criteria, mergedSpec } result.Metadata.AppliedOverlays = appliedOverlays + // Reject duplicate component-ref names before they reach ValidateCoherence + // or any caller of BuildRecipeResult/ResolveRecipe. RecipeMetadataSpec.Merge + // silently last-wins-collapses same-name refs via componentMap, so this + // only fires when an external --data base carries duplicates AND no + // matching overlay merge collapses them. Mirrors the same check + // PrepareAndValidate runs at the file-load/adopt/bundle boundary, so the + // criteria-resolve boundary (aicr recipe -o, aicr query, + // Client.ResolveRecipe) fails closed on the same recipes too. See #1874. + if err := validateRefNames(mergedSpec.ComponentRefs); err != nil { + return nil, err + } + // Reject refs whose deployment-shape fields are incoherent (e.g. a Helm ref // that also carries a Kustomize tag/path), after defaults populate Type. // The same check also runs at the load and adopt boundaries (see From 2109ea21b73801311d490ecfc7c405e993107da3 Mon Sep 17 00:00:00 2001 From: Mohit Yadav Date: Wed, 29 Jul 2026 00:07:10 +0530 Subject: [PATCH 3/3] address remaining review feedback: BundleComponents guard, empty-name doc exemption, tests --- api/aicr/v1/server.yaml | 5 +-- docs/user/api-reference.md | 5 +-- pkg/client/v1/aicr.go | 10 +++--- pkg/client/v1/aicr_internal_test.go | 54 +++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/api/aicr/v1/server.yaml b/api/aicr/v1/server.yaml index 0b821017f..cdf1609d2 100644 --- a/api/aicr/v1/server.yaml +++ b/api/aicr/v1/server.yaml @@ -1655,8 +1655,9 @@ components: type: string description: >- Component name. Must be unique within componentRefs (across - both enabled and disabled refs); a duplicate name is - rejected with 400. + both enabled and disabled refs) when non-empty; a duplicate + non-empty name is rejected with 400. Refs with an empty + name are exempt from the uniqueness check. type: type: string enum: [Helm, Kustomize] diff --git a/docs/user/api-reference.md b/docs/user/api-reference.md index 2081088fc..1d058d9ca 100644 --- a/docs/user/api-reference.md +++ b/docs/user/api-reference.md @@ -487,8 +487,9 @@ These are the recipe **components** in [`recipes/registry.yaml`](https://github. > verbatim. > Incoherent refs are rejected with HTTP 400 naming the component. > Component ref names must also be unique within a recipe (enabled or -> disabled refs); a duplicate name is rejected with HTTP 400 naming the -> conflicting positions. +> disabled refs) when non-empty; a duplicate non-empty name is rejected +> with HTTP 400 naming the conflicting positions. Refs with an empty +> name are exempt from the uniqueness check. ```shell # Basic: pipe recipe to bundle diff --git a/pkg/client/v1/aicr.go b/pkg/client/v1/aicr.go index f2a8c02b4..e224e2e33 100644 --- a/pkg/client/v1/aicr.go +++ b/pkg/client/v1/aicr.go @@ -1059,11 +1059,11 @@ func (c *Client) BundleComponents(ctx context.Context, r *RecipeResult) ([]Compo } // Agree with DefaultBundler.Make (bundler.go), which fails closed with - // ErrCodeInvalidRequest when every component is disabled. Without this, - // an all-disabled recipe silently returns an empty, successful bundle - // list here — an SDK caller that loops over the result and deploys - // would silently no-op instead of getting an actionable error. - if len(r.Components) == 0 && r.internal != nil && len(r.internal.ComponentRefs) > 0 { + // ErrCodeInvalidRequest whenever there is nothing to deploy — whether + // that's every component disabled or a zero-componentRef recipe. An + // SDK caller that loops over the result and deploys would otherwise + // silently no-op on either input. + if len(r.Components) == 0 { return nil, errors.New(errors.ErrCodeInvalidRequest, "recipe has no enabled components") } diff --git a/pkg/client/v1/aicr_internal_test.go b/pkg/client/v1/aicr_internal_test.go index 5c5342ae8..e0f6274c1 100644 --- a/pkg/client/v1/aicr_internal_test.go +++ b/pkg/client/v1/aicr_internal_test.go @@ -367,6 +367,60 @@ func TestBundleComponents_HelmComponentLoadsManifestFiles(t *testing.T) { } } +// TestBundleComponents_RejectsAllDisabled locks in that an all-disabled +// recipe is rejected the same way DefaultBundler.Make rejects it +// (bundler.go), rather than silently returning an empty, successful +// bundle list. See #1917. +func TestBundleComponents_RejectsAllDisabled(t *testing.T) { + t.Parallel() + + client := newClientForBundleTest(t) + r := newRecipeResultForBundleTest(client, + []recipe.ComponentRef{ + { + Name: "disabled-a", + Type: recipe.ComponentTypeHelm, + Overrides: map[string]any{"enabled": false}, + }, + }, + nil, // facade Components is empty because the only ref is disabled + ) + _, err := client.BundleComponents(context.Background(), r) + if err == nil { + t.Fatal("expected error for all-disabled recipe, got nil") + } + var se *aicrerrors.StructuredError + if !stderrors.As(err, &se) { + t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) + } + if se.Code != aicrerrors.ErrCodeInvalidRequest { + t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) + } +} + +// TestBundleComponents_RejectsZeroComponentRefs locks in that a recipe +// with no componentRefs at all is rejected the same way as an all- +// disabled recipe — the zero-ref divergence from DefaultBundler.Make +// that the dropped r.internal.ComponentRefs conjunct used to miss. +// See #1917. +func TestBundleComponents_RejectsZeroComponentRefs(t *testing.T) { + t.Parallel() + + client := newClientForBundleTest(t) + r := newRecipeResultForBundleTest(client, nil, nil) + _, err := client.BundleComponents(context.Background(), r) + if err == nil { + t.Fatal("expected error for zero-componentRef recipe, got nil") + } + var se *aicrerrors.StructuredError + if !stderrors.As(err, &se) { + t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) + } + if se.Code != aicrerrors.ErrCodeInvalidRequest { + t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) + } +} + // TestRecipeResultFromInternal_PlumbsHelmFields locks in that the // translation from pkg/recipe.ComponentRef into the facade's // ComponentRef carries Source, Chart, and Namespace through. Without