feat(aicr): wrap facade alias types as facade-owned structs by mchmarny · Pull Request #1111 · NVIDIA/aicr

mchmarny · 2026-05-29T22:55:33Z

Summary

Replace the transparent type aliases on the aicr Public (stable) facade with facade-owned structs and translate at the boundary, so internal pkg/snapshotter and pkg/validator type-shape changes no longer propagate to external Go importers without notice.

Motivation / Context

Per docs/integrator/public-api.md, the github.com/NVIDIA/aicr facade is the Public (stable) contract — but Snapshot, AgentConfig, PhaseResult, Phase, and the phase constants were transparent type aliases to Public (evolving) packages. Any field-shape change in the source packages propagated to consumers without a minor-version signal, undermining the semver promise. This PR closes that gap so every type reachable from the stable surface is now facade-owned, with translation handled at each method boundary.

Fixes: #1078
Related: #1076 (epic)

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: top-level facade (aicr/types.go, aicr/translate.go, aicr/aicr.go)

Implementation Notes

Per-type translation shape (issue #1078 recommended option 1: wrap immediate type, drop transitively-referenced internal types):

Type	Strategy
`Snapshot`	Public fields (`APIVersion`, `Kind`, `CapturedAt` parsed from `metadata.timestamp`) + unexported `internal *snapshotter.Snapshot` for zero-copy round-trip through `ValidateState`. Same pattern as existing `RecipeResult.internal`.
`AgentConfig`	Field-for-field mirror of `snapshotter.AgentConfig`. `Tolerations` kept as `[]corev1.Toleration` since `k8s.io/api/core/v1` is itself a stable contract.
`PhaseResult`	Public fields plus `Summary` (CTRF counts, ergonomic pass/fail check) and `RawReport []byte` (marshaled CTRF JSON for callers needing per-test detail). Drops the `*ctrf.Report` Go-type coupling — callers decode `RawReport` against the CTRF spec at `https://ctrf.io`.
`Phase` + 3 constants	Facade-owned `type Phase string` with constant values matching `pkg/api/validator/v1` verbatim for byte-identical wire round-trip with the validator.
`ReportSummary`	New facade-owned type with the CTRF count fields.

Translation boundary: CollectSnapshot translates the return via fromInternalSnapshot; ValidateState translates snap in via toInternalSnapshot (preserves internal pointer when set; rebuilds from public fields when nil) and the return slice via fromInternalPhaseResults. Helpers live in translate.go.

Behavior: No consumer-facing behavior change for the in-tree CLI / REST. The wire format on disk (snapshot YAML, CTRF JSON) is unchanged — the facade owns Go-type identity, not the wire schema.

Testing

make test    # pass (modulo pre-existing pkg/trust sandbox-network failure unrelated to this PR)
make lint    # 0 issues

New tests in translate_internal_test.go cover every translation helper:

toInternalAgentConfig: nil-safety + field-by-field mirror across all 20 fields
fromInternalSnapshot / toInternalSnapshot: nil-safety, wrap-and-unwrap pointer preservation, rebuild-from-public-fields fallback, RFC3339 timestamp parsing (including unparseable-leaves-zero case)
fromInternalPhaseResults / fromInternalPhaseResult: nil/empty contract, slice ordering, dual CTRF Summary + RawReport population, nil-Report path, value-pinning of phase constants to the validator's wire format

Coverage delta on ./. (the aicr package): 63.3% → 66.0% (+2.7%). All new functions in translate.go are at 100% except fromInternalPhaseResult at 90% (the json.Marshal error branch is not realistically reachable). No new exported functions with 0% coverage.

Risk Assessment

Low — Isolated to the facade boundary. No wire-format change. All in-tree tests pass. Translation helpers are pure functions with explicit nil-safety and full field-mirror tests.
Medium
High

Rollout notes: No migration required for in-tree CLI / REST callers — pkg/cli and pkg/api/server.go either don't reach into these types or will adopt the facade-owned shape transparently via #1108. External Go importers that pinned aicr.Snapshot's fields will see the facade now exposes a narrower public surface (no direct access to Measurements / Fingerprint / header.Header). Consumers needing those should import pkg/snapshotter directly; the recommendation is documented in the updated docs/integrator/public-api.md.

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
I updated docs if user-facing behavior changed (docs/integrator/public-api.md, docs/integrator/go-library.md)
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

github-actions · 2026-05-29T22:56:37Z

🌿 Preview your docs: https://nvidia-preview-feat-aicr-facade-owned-types.docs.buildwithfern.com/aicr

coderabbitai · 2026-05-29T22:59:55Z

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR migrates the NVIDIA AICR facade package from transparent type aliases to facade-owned wrapper structs with a bidirectional translation layer. The Snapshot, AgentConfig, PhaseResult, and Phase types move from direct aliases of internal packages (pkg/snapshotter, pkg/validator) to explicit facade-owned definitions. A new translation layer in pkg/client/v1/translate.go converts between facade and internal types at method boundaries. All consumers—API handlers, CLI commands, and tests—are switched to import from the stable pkg/client/v1 package instead of the unversioned pkg/aicr. Documentation is updated to clarify the ownership model and direct users to pkg/client/v1.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

[Epic]: Post-facade adoption and surface hardening (follow-ups to #1072) #1076: This PR implements the facade adoption and type-wrapping strategy called for by the epic.

Suggested labels

area/api, area/validator

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat(aicr): wrap facade alias types as facade-owned structs' clearly and specifically describes the main change: replacing transparent type aliases with facade-owned wrapper types, which is the primary objective of this PR.
Description check	✅ Passed	The PR description comprehensively explains the motivation, implementation strategy, testing performed, and risk assessment, all directly related to the type wrapping changes in the changeset.
Linked Issues check	✅ Passed	The PR fully satisfies issue `#1078` acceptance criteria: Snapshot, AgentConfig, PhaseResult, Phase, and phase constants are now facade-owned types [types.go]; translation logic is implemented in translate.go and used at facade boundaries [aicr.go, cli files]; docs updated [public-api.md, go-library.md]; all tests pass with no behavioral regressions.
Out of Scope Changes check	✅ Passed	All code changes directly support the facade type wrapping objective: type definitions, translation helpers, import updates across CLI/API, and documentation changes are all scoped to establishing the new facade ownership model without introducing unrelated functionality.

_{✏️ 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/aicr-facade-owned-types

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

github-actions · 2026-05-29T23:05:18Z

Coverage Report ✅

Metric	Value
Coverage	76.7%
Threshold	75%
Status	Pass

Coverage Badge

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

No Go source files changed in this PR.

dims

LGTM

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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/aicr/translate.go`:
- Around line 104-107: The fallback snapshot reconstruction only copies
APIVersion and Kind and drops CapturedAt; update the code that builds the
snapshot (the block creating out := &snapshotter.Snapshot{} in the fallback
path) to also preserve the timestamp by assigning out.CapturedAt = s.CapturedAt
(i.e., copy s.CapturedAt into the snapshot before returning) so
Snapshot.RoundTrip of public fields remains faithful.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: df0dbcaa-3ce6-4e12-8a87-a9c50e69caef

📥 Commits

Reviewing files that changed from the base of the PR and between 866c3be and 50458bb.

📒 Files selected for processing (6)

docs/integrator/go-library.md
docs/integrator/public-api.md
pkg/aicr/aicr.go
pkg/aicr/translate.go
pkg/aicr/translate_internal_test.go
pkg/aicr/types.go

Two coordinated changes shipped together so external Go importers migrate once: **1. Move pkg/aicr → pkg/client/v1** The previous pkg/aicr/ location read ambiguously (github.com/NVIDIA/aicr/pkg/aicr — duplicative). The new path follows the conventional Go SDK shape: pkg/client/v1/ with versioned `v1` signaling semver commitment, package name still `aicr`. Matches the existing pkg/api/validator/v1 pattern. External importers: - import aicr "github.com/NVIDIA/aicr/pkg/aicr" + import aicr "github.com/NVIDIA/aicr/pkg/client/v1" In-tree consumers (pkg/cli, pkg/api) updated mechanically. **2. Wrap facade alias types as facade-owned structs (closes #1078)** Replace transparent type aliases on the Public (stable) facade with facade-owned structs and translate at the boundary, so internal pkg/snapshotter and pkg/validator type-shape changes no longer propagate to external Go importers without notice. - Snapshot: facade-owned struct (APIVersion, Kind, CapturedAt) + unexported internal *snapshotter.Snapshot for zero-copy round-trip through ValidateState. Same pattern as RecipeResult.internal. WrapSnapshot(*snapshotter.Snapshot) exported so callers that load snapshots externally (e.g., CLI loading from YAML) can lift. - AgentConfig: facade-owned struct mirroring every field of snapshotter.AgentConfig. Tolerations keep k8s.io/api/core/v1 type. - PhaseResult: facade-owned struct with Phase/Status/Duration plus Summary (CTRF count breakdown), RawReport (marshaled CTRF JSON), and Report (typed *ctrf.Report retained for in-tree consumers that merge per-phase reports via ctrf.MergeReports). - Phase / phase constants: facade-owned typedef and string consts whose values match pkg/api/validator/v1 for byte-identical wire round-trip. - ReportSummary: new facade-owned type with CTRF count fields. CollectSnapshot and ValidateState translate at the boundary via toInternalAgentConfig / fromInternalSnapshot / toInternalSnapshot / fromInternalPhaseResults. The Recipe/AllowLists/Criteria/CriteriaRegistry aliases (introduced by #1077) remain aliases for now; #1078's scope covered the snapshotter/validator-tier aliases. docs/integrator/public-api.md updated to reflect the new path and the facade-ownership state. go-library.md updated. Closes #1078.

mchmarny requested a review from a team as a code owner May 29, 2026 22:55

mchmarny added the enhancement label May 29, 2026

mchmarny self-assigned this May 29, 2026

github-actions Bot added area/docs size/XL labels May 29, 2026

dims previously approved these changes May 29, 2026

View reviewed changes

mchmarny mentioned this pull request May 29, 2026

refactor(aicr): move public facade from module root to pkg/aicr/ #1112

Merged

25 tasks

mchmarny dismissed dims’s stale review via 50458bb May 29, 2026 23:29

mchmarny force-pushed the feat/aicr-facade-owned-types branch from 866c3be to 50458bb Compare May 29, 2026 23:29

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

Comment thread pkg/client/v1/translate.go

mchmarny force-pushed the feat/aicr-facade-owned-types branch from 50458bb to fe3e5c3 Compare May 30, 2026 00:08

github-actions Bot added area/cli area/api size/L and removed size/XL labels May 30, 2026

mchmarny merged commit 5c63c10 into main May 30, 2026
31 of 32 checks passed

mchmarny deleted the feat/aicr-facade-owned-types branch May 30, 2026 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(aicr): wrap facade alias types as facade-owned structs#1111

feat(aicr): wrap facade alias types as facade-owned structs#1111
mchmarny merged 1 commit into
mainfrom
feat/aicr-facade-owned-types

mchmarny commented May 29, 2026

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 2026 •

edited

Loading

Review failed

Walkthrough

Estimated code review effort

Possibly related issues

Suggested labels

Uh oh!

github-actions Bot commented May 29, 2026 •

edited

Loading

Uh oh!

dims left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mchmarny commented May 29, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 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

Suggested labels

Uh oh!

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

Uh oh!

Coverage Report ✅

Uh oh!

dims left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 29, 2026 •

edited

Loading

github-actions Bot commented May 29, 2026 •

edited

Loading