feat(sdk): bind AICRConfig to the facade for verify and recipe - #2243
Merged
Merged
Conversation
AICRConfig was CLI-only: pkg/config appeared across pkg/cli and nowhere in pkg/client/v1, so a team that had standardized on a committed document could not consume it from their own tooling. Worse, the flags-over-config precedence rule lived only in the CLI, so any non-CLI consumer reimplemented it and the two could silently disagree about the effective settings for the same file. Adds a facade-owned Config: LoadConfig (path or HTTP(S) URL), WrapConfig for documents parsed elsewhere, Unwrap, and per-section derivations for spec.verify and spec.recipe. Config DERIVES options rather than applying them, and does not attach to a Client. The facade's options are plain structs, so a field left at its zero value is indistinguishable from one a caller set to zero deliberately -- there is no equivalent of the CLI's cmd.IsSet. An implicit merge would have to guess, and would hand the config's value back to a caller who had deliberately cleared a setting. Deriving makes precedence one readable line at the call site, and mirrors what the CLI already does: build from config, then let an explicitly-set flag win. The flag half necessarily stays in pkg/cli, the only layer that knows. Every method is nil-safe, because the CLI derives unconditionally before it knows whether --config was supplied. Routes the CLI's spec.verify and spec.recipe reads through the facade: bundle_verify.go, root.go (recipe source), query.go, recipe.go, and mirror.go. The two remaining cfg.Recipe() calls are OutputPath and OutputFormat -- CLI presentation, deliberately not facade surface. Exports ToInternalCriteria alongside the existing ToInternalAllowLists. applyCriteriaFromConfig merges into a *recipe.Criteria whose fields are enum types, so a caller deriving criteria from a config needs a supported way back rather than reconstructing the enums by hand. RecipeProfile and RecipeAccountingMode are raw accessors that overlap RecipeResolveOptions on purpose: the options form suits SDK callers, while a caller applying its own precedence first needs the raw value. A test asserts the two agree. Scoped to verify and recipe as agreed. spec.bundle, spec.validate, and spec.snapshot follow in a second PR; deriving all five at once would be ~80 fields of mapping in one review. Test fixtures are per-section because the schema requires it: spec.recipe.criteria and spec.recipe.input.snapshot are mutually exclusive and a document must carry at least one section, so a single "everything" fixture is not a valid AICRConfig. That also exercises the realistic case of a document configuring some sections and not others. Fixes: #2026 Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
added
area/sdk
theme/community
Contributor
|
🌿 Preview your docs: https://nvidia-preview-feat-sdk-config-binding.docs.buildwithfern.com/aicr |
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Blocking review finding on #2243: a config naming a criteria value that exists only in an external catalog could not be loaded at all. config.Load validated criteria against a nil registry -- the EMBEDDED catalog -- before spec.recipe.data could construct the provider whose registry defines the value. Reproduced on the public path with an overlay contributing service: ncp-review: LoadConfig ... [INVALID_REQUEST] invalid service type: ncp-review That made the provider-aware RecipeCriteria(client.CriteriaRegistry()) path this PR documents unreachable: you could never get past loading to build the Client whose registry knows the value. Membership is now checked where a registry exists. Every consumer of spec.recipe.criteria already routes through ResolveCriteriaWithRegistry -- the facade's RecipeCriteria and the CLI's applyCriteriaFromConfig -- so a bogus value still fails closed, against the right catalog rather than the wrong one. Nodes stays a load-time check: a negative count is malformed regardless of catalog, so there is nothing to defer. The eager cases in TestValidate_Errors are relocated rather than dropped. TestValidate_DefersCriteriaMembership now pins both halves -- Validate accepts an external value, ResolveCriteriaWithRegistry still rejects an unknown one -- so the deferral cannot quietly become silent acceptance. TestResolveCriteria_InvalidEnums already covered the per-field membership errors. Adds the requested regression test over the whole chain, since each hop is where it previously broke: LoadConfig -> RecipeSource -> NewClient -> LoadCatalog -> RecipeCriteria, against a fixture overlay contributing a service the embedded catalog does not know. That test also pins the strict-mode interaction, which is why it first failed under make test and passed in isolation: the suite runs with AICR_CRITERIA_STRICT=1, and strict mode exists precisely to fence off externally-contributed values. The test disables it deliberately for the accept case and re-enables it to assert the reject case, rather than asserting the opposite of what strict mode is for. Two further review points: - loadFacadeConfig called pkg/config.Load through loadCmdConfig, keeping the parallel loader path this issue exists to remove. It now calls aicr.LoadConfig, so validation and error handling cannot drift between what the CLI sees and what an SDK consumer sees. - The loader-guard test claimed error codes survive but only checked for non-nil. It now asserts ErrCodeInvalidRequest for a malformed document and ErrCodeNotFound for a missing file. Signed-off-by: Mark Chmarny <mark@chmarny.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Four review findings on #2243. recipeCmd and five other commands still reached pkg/config.Load through loadCmdConfig, so a second loader path survived the previous fix. That matters more than it reads: the blocking finding on this PR was itself a validation divergence between two phases, and a second loader is how that class of bug persists unnoticed. Fixed at loadCmdConfig rather than at the six call sites -- it now delegates to loadFacadeConfig and unwraps, so every config-aware command routes through aicr.LoadConfig with no call-site churn. Nothing in pkg/cli calls config.Load any more. The call sites keep *config.AICRConfig deliberately. They thread it through ~10 helper signatures for the spec.bundle / spec.validate / spec.snapshot sections the facade does not project yet, so converting now would sprinkle Unwrap() -- the escape hatch this PR's own godoc flags as a missing-derivation signal -- and then remove it again in the follow-up. The divergence risk closes in the loader regardless of type. The integrator guide advertised the provider-aware path while omitting the step that makes it work: without LoadCatalog the registry is never seeded with overlay-contributed values, so RecipeCriteria rejects exactly the value the example exists to demonstrate. Same defect class as the blocking finding, and anyone following it verbatim would conclude the fix had not worked. The LoadConfig godoc had the right order; the guide now matches it, and matches the regression test. A comment in bundle_verify attributed the identity-pattern and IgnoreTLog/Key validation to the config derivation. VerifyBundle is what performs both; the derivation projects spec.verify and nothing else, and IgnoreTLog has no config field at all. Reworded to say what each layer does, including why the pairing check is repeated in the CLI -- to word the message in flags rather than struct fields, which otherwise reads as redundant. The ToInternalCriteria test left Platform empty, so that branch of the mapping never ran and a silent drop would have passed. The fixture now sets it and both tests assert it; verified by mutation, since commenting out the mapping previously changed nothing. Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
marked this pull request as ready for review
August 18, 2026 16:20
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mchmarny
enabled auto-merge (squash)
August 18, 2026 17:06
njhensley
approved these changes
Aug 18, 2026
njhensley
left a comment
njhensley
left a comment
Member
There was a problem hiding this comment.
Multi-persona review — Approve with comments
Method: 4 independent persona reviewers (Correctness · Security · Domain/Architecture · Test-coverage) → adversarial senior meta-reviewer re-derived every finding from the resolved code at df3b4c47. Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick.
Overall assessment
A careful, well-argued refactor that holds up. The correctness pass found the CLI rewiring like-for-like at every site — same values, same flag-over-config precedence, same error codes — including the two spots most likely to hide a bug: the recipeClientFromCmd restructure in root.go and the mode string-conversion change in query.go. The security pass confirmed the load-time → consumption-time criteria-membership deferral stays fail-closed on all three consuming commands (recipe/query/mirror each LoadCatalog then resolve through the per-provider registry; the server never consumes config criteria; a value in no catalog is still rejected at consume time), and that the IgnoreTLog / trust-floor invariants are preserved (no config field, empty MinTrustLevel still defaults to max, config-supplied identity regexp gets the same ValidateIdentityPattern gate as the flag).
No blockers, no majors. One real 🟡 (a godoc grouping defect on the new public symbol) plus a few 🔵 polish items, all inline.
Additional nitpick (not anchored to a changed line)
- 🔵 Residual spec.recipe reads bypass the facade —
recipeOutputPath(pkg/cli/recipe.go:267) andparseRecipeOutputFormat(277) still readcfg.Recipe().OutputPath()/.OutputFormat()directly on the unwrapped*config.AICRConfig, while criteria/data/profile/accounting reads moved to the facade. These are spec.recipe reads just as projectable as the ones that moved, so the boundary is non-uniform. Defensible as a staged split (consistent with the deferred bundle/validate/snapshot sections) — worth either projecting them too or notingspec.recipe.outputas intentionally deferred.
Pre-existing follow-up (out of scope for this PR)
- 🔵
criteriaStricthonored only byaicr recipe, notquery/mirror(pkg/cli/query.go,pkg/cli/mirror.go). A config pairingcriteriaStrict: truewith an external--dataoverlay resolves the external value onquery/mirror(strict never applied there). This is real but pre-existing — pre-PRrecipe.goalready applied strict andquery/mirrornever referenced it; this PR only swaps the accessor. The membership deferral makes it observable but is not a regression. Worth a separate issue.
Confirmed non-issues (examined, not defects)
- CLI rewiring behaviorally equivalent at every site (values, precedence, error codes).
- Criteria deferral fail-closed on every consumption path; strict-mode fencing verified.
- Trust floor intact: no
IgnoreTLogconfig field, emptyMinTrustLevel→max, config identity regexp validated. loadCmdConfignow routes throughaicr.LoadConfig; noconfig.Load(remains inpkg/cli(single-loader goal met).- Docs snippets match real signatures and the mandatory
LoadConfig → NewClient → LoadCatalog → RecipeCriteriaordering. - Every new exported method has an exercising + nil-Config test; deferral regression tests pin both halves (accept-at-load, reject-at-consume).
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick | Pre-existing |
|---|---|---|---|---|
| 0 | 0 | 1 | 4 | 1 |
Nothing blocks merge; the 🟡 godoc fix is a one-line blank-line move on the exported SDK symbol and is the only item I'd land before merge.
This was referenced Aug 18, 2026
7 tasks done
13 of 25 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a facade-owned
Config—LoadConfig,WrapConfig,Unwrap, and per-section derivations forspec.verifyandspec.recipe— and routes the CLI's reads of those two sections through it.Motivation / Context
AICRConfigwas CLI-only:pkg/configappeared acrosspkg/cliand nowhere inpkg/client/v1, so a team that had standardized on a committed document could not consume it from their own tooling.The sharper problem is correctness, not ergonomics. The flags-over-config precedence rule lived only in the CLI, so any non-CLI consumer reimplemented it — and the two could silently disagree about the effective settings for the same file.
Fixes: #2026
Related: #2016, #2024
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)pkg/client/v1),pkg/configImplementation Notes
Config derives options; it never applies them. A
Configdoes not attach to aClientand is never consulted implicitly. Each method returns a populated value the caller may then override.That is the load-bearing decision. The facade's options are plain structs, so a field left at its zero value is indistinguishable from one a caller set to zero deliberately — there is no equivalent of the CLI's
cmd.IsSet. An implicitWithConfigmerge would have to guess, and would hand the config's value back to a caller who had deliberately cleared a setting. Fixing that properly would mean pointer-wrapping already-shipped option structs.Deriving keeps precedence to one readable line at the call site, and mirrors what the CLI already does: build from config, then let an explicitly-set flag win.
The flag half necessarily stays in
pkg/cli.stringFlagOrConfigand friends are flag-aware by construction; only that layer knows whether a flag was set. So #2026's "no parallel application path" is achieved for loading, resolving, and mapping — not for precedence, which cannot move.Every method is nil-safe, because the CLI derives unconditionally before it knows whether
--configwas supplied.Wrapper, not alias.
AICRConfigis a versioned YAML schema with ~30 nested types; aliasing would freeze all of them under the API-diff gate, and Go cannot attach methods to an alias of another package's type anyway.Unwrap()keeps the raw document reachable, with a godoc note that needing it signals a missing derivation.ToInternalCriteriais exported alongside the existingToInternalAllowLists.applyCriteriaFromConfigmerges into a*recipe.Criteriawhose fields are enum types, so a caller deriving criteria from a config needs a supported way back rather than reconstructing the enums by hand.RecipeProfile/RecipeAccountingModeoverlapRecipeResolveOptionson purpose. The options form suits SDK callers; a caller applying its own precedence first — the CLI overlaying an explicitly-set flag — needs the raw value. A test asserts the two agree.Scoped to verify and recipe, as agreed.
spec.bundle,spec.validate, andspec.snapshotfollow in a second PR; deriving all five at once is ~80 fields of mapping in a single review.Test fixtures are per-section because the schema requires it. My first attempt used one "everything" document and the config validator rejected it:
spec.recipe.criteriaandspec.recipe.input.snapshotare mutually exclusive, and a document must carry at least one section. The validation was right and the fixture was wrong — splitting them also exercises the realistic case of a document configuring some sections and not others.Testing
make qualify # exit 0New
pkg/client/v1/config_test.gocovers the verify mapping field-by-field, the recipe derivations, the nil-Configcontract, section-absent-derives-zero in both directions, loader error propagation, the raw accessors agreeing with the options form, and theToInternalCriteriaround trip.Coverage:
pkg/client/v183.2% → 83.6% (+0.4%),pkg/cli74.8% → 74.8% (0.0%). No new exported function is below 83%;LoadConfig,WrapConfig,Unwrap,RecipeSource,RecipeProfile,SnapshotPath,IsCriteriaStrict, andToInternalCriteriaare at 100%.Risk Assessment
Additive on the facade. The risk is the CLI rewiring across five files, but each site is a like-for-like swap: same values, same precedence, same errors. The one behavioral subtlety is
recipeClientFromCmd, where--datastill wins overspec.recipe.dataexactly as before.Rollout notes: No migration.
pkg/configremains importable; this adds a supported path without removing one.Checklist
make testwith-race)make lint)git commit -S)