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 `@aicr_test.go`:
- Around line 255-258: The test creates a real client via aicr.NewClient with
aicr.WithRecipeSource(tmp) and never closes it; add cleanup to close the client
to avoid leaking per-client cache state—after successful NewClient return,
register a cleanup (e.g., using t.Cleanup or defer) that calls client.Close() so
the client's resources are released; locate the client variable created by the
call to aicr.NewClient and ensure client.Close() is invoked during test
teardown.
- Around line 54-56: Replace the hardcoded absolute path in the test when
calling aicr.NewClient with a path built from the test's temporary directory so
it is guaranteed missing; specifically, construct the recipe path using
t.TempDir() (e.g., filepath.Join(t.TempDir(), "provider-nvidia-aicr-test")) and
pass that into aicr.WithRecipeSource(aicr.FilesystemSource(...)) so the test is
deterministic across environments.

In `@aicr.go`:
- Around line 146-148: The NewClient function currently iterates the opts slice
and calls each opt(c) unguarded, which can panic if an Option entry is nil;
update the loop in NewClient to check for nil before invocation (e.g., skip or
return an error when opt == nil) so Option entries are validated, ensuring you
reference the opts slice and the Option function type and call sites such as
opt(c) safely.
- Around line 297-298: Guard against a nil context before calling
context.WithTimeout in ResolveRecipe, BundleComponents, CollectSnapshot, and
ValidateState: add an explicit check if ctx == nil and return
ErrCodeInvalidRequest (or the function's error return) before invoking
context.WithTimeout(ctx, ...) so the subsequent ctx, cancel :=
context.WithTimeout(...) / defer cancel() cannot panic; update the code paths
where context.WithTimeout is used (the local ctx variable and cancel handling)
to assume ctx is non-nil after the guard.

In `@docs/integrator/go-library.md`:
- Around line 175-178: The docs state that ResolveRecipe applies
defaults.RecipeBuildTimeout as the cap, but the actual facade implementation
uses defaults.RecipeOperationTimeout; update the text to reflect the real
behavior by replacing the reference to defaults.RecipeBuildTimeout with
defaults.RecipeOperationTimeout (or otherwise state that ResolveRecipe uses the
RecipeOperationTimeout cap), and ensure the sentence references the
ResolveRecipe method and the defaults.RecipeOperationTimeout symbol so
integrators are not misled.

In `@docs/integrator/public-api.md`:
- Line 23: The package matrix entry for pkg/validator incorrectly says
"four-phase validation"; update the wording in the public API docs to match the
PR's actual public facade by changing the description for `pkg/validator` to
"three-phase validation (Deployment, Performance, Conformance)" or equivalent
phrasing that lists the three phases, ensuring the `pkg/validator` row text and
any related mention of phase count reflect three phases consistently.

In `@pkg/recipe/metadata_store_test.go`:
- Around line 1677-1680: The test currently only asserts err != nil for the
first canceled load; change it to assert the error carries the structured
timeout code (errors.ErrCodeTimeout) so regressions to plain context.Canceled
fail. Update the test assertions around the first load call (and similarly at
the other spot noted around 1692-1694) to use the pkg/errors helpers—e.g.,
verify the returned error has ErrCodeTimeout (using the package's
structured-error inspection function or errors.PropagateOrWrap semantics) rather
than merely non-nil, so the timeout code is preserved.

In `@pkg/recipe/metadata_store.go`:
- Around line 116-127: Update the LoadMetadataStoreFor godoc to reflect the new
auto-eviction of transient context cancellation errors: remove or correct any
sentence stating that transient cancellations remain cached and require
EvictCachedStore, and instead document that entries whose errs satisfy
isTransientLoadError (e.g., context.Canceled/context.DeadlineExceeded) are
automatically removed by the cache via storeCache.CompareAndDelete on detection,
so callers do not need to call EvictCachedStore to retry after transient
cancel/deadline errors; reference the LoadMetadataStoreFor behavior and the
transient-error handling implemented around storeCache.CompareAndDelete.

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 `@aicr_test.go`:
- Around line 370-392: The test currently only checks that BundleComponents
returns a non-nil error after client.Close; update the assertion to verify the
structured error code equals ErrCodeInvalidRequest to prevent contract
regressions. Locate the failing branch in the test where client.Close() has been
called and BundleComponents(context.Background(), &aicr.RecipeResult{Components:
nil}) is invoked, then replace the loose nil check with an assertion that
inspects the returned error (e.g., unwrap or type-assert to your SDK error type)
and confirms its code equals aicr.ErrCodeInvalidRequest while still failing the
test if it does not.
- Around line 126-133: The test creates two real clients via NewClient (clientA
and clientB) but never closes them, leaking per-client cache state; after each
successful NewClient call (for clientA and clientB) call and/or defer the
client's Close method (e.g., defer clientA.Close() immediately after clientA is
created and check err, and likewise defer clientB.Close()) so both clients are
properly released at the end of the test and state is not leaked between tests.

