feat(recipe): push owner-token guard down to pkg/recipe.RecipeResult by mchmarny · Pull Request #1113 · NVIDIA/aicr

mchmarny · 2026-05-30T00:23:12Z

Summary

Push the cross-Builder safety guard from the aicr facade down to recipe.RecipeResult, so direct pkg/recipe.Builder importers get the same protection the facade callers already have.

Motivation / Context

pkg/client/v1.assertOwns in aicr.go rejects a RecipeResult produced by Client A from being passed to Client B.BundleComponents / ValidateState. That guard lives only at the facade layer; direct pkg/recipe.Builder consumers (which the pkg/recipe package's "Public (evolving)" tier explicitly allows) get no protection. After the per-Builder isolation work in #1107, any external project that constructs two Builders today (e.g., for tenant isolation) hits the same bug class once they trust the per-Builder routing.

Push the guard to the layer where the data lives so it covers both call paths.

Fixes: #1080
Related: #1076 (epic — this is the last open child)

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Refactoring (no functional changes)
Build/CI/tooling

Component(s) Affected

CLI (cmd/aicr, pkg/cli)
API server (cmd/aicrd, pkg/api, pkg/server)
Recipe engine / data (pkg/recipe)
Bundlers (pkg/bundler, pkg/component/*)
Collectors / snapshotter (pkg/collector, pkg/snapshotter)
Validator (pkg/validator)
Core libraries (pkg/errors, pkg/k8s)
Docs/examples (docs/, examples/)
Other:

Implementation Notes

recipe.RecipeResult gains unexported owner *Builder field next to the existing provider field. Pointer identity is the token — zero-cost, naturally unique, unforgeable from outside the package.
Builder.buildWithStore stamps result.owner = b before returning, covering both BuildFromCriteria and BuildFromCriteriaWithEvaluator via the shared code path.
New (*RecipeResult).Owner() *Builder returns the producer for comparison.
New (*RecipeResult).AssertOwnedBy(b *Builder) error returns nil for the producer; ErrCodeInvalidRequest (with producer/expected %p pointers in context) for any other Builder. Nil-safe on the receiver (vacuously owned); nil b argument is rejected (must specify what to assert against); nil owner on a non-nil result is rejected (no provenance — e.g., LoadFromFile path).
DeepCopy leaves owner nil — mirrors how provider is also left nil. Adopted-recipe flows (AdoptRecipe) cannot pass deep-copies back through ownership-checked entry points without re-building.

No facade or in-tree caller changes. The existing pkg/client/v1.assertOwns continues to function as before; the new recipe-level check is additive defense-in-depth for external pkg/recipe.Builder importers and a foundation for future pkg/recipe-level enforcement (e.g., GetValuesForComponent checks).

Testing

go test -race ./pkg/recipe/...   # pass
make lint                         # 0 issues

New tests in pkg/recipe/builder_test.go:

TestRecipeResult_OwnerStampedByBuilder — happy path.
TestRecipeResult_AssertOwnedBy_RejectsCrossBuilder — analog of pkg/client/v1.TestClient_CrossClientBundleRejected; exercises the rejection without going through the facade (satisfies the issue's test acceptance criterion).
TestRecipeResult_AssertOwnedBy_NilCases — pins documented nil handling.
TestRecipeResult_DeepCopy_DropsOwner — pins the deep-copy contract.

Risk Assessment

Low — Additive change. New unexported field, new methods, no signature changes on existing exports. No in-tree caller updates required. Facade-level guard untouched.
Medium
High

Rollout notes: External Go importers that hold *pkg/recipe.Builder can now call result.AssertOwnedBy(b) before consuming a RecipeResult to verify provenance. Recommended but optional; existing call patterns continue to work.

Checklist

Tests pass locally (go test -race ./pkg/recipe/...)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added tests for the new functionality
No user-facing behavior change; docs unchanged
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

coderabbitai · 2026-05-30T00:25:59Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: dd4cbf34-3b66-4cc9-abdb-e7fc567bcfd5

📥 Commits

Reviewing files that changed from the base of the PR and between 56ef865 and f6f901b.

📒 Files selected for processing (3)

pkg/recipe/builder.go
pkg/recipe/builder_test.go
pkg/recipe/metadata.go

📝 Walkthrough

Walkthrough

This PR implements an ownership-token guard on RecipeResult to prevent cross-Builder result mixing. The RecipeResult struct gains an unexported owner field that is stamped by the producing Builder in buildWithStore. Two new methods provide the enforcement: Owner() returns the stamped Builder identity, and AssertOwnedBy() rejects results from different Builders or those constructed outside the Builder. DeepCopy() intentionally drops the owner stamp. Test coverage validates stamping, cross-Builder rejection with proper error codes, nil-case semantics, and the owner-dropping behavior of DeepCopy().

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

#1076: Implements the same owner-token guard on pkg/recipe.RecipeResult; this PR adds the owner field, stamping, and checks described in that issue.
#1115: Both changes operate on RecipeResult provenance/ownership semantics and interact with facade wrappers that carry internal RecipeResult tokens, so the proposals overlap.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically identifies the main change: pushing an owner-token guard from the facade down to pkg/recipe.RecipeResult.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, covering motivation, implementation details, testing, and risk assessment.
Linked Issues check	✅ Passed	The PR substantially addresses all key acceptance criteria from `#1080`: stamping owner field [`#1080`], checking ownership in assertions [`#1080`], test coverage for rejection [`#1080`], and nil-safe handling [`#1080`].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issue objectives: adding owner field to RecipeResult, stamping by Builder, ownership assertion methods, and test coverage.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/recipe-owner-token-pushdown

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-05-30T00:35:28Z

Coverage Report ✅

Metric	Value
Coverage	76.8%
Threshold	75%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-76.8%25-green)

No Go source files changed in this PR.

Closes #1080. Add an unexported owner *Builder field to recipe.RecipeResult, stamped by Builder.buildWithStore at build time, plus an AssertOwnedBy(b *Builder) method that returns ErrCodeInvalidRequest when the result was produced by a different Builder (or constructed outside Builder). This pushes the cross-Builder safety guard from the aicr facade (pkg/client/v1.assertOwns in aicr.go) down to the layer where the data lives, so direct pkg/recipe.Builder importers get the same protection. The facade-level check stays as a friendlier error at the facade boundary; this adds defense-in-depth at the recipe layer. Implementation: - recipe.RecipeResult gains unexported owner *Builder field next to the existing provider field. Pointer identity is the token — zero-cost, naturally unique, unforgeable from outside the package. - Builder.buildWithStore stamps result.owner = b before returning, covering both BuildFromCriteria and BuildFromCriteriaWithEvaluator via the shared codepath. - (*RecipeResult).Owner() returns the producer Builder for comparison. - (*RecipeResult).AssertOwnedBy(b) returns nil for the producer, ErrCodeInvalidRequest with the producer/expected %p pointers in context for any other Builder. Nil-safe on the receiver (vacuously owned); nil b argument is rejected (must specify what to assert against); nil owner on a non-nil result is rejected (no provenance, e.g., LoadFromFile path). - DeepCopy leaves owner nil so adopted-recipe flows (AdoptRecipe) cannot pass deep-copies back through ownership-checked entry points without re-building, mirroring how provider is also left nil. Tests in pkg/recipe/builder_test.go: - TestRecipeResult_OwnerStampedByBuilder — happy path. - TestRecipeResult_AssertOwnedBy_RejectsCrossBuilder — analog of pkg/client/v1's TestClient_CrossClientBundleRejected, exercises the rejection without going through the facade. - TestRecipeResult_AssertOwnedBy_NilCases — pins documented nil handling. - TestRecipeResult_DeepCopy_DropsOwner — pins the deep-copy contract. No facade or in-tree caller changes. Existing aicr.assertOwns continues to function as before; the new recipe-level check is additive defense for external pkg/recipe.Builder importers and a foundation for any future pkg/recipe-level enforcement (e.g., GetValuesForComponent checks).

mchmarny requested a review from a team as a code owner May 30, 2026 00:23

mchmarny added the enhancement label May 30, 2026

mchmarny self-assigned this May 30, 2026

github-actions Bot added the size/M label May 30, 2026

mchmarny mentioned this pull request May 30, 2026

[Epic]: Post-facade adoption and surface hardening (follow-ups to #1072) #1076

Closed

mchmarny mentioned this pull request May 30, 2026

refactor(validator): move v1 schema from pkg/api/validator/v1 to pkg/validator/v1 #1117

Merged

25 tasks

mchmarny force-pushed the feat/recipe-owner-token-pushdown branch from 56ef865 to f6f901b Compare May 30, 2026 00:49

mchmarny merged commit cb098fa into main May 30, 2026
26 of 27 checks passed

mchmarny deleted the feat/recipe-owner-token-pushdown branch May 30, 2026 00:50

coderabbitai Bot mentioned this pull request May 30, 2026

feat(client/v1): wrap remaining facade alias types #1124

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(recipe): push owner-token guard down to pkg/recipe.RecipeResult#1113

feat(recipe): push owner-token guard down to pkg/recipe.RecipeResult#1113
mchmarny merged 1 commit into
mainfrom
feat/recipe-owner-token-pushdown

mchmarny commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026 •

edited

Loading

Review failed

Walkthrough

Estimated code review effort

Possibly related issues

Uh oh!

github-actions Bot commented May 30, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mchmarny commented May 30, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

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

Uh oh!

Review failed

Walkthrough

Estimated code review effort

Possibly related issues

Uh oh!

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

Uh oh!

Coverage Report ✅

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented May 30, 2026 •

edited

Loading

github-actions Bot commented May 30, 2026 •

edited

Loading