fix(recipe): enforce stated-criteria coverage on recipe resolution#1784
Conversation
Recipe resolution now fails with ErrCodeInvalidRequest (HTTP 400) when a query-stated criteria dimension (service, accelerator, intent, os, platform) is not honored by any applied overlay, instead of silently returning a partial recipe that echoes criteria it did not apply. - New coverage post-condition runs on the final applied set of both BuildRecipeResult and BuildRecipeResultWithEvaluator (after constraint exclusion and the mixin-failure fallback rebuild). - Errors carry actionable minimal completion tuples and a machine-readable details.uncovered context array. - requireOSIfNeeded stays and runs first: per-dimension coverage does not subsume its joint service+accelerator OS gating. - Constraint evaluation fails closed on non-NotFound evaluator errors (codes preserved); NotFound remains the designed graceful exclusion. - nodes is exempt from coverage (advisory; no overlay gates on it). - Golden projection matrix (415 entries) pins every projection outcome of the embedded overlay data against silent drift. Fixes: #1542 Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
added
the
theme/recipes
|
🌿 Preview your docs: https://nvidia-preview-fix-1542-criteria-coverage.docs.buildwithfern.com/aicr |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded criteria-coverage validation for recipe resolution across service, accelerator, intent, OS, and platform dimensions. Resolution now reports structured invalid-request details for uncovered criteria, including completion suggestions and constraint-exclusion context. Constraint evaluation fails closed for non-NotFound errors. Tests, golden fixtures, CLI/API documentation, OpenAPI examples, scripts, and request fixtures were updated accordingly. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@api/aicr/v1/server.yaml`:
- Around line 231-238: Update the GET and POST /v1/query 400-response
documentation to include the structured uncovered-dimension error contract
already described here: document details.uncovered with dimension,
requestedValue, and validCompletions, plus details.excludedOverlays and
details.constraintWarnings when snapshot-driven constraint exclusion occurs, and
add a representative example.
In `@pkg/cli/config_integration_test.go`:
- Around line 132-137: Update the integration test around root.Run to capture
the generated recipe output, then assert that its criteria contains service:
aks. Keep the existing --service override invocation and success assertion,
ensuring the test fails if testRecipeConfig’s default service: eks is used
instead.
In `@pkg/recipe/metadata_store.go`:
- Around line 766-770: Normalize evaluator failures in metadata_store.go at
lines 766-770 and 1167-1194 by passing each non-NotFound constraint error
through aicrerrors.PropagateOrWrap with ErrCodeInternal and the message “failed
to evaluate mixin constraint,” preserving existing structured codes and updating
the overlay comment’s guarantee. In metadata_store_test.go lines 305-339, add
coverage for a generic evaluator error asserting ErrCodeInternal while retaining
coded-error preservation coverage.
🪄 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: 3b0722ae-d947-47db-8ac1-e956e1078148
📒 Files selected for processing (15)
api/aicr/v1/server.yamldocs/contributor/recipe.mddocs/user/api-reference.mddocs/user/cli-reference.mdpkg/cli/config_integration_test.gopkg/client/v1/aicr_test.gopkg/recipe/coverage.gopkg/recipe/coverage_matrix_test.gopkg/recipe/coverage_test.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/testdata/coverage_golden.yamlpkg/server/recipe_handler_test.gopkg/server/serve_test.gotests/chainsaw/cli/config-file/chainsaw-test.yaml
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. |
ConstraintEvaluatorFunc returns a plain error; the fail-closed branches in evaluateOverlayConstraints and evaluateMixinConstraints propagated it as-is, so a non-StructuredError evaluator failure reached the server layer as an uncoded 500. Route both through the existing aicrerrors.PropagateOrWrap so an already-coded error still passes through untouched, and anything else gets ErrCodeInternal. Signed-off-by: Mark Chmarny <mark@chmarny.com>
fingerprint.ToCriteria only ever derives service/accelerator/os from a snapshot (intent/platform always come back "any"), so an overlay tree deliberately agnostic to one of those three dimensions could still trip the criteria-coverage post-condition on a value the snapshot detected but no recipe content distinguishes. buildRecipeFromCmdWithConfig now retries once with any uncovered snapshot-derived dimensions cleared to unstated, logging a warning per relaxed dimension; if any uncovered dimension was user-stated (--config or a CLI flag), the error still propagates unchanged. applyCriteriaFromConfig/applyCriteriaOverrides gained a nil-safe touched-set parameter to track that distinction. This is a caller-layer relaxation only — pkg/recipe stays strict. The retry lives in the shared buildRecipeFromCmdWithConfig helper, so aicr query and aicr mirror inherit it along with aicr recipe. Also strengthens TestRecipeCmd_ConfigFlag_FlagOverride, which previously only asserted the command succeeded (and would have passed even if --service were silently ignored); it now reads the generated recipe back and asserts the override actually landed. Signed-off-by: Mark Chmarny <mark@chmarny.com>
The from-snapshot recipe/validate steps in tests/e2e/run.sh requested --intent training without an explicit --service/--accelerator, relying purely on fingerprint detection. On a GPU-less Kind cluster (fake GPU operator faking device presence at the API level, no real driver data), accelerator never gets derived, and Kind's training overlay requires accelerator=h100 - a genuinely unsatisfiable combination, since intent is user-stated and not eligible for the coverage relaxation. Switch both invocations to --intent inference, which the OS-agnostic kind-inference overlay covers on service=kind alone. Signed-off-by: Mark Chmarny <mark@chmarny.com>
GET and POST /v1/query resolve recipes through the same engine as /v1/recipe, so they surface the same criteria-coverage 400 contract (details.uncovered). Mirror the description and examples onto both verbs, and add a pointer to that contract in the api-reference.md query section, which previously documented no error responses at all. Also documents the CLI-layer relaxation for snapshot-derived dimensions: cli-reference.md's aicr recipe snapshot section and recipe.md's Criteria Coverage Post-Condition section now both call out that the engine stays strict while the CLI's --snapshot path relaxes derived-only coverage failures. Signed-off-by: Mark Chmarny <mark@chmarny.com>
|
Update (round 1 fixes pushed, The first CI run surfaced a real design-consequence failure worth calling out: on the nvkind GPU runners, the snapshot fingerprint detects Amendment (maintainer-approved): the recipe engine stays strict, but the shared CLI snapshot path (recipe/query/mirror) now treats fingerprint-derived dimensions as advisory — when a coverage failure's uncovered dimensions are ALL snapshot-derived (not set by config or flags), they are relaxed with a warning and resolution retries once; any user-stated uncovered dimension still fails hard. Also in this round: unstructured evaluator errors are now coded via |
The Kind overlay tree is deliberately OS-agnostic; --os ubuntu never contributed content and is now correctly rejected by the criteria-coverage post-condition as an uncovered stated dimension. Affects the runtime-install GPU test action and tools/mirror-e2e. Signed-off-by: Mark Chmarny <mark@chmarny.com>
|
GPU test root cause (round 2, |
ayuskauskas
left a comment
ayuskauskas
left a comment
There was a problem hiding this comment.
Approving. Faithful implementation of the #1542 dimension-uniform coverage post-condition, and retaining requireOSIfNeeded rather than subsuming it (as I'd claimed on the issue) is the correct call — coverage is satisfied when service/accelerator are honored by separate overlays, while the joint OS guard still needs to fire. Nicely pinned by the golden matrix.
One non-blocking follow-up on the CLI snapshot-relax path (pkg/cli/query.go):
uncoveredCoverageDimensions depends on (a) the coverage error being the first StructuredError in the chain and (b) Context["uncovered"] asserting to exactly []map[string]any. That holds today — the facade (aicr.go) and buildWithStore (builder.go) both return the builder's error unwrapped — but it's only unit-tested with a synthetic error (mkCoverageErr), and the e2e scripts were changed to drop --os ubuntu entirely, which sidesteps the exact scenario this path exists for. So relaxation is unexercised end-to-end, and a future re-wrap of the error (or a JSON boundary) would silently disable it with no failing test.
Suggestion: add an integration test through ResolveRecipeFromSnapshot that drives a real coverage failure → relax → retry, and/or make extraction robust by walking the chain for any StructuredError carrying uncovered instead of asserting on the first match.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
uncoveredCoverageDimensions previously asserted on the first StructuredError in the chain (stderrors.As) and on the exact in-process payload type []map[string]any, so an intermediate wrap between the builder and the CLI — or a marshaling boundary — would have silently disabled snapshot-derived criteria relaxation. Walk every StructuredError node in the wrap chain and accept the decoded-JSON payload shape ([]any of map[string]any) as well. Pin the contract end-to-end: a new integration test drives a REAL coverage failure through ResolveRecipeFromSnapshot on the embedded catalog (kind + snapshot-detected ubuntu), asserts extraction sees the builder's actual error shape, relaxes, and resolves on retry. Unit tests cover wrapped (same/different code) and JSON-shaped payloads. Follow-up to the PR #1784 review; related to #1542. Signed-off-by: Mark Chmarny <mark@chmarny.com>
The merge of main brought in e0efbb5, which reverted the AKS H100 slinky-slurm recipe and deleted h100-aks-ubuntu-training-slurm.yaml. The golden coverage matrix still pinned that overlay's aks+slurm projections, failing TestCoverageGoldenMatrix in CI. Regenerated via AICR_UPDATE_GOLDEN=1; the diff removes exactly the reverted overlay's projection family. Signed-off-by: Mark Chmarny <mark@chmarny.com>
|
Both follow-ups addressed in 2715f3f:
|
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
📋 Multi-Persona Review
Method: 5 independent persona reviewers (Correctness, Domain/Architecture, API-contract, Test-coverage, CI-DX) → adversarial senior meta-reviewer that re-derived every finding from the resolved code at head 85eb09e. 10 candidates raised; 8 survived, 2 refuted.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
This is a well-engineered, well-tested bug fix with a sound core invariant. The coverage algorithm is correct: appliedOverlays genuinely includes inheritance-chain ancestors (so ancestor-supplied coverage counts) but excludes constraint-excluded overlays (so "honored only by an excluded overlay" fails loudly, as intended). The requireOSIfNeeded / per-dimension-coverage composition has no gap or double-gating. The shallow-copy retry in the CLI is safe (Criteria is all scalars). Error-code choice (400) is consistent with the sibling OS guard. Fail-closed evaluation aligns with the project's own anti-pattern rules.
No blockers. The one 🟠 is a test-coverage gap on the PR's central safety property, not a code defect. The 🟡s are accuracy/UX refinements.
Recommendation: Approve with comments — worth addressing the 🟠 (test the touched invariant) and the OpenAPI overpromise before or shortly after merge.
Confirmed non-issues (examined and cleared)
- Coverage-set correctness —
appliedOverlayscorrectly includes inheritance-chain ancestors but not excluded overlays; bothverifyCriteriaCoveragecall sites run on the final applied set; single-retry relaxation is provably sufficient. next := *criteriaaliasing —Criteriais all scalar enum/int fields, no slices/pointers; the shallow copy fully isolates the retry.touchedkeys ↔ coverage dimension names — identical constants inconsts.goandcoverage.go;nodesintentionally absent from both.- No internal leak —
verifyCriteriaCoverageusesNewWithContext(noCause), soWriteErrorFromErr's 4xxdetails.errorbranch never fires; message is only completion clauses. Struct↔JSON tags align. - Guard composition —
requireOSIfNeeded(joint service+accel) and per-dimension coverage neither subsume each other nor double-gate. PropagateOrWrap/ wrap-chain walk — no code-clobbering;uncoveredCoverageDimensionsinspects every chain node and won't false-match a bareErrCodeInvalidRequest.- Operational call sites — the 4 changed shell/chainsaw files adapt correctly (drop stated-but-undistinguished
--os, retargetgke→aks); no|| truemasking; no unmodified in-repo resolver call site breaks post-merge; no shell-injection. - Blast-radius fixtures — spot-checked; none weaken assertions (
config_integration_testwas strengthened). gpu-alias— refuted: nogb200-on-gkedoc example returns a franken-recipegb200+gkeoverlay exists, soservice/osare uncovered → this actually returns 400, not a 200 partial. (Minor residue: theapi-reference.md:146example's comment impliesos=cosmakes it valid when it would 400 onservice— worth a re-check of the intended illustration.)e2e— refuted: the flip is almost certainly coverage-forced (mirroring the sibling--intent training→inferencenarrows coverage for no reason--osdrops); the only residue is thatrun.shlacks the explanatory comment its siblings got.
Summary
🔴 Blocker 0 | 🟠 Major 1 | 🟡 Minor 5 | 🔵 Nitpick 2 Recommendation: Approve with comments
| Tier | Finding | Location |
|---|---|---|
| 🟠 | touched-map invariant untested (silent-relax regression could ship green) |
pkg/cli/recipe.go:374 |
| 🟡 | Fail-closed aborts on unparseable snapshot value (snapshot path only) | pkg/recipe/metadata_store.go:1195 |
| 🟡 | Misleading "no recipe provides" when sole overlay was excluded | pkg/recipe/coverage.go:248 |
| 🟡 | OpenAPI/docs promise HTTP fields that can't be emitted | api/aicr/v1/server.yaml:237 |
| 🟡 | Snapshot-relaxation policy CLI-only, undocumented SDK asymmetry | pkg/cli/query.go:237 |
| 🔵 | Golden matrix doesn't guard completion-suggestion content | pkg/recipe/coverage_matrix_test.go:32 |
| 🔵 | Completions constraint-unaware + multi-element wording unasserted | pkg/recipe/coverage.go:88 |
Generated via multi-persona review + adversarial meta-review. Findings independently re-derived from resolved code; refuted candidates listed above for transparency.
- pkg/cli: pin the touched-map invariant — table tests assert applyCriteriaOverrides/applyCriteriaFromConfig mark exactly the stated coverage dimensions, plus end-to-end CLI cases proving a user-stated uncovered dimension propagates the coverage error while a fingerprint-derived one is relaxed - pkg/constraints: route parsed.Evaluate errors through PropagateOrWrap so the version parser's ErrCodeInvalidRequest is not clobbered to ErrCodeInternal; add TestEvaluate pinning the eval error-code contract - pkg/recipe: truthful wording when an uncovered dimension is provided only by constraint-excluded overlays (was: 'no recipe provides'); golden matrix now pins per-dimension validCompletions content; assert multi-tuple and multi-value completion wording - api/docs: scope excludedOverlays/constraintWarnings to snapshot-driven (CLI/SDK) resolution in all four HTTP 400 descriptions and api-reference; document strict facade coverage + CLI-only relaxation on ResolveRecipeFromSnapshot - tests/e2e: explain why kind queries use --intent inference - golden regenerated (also folds in main's new h100-aks-ubuntu-training-slurm projections missing since the merge) Signed-off-by: Mark Chmarny <mark@chmarny.com>
|
Multi-persona review round addressed in 760cf2a — all 7 inline threads replied to and resolved (1 🟠, 4 🟡, 2 🔵; details in each thread). Notes on the review-body residues that had no inline thread:
Gates: |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-reviewed at 760cf2a3 — all 7 findings from the multi-persona round are addressed, and every new test executes and passes locally (pkg/constraints, pkg/recipe, pkg/cli green).
- 🟠 touched-map invariant —
touched_invariant_test.gopins production (exact-set table tests over all 5 dimensions) and end-to-end (stated--ospropagates the coverage error; derived os relaxes and drops from output). A dropped/mis-keyed mark now fails four tests. - 🟡 fail-closed on unparseable value — kept fail-closed (correct); fixed the real sub-defect via
PropagateOrWrapinevaluate.gosoErrCodeInvalidRequestreaches the engine un-clobbered, pinned byTestEvaluate. - 🟡 misleading "no recipe provides" —
excludedOverlayProvides+onlyExcludedwording, with a subtest asserting the old wording is gone. - 🟡 OpenAPI overpromise — all four HTTP 400 descriptions + api-reference now scope
excludedOverlays/constraintWarningsto snapshot-only. - 🟡 SDK parity — documented the STRICT post-condition on
ResolveRecipeFromSnapshot(right call; the facade has no touched-provenance). - 🔵 golden completion content —
ValidCompletionsadded to the classification and diffed; golden regenerated (also folded in the missingh100-aks-ubuntu-training-slurmprojections). - 🔵 multi-element wording — verbatim multi-tuple and sorted multi-value assertions added; deprioritizing excluded completions declined with sound reasoning.
The two declined items are defensible design decisions, not gaps. Approving.
3 participants
Summary
Recipe resolution now fails with an actionable
ErrCodeInvalidRequest(HTTP 400) when a query-stated criteria dimension is not honored by any applied overlay, instead of silently returning a partial recipe that echoes criteria it did not apply. Constraint evaluation additionally fails closed on non-NotFound evaluator errors.Motivation / Context
The OS-required guard (
requireOSIfNeeded) models only service+accelerator/OS, while the criteria matcher treats all dimensions uniformly — so a statedplatformorintentcould silently contribute nothing. Live repro onmain:aicr recipe --service eks --accelerator h100 --intent training --platform kubeflowexited 0 with nokubeflow-trainercontent while echoingplatform: kubeflowin its criteria.Fixes: #1542
Related: #1781, #1782, #1783 (follow-ups: nodes demotion, guard subsumption, constraint-path load-time validation)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)docs/,examples/)Implementation Notes
pkg/recipe/coverage.go): every stated dimension (service, accelerator, intent, os, platform —nodesis exempt/advisory) must appear with its exact value on ≥1 applied overlay (leaf or inheritance-chain ancestor). Runs on the FINAL applied set in bothBuildRecipeResultandBuildRecipeResultWithEvaluator(after constraint exclusion and the mixin-failure fallback rebuild).platform 'kubeflow' for criteria(service=eks, accelerator=h100, intent=training) requires os (valid: ubuntu)vsno recipe provides platform 'nim' for ...— plus a machine-readabledetails.uncoveredarray (dimension,requestedValue,validCompletions); evaluator path addsexcludedOverlays/constraintWarningscontext.requireOSIfNeededstays and runs first: per-dimension coverage does not subsume its joint service+accelerator OS gating (see recipe: joint-coverage formulation to subsume requireOSIfNeeded #1782). Pinned byTestBuildRecipeResult_GuardAndCoverageComposition.ErrCodeInvalidRequest, internal failures →ErrCodeInternal) now abort resolution with their codes preserved, in both per-overlay and mixin evaluation.ErrCodeNotFound("value not found in snapshot") remains the designed graceful-exclusion signal, sorecipe --snapshotdegradation is unchanged and no new 404 surfaces on/v1/recipe.pkg/recipe/testdata/coverage_golden.yaml, 415 entries): pins the resolution outcome of every projection of every overlay's criteria. Any registry/overlay data change that flips an outcome failsTestCoverageGoldenMatrix; regenerate deliberately withAICR_UPDATE_GOLDEN=1 go test ./pkg/recipe/ -run TestCoverageGoldenMatrix.--service gkeoverride onto anos=ubuntubase — no gke+ubuntu overlay exists).Testing
Coverage deltas vs
origin/main:pkg/recipe: 87.3% → 88.0% (+0.7%);pkg/client/v1,pkg/server,pkg/cliunchanged. Every function inpkg/recipe/coverage.goexercised.Live verification: the #1542 repro now exits non-zero with the completion-suggestion error;
--os ubuntuand the generic-tier query (eks+h100+training, no platform) still exit 0.Risk Assessment
Rollout notes: Intentional contract change: queries that previously returned 200 with silently-partial recipes now return 400 with
details.uncovered; malformed constraints in recipe data now hard-fail resolution instead of silently excluding an overlay. No transition flag (pre-1.0, per approved design). External recipe-data consumers (internal recipes repo) notified on #1542. Revert is a clean single-commit revert.Checklist
make testwith-race)make lint)git commit -S) — GPG signing info