In `@aicr.go`:
- Around line 607-617: BundleComponents is honoring ctx but loadManifestFiles
does not, so long loops over ref.ManifestFiles can continue after cancellation;
change loadManifestFiles to accept a context.Context (e.g.,
loadManifestFiles(ctx context.Context, r.internal, dp, facade.Name) or similar),
update its callers in BundleComponents (both the generic and "kustomize" cases)
to pass the ctx, and inside loadManifestFiles check ctx.Done() (or return
ctx.Err()) between/ before file reads (and when returning) so the function
aborts promptly on cancellation and propagates the context error up.

In `@options.go`:
- Around line 55-60: Update the applyValidateOptions doc comment to remove the
incorrect claim that the translation occurs "inside the locked region" and
instead state that ValidateState calls applyValidateOptions after releasing
Client.mu (i.e., the translation happens once per call outside the Client.mu
lock), so future additions to validator.With* only require edits here and not on
the façade; reference applyValidateOptions, ValidateState, and Client.mu in the
revised sentence to make the synchronization scope explicit.

---

Duplicate comments:
In `@docs/integrator/go-library.md`:
- Around line 175-178: Update the docs to reflect the actual timeout used by the
facade: change the reference from defaults.RecipeBuildTimeout to
defaults.RecipeOperationTimeout in the paragraph describing ResolveRecipe (and
other context-aware facade methods) so the documentation matches the current
implementation where ResolveRecipe applies defaults.RecipeOperationTimeout as
the cap on caller-provided contexts.

In `@docs/integrator/public-api.md`:
- Line 23: Update the docs entry for `pkg/validator` in the package matrix to
reflect the correct phase count and phrasing: replace "four-phase validation"
with "three-phase validation (Deployment, Performance, Conformance)" so the
README matches the public facade and lists the actual phases.

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 `@aicr_test.go`:
- Around line 198-201: The goroutine currently discards the error from
client.Close(); instead capture and assert the Close() result: in the goroutine
call err := client.Close() and send err on a dedicated channel (e.g.,
closeErrCh) before calling wg.Done(); after wg.Wait() read from closeErrCh and
fail the test if err != nil (t.Fatalf or require.NoError). Apply the same
pattern for the other identical goroutine usage so both client.Close() calls are
checked.

---

Duplicate comments:
In `@aicr_test.go`:
- Around line 131-138: The test creates two clients via aicr.NewClient (clientA
and clientB) but never closes them; call the clients' Close method (e.g., defer
clientA.Close() immediately after clientA is created and err checked, and defer
clientB.Close() after clientB is created) or explicitly call clientA.Close() and
clientB.Close() at the end of the test to release per-client cache state and
avoid leaking resources.

In `@aicr.go`:
- Around line 833-858: Update loadManifestFiles to accept a context (change
signature to loadManifestFiles(ctx context.Context, internal
*recipe.RecipeResult, dp recipe.DataProvider, componentName string) ([]byte,
error)) and make the manifest-reading loop cancellation-aware: before each
iteration (and optionally after each read) check ctx.Done() and if canceled
return ctx.Err() (wrapped into the package error type like other failures);
also, if a context-aware manifest reader exists (e.g.,
recipe.GetManifestContentWithProviderWithContext or similar), call that instead
of recipe.GetManifestContentWithProvider so the underlying I/O is also
cancellable. Ensure references to loadManifestFiles callers (e.g.,
BundleComponents) are updated to pass the ctx.

In `@options.go`:
- Around line 55-59: The godoc for applyValidateOptions incorrectly states the
translation happens “inside the locked region”; update the sentence to reflect
that applyValidateOptions is invoked after Client.mu is released by
Client.ValidateState (i.e., the translation occurs outside the locked region /
after unlocking). Edit the comment in applyValidateOptions to say the
translation happens once per call and is performed outside the Client.mu lock
(so future fields added to validator.With* require one edit here and none on the
facade surface).

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/integrator/go-library.md`:
- Line 145: Replace the speculative sentence "Additional sources (HTTP, Git) may
be added in future minor releases." with a factual, present-tense statement
reflecting current capabilities (e.g., "Currently supports local file and OCI
sources." or "No additional sources are supported at this time.") so the
documentation does not imply an uncommitted roadmap; update the sentence in
docs/integrator/go-library.md where that exact phrase appears.
- Around line 168-176: The documentation wording is inaccurate: the facade
applies a per-operation timeout unconditionally (it always calls
context.WithTimeout), so the effective deadline is the minimum of the caller's
deadline and the facade cap. Update the text to state that each context-aware
facade method (e.g., ResolveRecipe, BundleComponents, CollectSnapshot,
ValidateState) always wraps the caller context with the per-operation cap from
pkg/defaults (or AgentConfig.Timeout for CollectSnapshot), and that the
effective deadline used is the smaller of the caller's deadline and the facade's
cap.

---

Duplicate comments:
In `@aicr_test.go`:
- Around line 131-138: The test creates two real clients (clientA and clientB)
via aicr.NewClient but never closes them, leaking per-client cache/state; ensure
both clients are closed—either immediately defer clientA.Close() after
successful creation of clientA and defer clientB.Close() after successful
creation of clientB, or explicitly call clientA.Close() and clientB.Close() at
test teardown so any per-client resources are released; update the test to call
the Close method on both clientA and clientB (references: aicr.NewClient,
clientA, clientB).
- Around line 198-203: Capture the error returned by client.Close() in the
goroutine and assert it succeeds instead of discarding it; replace `_ =
client.Close()` with saving the result (e.g., `err := client.Close()`) and fail
the test on non-nil (using t.Fatalf/require.NoError) so the concurrency
lifecycle test (the goroutine that calls client.Close() and the wg
synchronization) will surface any future shutdown/drain regressions.
- Around line 392-398: The test currently only checks for a non-nil error from
client.BundleComponents; change it to assert the returned error encodes the
specific contract error ErrCodeInvalidRequest. After calling
client.BundleComponents with &aicr.RecipeResult{Components:nil}, replace the
single non-nil check with logic that extracts the AICR error (e.g., using
errors.As to cast to the package error type or the test project's helper used
elsewhere) and assert its code equals aicr.ErrCodeInvalidRequest so the
closed-client branch is explicitly validated.

In `@aicr.go`:
- Around line 620-633: The helper loadManifestFiles should honor cancellation:
add a context.Context parameter to loadManifestFiles and in its loop over
ref.ManifestFiles check ctx.Done (or call ctx.Err()) before each read to abort
early; update all callers (e.g., BundleComponents where
loadManifestFiles(r.internal, dp, facade.Name) is invoked in the "manifests" and
"kustomize" branches) to pass the incoming ctx (or an appropriate derived
context) so that canceled requests stop reading files immediately; ensure the
function returns promptly with ctx.Err() when canceled.

In `@options.go`:
- Around line 55-60: The doc comment on applyValidateOptions incorrectly claims
the translation occurs "inside the locked region"; update the comment to state
that applyValidateOptions is called after Client.mu is released (from
ValidateState) so the translation happens outside the lock, and adjust wording
to avoid implying any synchronization guarantees—reference applyValidateOptions
and ValidateState (and Client.mu) in the text so future readers know where the
call occurs.

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 `@options.go`:
- Around line 119-121: The WithValidation* option factories
(WithValidationImagePullSecrets, WithValidationTolerations,
WithValidationNodeSelector) must defensively clone caller-provided slices/maps
before storing them on validateConfig to avoid data races and preserve
nil-vs-empty semantics: for each slice argument, if it is nil leave nil,
otherwise allocate a new slice with make and copy(...) the contents and assign
that copy to c.imagePullSecrets / c.tolerations; for each map argument, if it is
nil leave nil, otherwise allocate a new map and copy all key/value pairs (for
k,v := range src { dst[k]=v }) and assign that copy to c.nodeSelector; this
ensures downstream code (e.g., serializeNodeSelector in pkg/api/validator/v1)
does not observe concurrent mutations.