feat: select validations by suite and capability - #561
Merged
Conversation
Introduce declarable capability vocabulary, expand_capabilities() for compute inheritance from vm/bare_metal, per-check requires filtering with explicit capability_requirement skip reasons, and validation for requires values on plain-suite checks. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Reject tests.module, tests.platform=compute, requires on platform suites, and legacy per-check platforms fields. Rewrite validate_suite_wiring for plain vs platform suite rules and globally unique wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Resolve platform or plain suite configs via unified --suite selection, parse optional --capabilities filter context (rejecting compute), apply capability filtering in orchestration with human-readable dry-run plans, and skip setup steps when all bound checks are filtered out. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Build catalog entries from canonical suite YAML with source, suite, platform, and requires metadata. Bump schemaVersion to 2, replace platforms with capabilities in the envelope, and remove the legacy catalog_platforms registry module. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Remove tests.platform from plain capability suites, add per-check requires from requires-worksheet, move kubernetes storage checks into storage.yaml with ensure_cluster wiring, add my-isv demo stub and AWS reference setup, and drop provider platform keys from plain-suite command groups. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Align merger, nico provider, and capacity config tests with plain-suite capability resolution and catalog schema v2 entry shape. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
my-isv storage demo exercises compute-path checks only; pass --capabilities vm so kubernetes requires checks are skipped with explicit capability_requirement reasons. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Emit declarable capabilities as `platforms` and plain suite names as `suites` in catalog documents and upload payloads so backend catalog ingestion matches the feat/test-structure contract.
Both flags now build and save the catalog locally without uploading, using --dry-run as the primary name for consistency with other commands. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> (cherry picked from commit 1550697)
K8S23-04..07 gained the storage label in the suite YAML; the test-plan coverage guardrail requires the doc to carry the same label union. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The four capabilities (kubernetes, slurm, vm, bare_metal) are mutually exclusive execution environments — you run on one, never a combination — so a comma-separated multi-value context never made sense. - Rename the flag --capabilities -> --capability (single value); reject comma-separated input with an explanatory error. - parse_capabilities -> parse_capability (returns str | None). - requirements_satisfied / resolve_entries take one capability instead of a set: `not requires or capability in requires`. - Thread a single capability through the orchestrator and dry-run. `requires` (per-check) stays a list: any-match applicability across the isolated contexts, not a request to combine them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
A plain suite named after a declarable capability (vm/bare_metal/kubernetes/slurm) would collapse the merged frontend test-target list and cross-wire the backend's flat-selection re-split (selection ∩ catalog.platforms). Reject the collision at the upload chokepoint (catalog_document) and in the wiring lint (validate_suite_wiring.py), reusing DECLARABLE_CAPABILITIES, so the capability-vs-suite distinction stays recoverable by construction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
test validate now rejects unknown/duplicate requires values via the Pydantic suite-shape validator, matching the run-time and wiring checks. validate_suite_wiring flags a requires naming a capability with no platform suite (no ISV can declare it, so the check is unreachable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Capability gating skips a step when it carries an explicit `requires:` or when every validation bound to it is requirement-filtered. Teardown steps have no bound validations, so inference cannot reach them: under the core-by-default `--suite` path they kept running against fixtures that were never created. Only storage carried explicit gates; three other suites had the same hole. - image-registry: teardown deleted both the core upload_image resources and the vm-only launch_instance ones, so it referenced a skipped step and was abandoned whole with "missing step reference" - leaking the image, disks and bucket. Split by owner into teardown_instance (requires: [vm]) and teardown_image (core); the scripts already guard each resource independently, so no script change was needed. Rebind the suite's teardown check to teardown_image, the half that always runs. - observability: gate launch_host/teardown_host on [vm, bare_metal] so a core run no longer launches the reference metal host only to skip every check that needed it. - security: gate topology_block_teardown/capacity_teardown on [vm, bare_metal], mirroring the gate inferred on the test-phase steps that allocate those resources. StorageCapacityTelemetryCheck moves from core to [vm, bare_metal]. It probes the volumes attached to a running instance via the same script and --instance-id as its StoragePerformanceTelemetryCheck twin, which already declared that gate; with no instance it reports "no volumes attached", so it could never pass as a core check. Needs a catalog re-upload. Add test_capability_step_gating.py: instead of three point tests it walks every plain-suite provider config across every context and asserts no surviving step reads a gated-off step's output without a `default(...)`, so new suites inherit the guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
my-isv is the folder ISVs copy, so a guarantee that holds only in aws/ teaches the wrong lifecycle. Three divergences, all in the capability-gated paths added on this branch. - storage: setup_cluster carried no `requires: [kubernetes]` and had no teardown_cluster at all, so the scaffold demonstrated acquiring a cluster and never releasing it - the opposite of the setup_cluster/teardown_cluster contract aws/ has carried since 607c121. Add both, with STORAGE_SKIP_TEARDOWN mirroring AWS_SKIP_TEARDOWN, and a teardown_cluster.py stub in the house style. A no-op teardown is a valid answer when the stub reuses a long-lived cluster; the step still has to exist so a standalone storage run cannot leak a cluster it provisioned. - kubernetes/slurm: no config existed, so `--suite kubernetes --provider my-isv` errored - and that is exactly what the frontend emits for its default provider. The scripts were already there; only the wiring was missing. Steps are overridden rather than inherited because the canonical suites point at ../providers/my-isv/scripts/*, which resolves wrong from a provider config directory. Neither joins `make demo-test`: both drive real kubectl/sinfo, so a dummy-success stub has nothing to return. Also fixes a pre-existing scaffold break found while verifying the above: `--suite storage --capability kubernetes` failed with "'cluster_name' is a required property". The step name auto-detects the platform-suite `cluster` schema, which demands cluster_name/node_count - inventory no storage check reads. Set `output_schema: generic` on both providers; scoping it to my-isv would have recreated the divergence this commit removes, and per the JSON contract discipline storage must not require cluster inventory of an ISV. test_storage_cleanup_steps_have_explicit_capability_gates encoded the divergence as `if provider == "aws":`; both providers now assert the same gates. Add coverage that every declarable capability resolves to a my-isv platform suite, so a UI-emitted `--suite <capability>` cannot silently become an error again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…path The same config ran a different set of checks depending on how it was named: `--suite network` gave 26 checks (core default), `-f .../network.yaml` gave 35 (unfiltered), and `--provider X --label network` gave 35 as well. The label path is the one requirements.md NVIDIA#4 calls out by name - label selection must not run checks gated on a capability the ISV does not support - and it did. Resolve the context after config validation instead of inside the `--suite` branch, so one rule covers `--suite`, `-f` and `--label` discovery alike: a plain suite with no `--capability` runs its core checks. Platform suites declare no `requires:`, so they keep the unfiltered context. Label discovery inherits this through its recursive call, which closes NVIDIA#4 without special casing it. This drops the "no filtering" pseudo-context as a default. It modelled no real ISV situation - nobody runs on vm and kubernetes at once - so a plain suite now always carries exactly one context. Warn when `--capability` names something no check in the suite requires (`--suite iam --capability kubernetes`): the run proceeds with core checks, but a flag that silently does nothing reads as a typo. UI-composed commands are unaffected. `-f` used to hand the demos their gated checks for free, so the Makefile now names a capability per suite to keep exercising those stubs; without it `make demo-test` would quietly shrink by ~29 checks and stop covering the launch_instance / install_image_bm / install_config_bm scripts while still printing all-green. image-registry splits its gated checks between vm and bare_metal, so it runs once per context. Document the rule in configs/suites/README.md, which described neither `requires:` nor the selection grammar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
`requires:`, `--suite` and `--capability` appeared in no user-facing doc. The model existed only in the code and in decision.md, an internal design record, so an ISV had no way to learn what governs which checks they owe. - configs/suites/README.md already gained the rule when the default changed; this adds the full model to the two guides an ISV actually reads. - configuration.md: a "Capabilities and requires" section (the four mutually exclusive capabilities, platform vs plain suites, any-match semantics and why AND is deliberately inexpressible, the one selection rule, opt-in scope, the shared namespace), plus `requires` in the step field table with the gate-both-halves-of-a-fixture guidance. - external-validation-guide.md: the same model stated for an ISV audience, and a Running Validations section that shows the --suite/--capability grammar, --dry-run, label composition and single-check rerun by pytest passthrough. - my-isv scaffold README: real invocations rather than only the -f form, plus what governs which checks run. Also corrects drift found in the scaffold's Domains table: 7 of 11 script counts were wrong, and k8s/slurm listed no provider YAML - true until those configs landed in 3735674. Both now note they need a real cluster and so sit outside `make demo-test`. Drops a stale MY_ISV_DOMAINS reference for the MY_ISV_SUITES / DEMO_CAP_<suite> variables that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
A run's signal is the pair, not the suite alone: `network` and `network --capability vm` execute different checks, so recording only one axis loses the distinction at the point where it matters most. Neither axis was transmitted at all -- the service inferred a single test_target_type from the first command, which collapsed both. Two client-side spellings have to be translated before they leave the process. CORE_REQUIREMENT_CONTEXT is this module's word for "no capability", which the service records as NULL rather than a sentinel. And a platform suite carries no explicit context because its own platform *is* the capability it runs under, so report that instead of dropping the axis for every platform-suite run. resolve_suite_name recovers the suite from every entry path, including `-f lab.yaml -f commands.yaml -f suites/k8s.yaml` where the first config is not the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… suites The pre-commit validator carried a hand-copied DECLARABLE_CAPABILITIES literal, so the hook that exists to enforce the vocabulary was the one place that could silently drift from the schema it enforces. It now imports the shared constant, and the REQUIREMENT_VOCABULARY alias (a pure alias with no external importers) is gone. Each suite YAML was also read three to four times and parsed two to three times per run: _build_suite_map parsed a file and then called iter_config_checks, which re-read and re-parsed the same file, and wiring_errors globbed the directory twice while reading each file twice more in its main loop. Both now expose iter_checks_from_data so a caller that has already parsed the document reuses it, and wiring_errors reads and parses each file exactly once. The suite vocabularies are cached: resolve_suite_name alone scanned the suites directory twice per invocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Two rules had each been written out four times. `requires` validation (a list, drawn from DECLARABLE_CAPABILITIES, no duplicates) lived in StepConfig.validate_requires, in ValidationConfig.validate_suite_shape, in _validate_entry_shape, and in the wiring script -- with four separately maintained message strings, so "requires must contain only" and "requires must be a list containing only" were the same verdict spelled two ways. requires_error() states it next to the vocabulary it validates; each caller adds only its own prefix. The pre-commit hook and the runtime validator can no longer disagree about what a valid suite file is. Suite-name canonicalization was worse, because suite name is the join key between a test run and its catalog entries: `_normalize_name` did lower + `-`/`_` + the k8s alias, `catalog.py` did the replace alone in two places, and the wiring script hardcoded `if stem == "k8s"`. Adding a second aliased filename meant finding all four. canonical_suite_name() in isvtest.core.resolution is now the single authority -- isvctl already depends on isvtest, so nothing new is coupled. Alongside those, three smaller consolidations: iter_checks_from_data existed twice, differing only in whether the category is yielded and in a branch the script's copy had dropped (dict-form categories without a `checks` key). The three-tuple version in isvtest.catalog is now the only walker of the wiring shape; no suite YAML uses the missing branch today, so the script only gains coverage. catalog_document parsed the suites directory three times -- once via _build_suite_map, then again in each vocabulary builder. _iter_suite_docs makes it one pass, and suite_vocabularies returns both lists from it. CONFIGS_ROOT moves from cli/test.py to config/suite_resolution.py, so `deploy` no longer imports the whole `test` command module -- typer app, orchestrator and all -- to obtain one Path. _suite_name gains @cache, since classifying a config merges it with the suite it imports and a single run asks both resolve_suite and resolve_suite_name for it. test_capability_step_gating copied the run/skip cascade out of _apply_capability_step_gates into its own _gated_step_names, so it would have kept passing against the old rule if the real gate changed. It now calls the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Typer forces a terminal under GITHUB_ACTIONS, so the reported option name arrives spliced with escape codes and the substring match fails in CI while passing locally. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
tests.platform now rejects anything outside vm, bare_metal, kubernetes and slurm, but its field description (and the generated JSON schema) still listed the old vocabulary, and four config examples declared values that raise at load. Plain suites omit the key entirely. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The gating invariant listed aws and my-isv by name, so nico's plain suites escaped it; discovering the config directories keeps a new provider covered. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The explicit two-capability rule overrode the static pattern rule, so every make invocation printed an overriding-recipe warning. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
`capability` was already the shared word across the client, backend, and frontend (`--capability`, `DECLARABLE_CAPABILITIES`, `isv_version.capabilities`, `CapabilityChip`). `platform` was the odd one out, and it meant three different things: the capability a platform suite declares (`tests.platform:`), the lifecycle command groups (`commands.<key>`, which also span plain suites like `iam` and `security`), and the capability-valued half of the catalog namespace. Delete it as a synonym for capability. `requires` stays as-is: it is the predicate on a check, `capability` is the value, and collapsing the two would lose the distinction between "this check requires kubernetes" and "this run ran under kubernetes". - `tests.platform:` -> `tests.capability:` in the four platform suites and every reader (schema, suite_resolution, cli/test, orchestrator/loop, isvtest catalog, isvreporter platform detection, the suite wiring validator). - The legacy key now raises rather than being ignored. `ValidationConfig` allows extras, so an unmigrated `tests.platform:` would have landed in `model_extra`, demoted the suite to a plain suite, and gated every check on a capability it no longer declared. Both validators report the rename, matching how `tests.module` is handled. - `commands.<key>` keys are unchanged; their descriptions and KeyError messages now say "test target", since the keys span capabilities and plain suites. - `isvctl test validate` prints `Capability:` instead of `Platform:`. - Regenerated config.schema.json. The wire format is deliberately untouched: the catalog envelope still carries `platforms`/`suites` and entries still carry `platform` xor `suite` plus `requires`. Renaming those columns is a separate migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Client half of the catalog wire rename. The document envelope's `platforms` becomes `capabilities` and a per-entry `platform` becomes `capability`, matching the renamed backend columns and the word the CLI already uses for this axis (`--capability`, tests.capability). - isvtest.catalog: the envelope key and the per-check suite map. - isvreporter.client.upload_test_catalog: the `capabilities` kwarg and the JSON body it POSTs. - isvctl catalog push/list read the renamed keys. The backend accepts both spellings on upload (@JsonAlias), so a lab still running an older isvctl is unaffected; this only changes what a current client sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR replaces platform-based suite metadata with capability-based execution, adds ChangesCapability-based execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-28 14:49:03 UTC | Commit: 02acfc0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
isvctl/src/isvctl/config/schema.py (1)
127-158: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "phase level" skip claim in
PlatformCommandsdocstring, propagated into the generated JSON schema.
isvctl/src/isvctl/config/schema.py'sPlatformCommandsdocstring says both "...and phase level." (L133) and, further down, "...and step level." (L153-154) — there is no per-phase skip field, so the L133 mention is stale and contradicts the corrected sentence. Becauseisvctl/schemas/config.schema.jsonis generated from this pydantic model, it embeds the exact same contradictory text verbatim.
isvctl/src/isvctl/config/schema.py#L127-L158: remove the stale "Supports skip at both target level (skips all phases) and phase level." sentence (L133), keeping only the corrected "...and step level." sentence (L153-154).isvctl/schemas/config.schema.json#L65-L65: regenerate this file fromschema.pyafter the docstring fix lands; no independent edit needed here.🤖 Prompt for 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. In `@isvctl/src/isvctl/config/schema.py` around lines 127 - 158, Remove the stale phase-level skip sentence from the PlatformCommands docstring in isvctl/src/isvctl/config/schema.py lines 127-158, retaining the target-level and step-level skip documentation. Regenerate isvctl/schemas/config.schema.json at lines 65-65 from the updated Pydantic schema; do not edit that generated file independently.scripts/validate_suite_wiring.py (1)
218-231: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
--checkhelp text is now stale.
main()(L231) now runs onlywiring_errors()—platform_registration_errors()andregistry_consistency_errors()were removed. The--checkoption's help text (unchanged by this diff) still promises checks for "unregistered suites" and "isvreporter platform mismatches," which no longer run.📝 Proposed fix
parser.add_argument( "--check", action="store_true", - help=( - "Exit 1 on wiring violations (missing test_id/labels, unregistered suites, " - "or isvreporter platform mismatches)." - ), + help="Exit 1 on wiring violations (missing test_id/labels, invalid suite metadata, unreachable requires).", )🤖 Prompt for 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. In `@scripts/validate_suite_wiring.py` around lines 218 - 231, Update the --check argument help text in main to describe only the validations currently performed by wiring_errors(), removing claims about unregistered suites and isvreporter platform mismatches.isvreporter/src/isvreporter/main.py (1)
314-322: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winCritical:
report update --test-catalogalso callsupload_test_catalogwith the removedplatformskeyword.Same contract break as in
isvctl/src/isvctl/reporting.py:upload_test_cataloginisvreporter/src/isvreporter/client.pyonly acceptscapabilities(keyword-only), notplatforms. Herecatalog_data.get("platforms", [])at least avoids aKeyError(via.getwith a default), but the call itself will raiseTypeError: upload_test_catalog() got an unexpected keyword argument 'platforms', caught by theexcept Exceptiona few lines down and logged as a warning — soisvctl report update --test-catalog <file>never actually uploads the catalog.🐛 Proposed fix
upload_test_catalog( endpoint=endpoint, jwt_token=jwt_token, isv_test_version=catalog_version, entries=catalog_entries, schema_version=catalog_data.get("schemaVersion", 1), - platforms=catalog_data.get("platforms", []), + capabilities=catalog_data.get("capabilities", catalog_data.get("platforms", [])), suites=catalog_data.get("suites", []), )(the fallback to the legacy
platformskey on read preserves the PR's stated intent of "accepting legacy spellings on upload" for older locally-saved catalog files.)🤖 Prompt for 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. In `@isvreporter/src/isvreporter/main.py` around lines 314 - 322, Update the upload_test_catalog call in the report update flow to pass the supported capabilities keyword instead of the removed platforms keyword. When reading catalog_data, use capabilities first with a fallback to the legacy platforms key so older saved catalogs remain accepted, while preserving the existing default empty list.
🧹 Nitpick comments (4)
isvctl/configs/suites/observability.yaml (1)
118-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: mirror the gating rationale onto the performance twin.
StorageCapacityTelemetryCheckdocuments why it needs an attached-volume fixture; its twin here carries the samerequireswith no note. A one-line "same rationale as its capacity twin" keeps the pair from drifting.🤖 Prompt for 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. In `@isvctl/configs/suites/observability.yaml` around lines 118 - 124, Add a one-line comment to StoragePerformanceTelemetryCheck in the storage_performance_telemetry suite documenting that its attached-volume requirement uses the same rationale as StorageCapacityTelemetryCheck. Keep the existing requires values and check configuration unchanged.isvtest/tests/test_catalog.py (1)
91-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the documented catalog field to
capability
isvtest/src/isvtest/catalog.pystill describes the returned field asplatform; update it tocapabilityso the docstring matchesbuild_catalog()and the catalog entry contract.🤖 Prompt for 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. In `@isvtest/tests/test_catalog.py` around lines 91 - 111, Update the catalog field documentation in build_catalog() so it names the returned field as capability instead of platform, matching the actual catalog entry contract and test_entries_have_suite_contract expectations. Leave the returned data and other documented fields unchanged.isvctl/src/isvctl/cli/deploy.py (1)
388-402: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
platformmetadata still derived fromconfig_files[0]only, unlike the newsuiteresolution.
suite=resolve_suite_name(list(config_files), CONFIGS_ROOT)correctly scans every-ffile (per its own docstring: "the suite has to be recoverable from every entry path -- including-f lab.yaml -f commands.yaml -f suites/k8s.yaml, where the first config is not the suite"). Butplatform = get_platform_from_config(config_files[0])(line 391, unchanged) still only reads the first file. Since--config/-fis documented as repeatable with later files overriding, a run like-f lab.yaml -f suites/k8s.yamlwould resolvesuitecorrectly but could report the wrongtest_target_typeon the created test run.🔧 Possible fix: reuse the same multi-file resolution for platform detection
- platform = get_platform_from_config(config_files[0]) if config_files else "kubernetes" + platform = ( + get_platform_from_config(resolve_suite_config_path(config_files, CONFIGS_ROOT)) + if config_files + else "kubernetes" + )(or scan
config_filesin order and use the first file classified as a known suite, mirroringresolve_suite_name's own logic.)🤖 Prompt for 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. In `@isvctl/src/isvctl/cli/deploy.py` around lines 388 - 402, Update platform detection in the deploy flow around create_test_run to resolve metadata across all config_files, matching resolve_suite_name’s multi-file scanning rather than using config_files[0]. Preserve the Kubernetes fallback when no config files or known platform suite is found, and pass the resolved platform to create_test_run.isvctl/src/isvctl/cli/test.py (1)
125-160: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_resolve_capability_contextkeeps a mismatched capability instead of downgrading to core.When
capabilityis set but no validation entryrequiresit (line 141-143), the code prints "running core checks only" but stillreturn capabilityrather thanCORE_REQUIREMENT_CONTEXT. In practice this is harmless today because every consumer (requirements_satisfied,_apply_capability_step_gates) treats an entry/step with emptyrequiresas always-satisfied regardless of the context value, so behavior is unaffected — but_reported_capabilitywill still record the mismatched capability on the uploaded test run, which is inconsistent with the printed "core checks only" message.🤖 Prompt for 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. In `@isvctl/src/isvctl/cli/test.py` around lines 125 - 160, Update _resolve_capability_context so that when no validation entry requires the selected capability, it returns CORE_REQUIREMENT_CONTEXT after printing the existing warning. Preserve returning capability when a matching requirement exists and leave _reported_capability unchanged.
🤖 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 `@docs/packages/isvreporter.md`:
- Around line 183-184: Update the CLI option documentation adjacent to --config
to use the canonical capability argument instead of --platform, and list all
supported values including vm; if --platform remains supported, explicitly
identify it as a compatibility alias.
In `@isvctl/src/isvctl/config/schema.py`:
- Line 130: Wrap the full target list in the command-group docstring and the
corresponding Field description across multiple lines, keeping each line within
120 characters while preserving the existing wording and all nine target names.
- Around line 127-134: Update the PlatformCommands docstring to remove the
incorrect “phase level” skip description and describe skip support only at the
target level and step level, matching the StepConfig skip field and the
corrected documentation later in the class. Ensure the generated config schema
documentation is regenerated so it no longer contains the stale wording.
In `@isvctl/src/isvctl/config/suite_resolution.py`:
- Around line 149-170: Update resolve_suite to classify provider config files
individually and ignore files where _suite_name raises SuiteResolutionError,
matching the existing behavior in resolve_suite_name. Filter out those invalid
unrelated configs before matching the requested suite, while preserving the
current no-match, duplicate-match, and successful ResolvedSuite behavior.
In `@isvctl/src/isvctl/reporting.py`:
- Around line 218-229: Update the upload_test_catalog call in the
catalog_document handling block to pass catalog_document["capabilities"] via the
capabilities keyword instead of using the nonexistent platforms key and keyword.
Align the forwarded arguments with upload_test_catalog and the existing
catalog.py push command, and update test_forwards_complete_catalog_document so
it verifies capabilities is forwarded correctly.
In `@isvctl/tests/test_capability_step_gating.py`:
- Around line 67-80: Update _unguarded_references so default(...) suppresses
only step references within the same {{...}} template expression, rather than
the entire string; continue collecting unguarded steps from other expressions
and plain text while preserving recursive dict and list traversal.
In `@isvctl/tests/test_reporting.py`:
- Around line 153-176: Update the catalog fixture and expected upload mapping in
update_test_run to use the schema-v2 capabilities field instead of platforms.
Replace the platforms-based lookup and mock_upload_catalog assertion with
capabilities while preserving the existing capability values and other fields.
In `@isvtest/src/isvtest/catalog.py`:
- Around line 216-224: Update the docstrings for _declared_platform and
build_catalog to use the catalog entry key “capability” instead of “platform,”
including the listed field description and wire-name reference; leave the
implementation and other platform terminology unchanged where it describes the
declared suite concept.
- Around line 383-399: Update the --test-catalog upload parsing in the relevant
isvreporter main flow to read the schema-v2 “capabilities” field first, while
falling back to legacy “platforms” when capabilities is absent. Preserve
existing behavior for v1 catalog files and ensure schema-v2 uploads pass the
parsed capabilities list onward instead of an empty value.
---
Outside diff comments:
In `@isvctl/src/isvctl/config/schema.py`:
- Around line 127-158: Remove the stale phase-level skip sentence from the
PlatformCommands docstring in isvctl/src/isvctl/config/schema.py lines 127-158,
retaining the target-level and step-level skip documentation. Regenerate
isvctl/schemas/config.schema.json at lines 65-65 from the updated Pydantic
schema; do not edit that generated file independently.
In `@isvreporter/src/isvreporter/main.py`:
- Around line 314-322: Update the upload_test_catalog call in the report update
flow to pass the supported capabilities keyword instead of the removed platforms
keyword. When reading catalog_data, use capabilities first with a fallback to
the legacy platforms key so older saved catalogs remain accepted, while
preserving the existing default empty list.
In `@scripts/validate_suite_wiring.py`:
- Around line 218-231: Update the --check argument help text in main to describe
only the validations currently performed by wiring_errors(), removing claims
about unregistered suites and isvreporter platform mismatches.
---
Nitpick comments:
In `@isvctl/configs/suites/observability.yaml`:
- Around line 118-124: Add a one-line comment to
StoragePerformanceTelemetryCheck in the storage_performance_telemetry suite
documenting that its attached-volume requirement uses the same rationale as
StorageCapacityTelemetryCheck. Keep the existing requires values and check
configuration unchanged.
In `@isvctl/src/isvctl/cli/deploy.py`:
- Around line 388-402: Update platform detection in the deploy flow around
create_test_run to resolve metadata across all config_files, matching
resolve_suite_name’s multi-file scanning rather than using config_files[0].
Preserve the Kubernetes fallback when no config files or known platform suite is
found, and pass the resolved platform to create_test_run.
In `@isvctl/src/isvctl/cli/test.py`:
- Around line 125-160: Update _resolve_capability_context so that when no
validation entry requires the selected capability, it returns
CORE_REQUIREMENT_CONTEXT after printing the existing warning. Preserve returning
capability when a matching requirement exists and leave _reported_capability
unchanged.
In `@isvtest/tests/test_catalog.py`:
- Around line 91-111: Update the catalog field documentation in build_catalog()
so it names the returned field as capability instead of platform, matching the
actual catalog entry contract and test_entries_have_suite_contract expectations.
Leave the returned data and other documented fields unchanged.
🪄 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: CHILL
Plan: Enterprise
Run ID: fa50a861-9c07-4a52-87d5-dd444a2554ed
📒 Files selected for processing (68)
Makefiledocs/guides/configuration.mddocs/guides/external-validation-guide.mddocs/guides/local-development.mddocs/packages/isvreporter.mddocs/test-plan.yamlisvctl/configs/providers/aws/config/image-registry.yamlisvctl/configs/providers/aws/config/observability.yamlisvctl/configs/providers/aws/config/security.yamlisvctl/configs/providers/aws/config/storage.yamlisvctl/configs/providers/aws/scripts/control-plane/docs/aws-control-plane.mdisvctl/configs/providers/my-isv/config/image-registry.yamlisvctl/configs/providers/my-isv/config/k8s.yamlisvctl/configs/providers/my-isv/config/observability.yamlisvctl/configs/providers/my-isv/config/slurm.yamlisvctl/configs/providers/my-isv/config/storage.yamlisvctl/configs/providers/my-isv/scripts/README.mdisvctl/configs/providers/my-isv/scripts/storage/setup_cluster.pyisvctl/configs/providers/my-isv/scripts/storage/teardown_cluster.pyisvctl/configs/providers/nico/config/control-plane.yamlisvctl/configs/providers/nico/config/iam.yamlisvctl/configs/providers/nico/config/network.yamlisvctl/configs/providers/nico/config/observability.yamlisvctl/configs/suites/README.mdisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/control-plane.yamlisvctl/configs/suites/iam.yamlisvctl/configs/suites/image-registry.yamlisvctl/configs/suites/k8s.yamlisvctl/configs/suites/network.yamlisvctl/configs/suites/observability.yamlisvctl/configs/suites/security.yamlisvctl/configs/suites/slurm.yamlisvctl/configs/suites/storage.yamlisvctl/configs/suites/vm.yamlisvctl/schemas/config.schema.jsonisvctl/src/isvctl/cli/catalog.pyisvctl/src/isvctl/cli/deploy.pyisvctl/src/isvctl/cli/test.pyisvctl/src/isvctl/config/schema.pyisvctl/src/isvctl/config/suite_resolution.pyisvctl/src/isvctl/orchestrator/loop.pyisvctl/src/isvctl/reporting.pyisvctl/tests/providers/nico/test_nico_provider.pyisvctl/tests/test_capability_step_gating.pyisvctl/tests/test_capacity_config.pyisvctl/tests/test_catalog_cli.pyisvctl/tests/test_cli_streams.pyisvctl/tests/test_cli_test_user_config.pyisvctl/tests/test_merger.pyisvctl/tests/test_orchestrator_loop.pyisvctl/tests/test_reporting.pyisvctl/tests/test_schema.pyisvctl/tests/test_suite_resolution.pyisvctl/tests/test_test_cli_labels.pyisvreporter/src/isvreporter/client.pyisvreporter/src/isvreporter/main.pyisvreporter/src/isvreporter/platform.pyisvreporter/tests/test_catalog_upload.pyisvreporter/tests/test_client.pyisvreporter/tests/test_platform.pyisvtest/src/isvtest/catalog.pyisvtest/src/isvtest/catalog_platforms.pyisvtest/src/isvtest/core/resolution.pyisvtest/tests/test_catalog.pyisvtest/tests/test_resolution.pyscripts/tests/test_validate_suite_wiring.pyscripts/validate_suite_wiring.py
💤 Files with no reviewable changes (8)
- isvctl/configs/providers/nico/config/network.yaml
- isvctl/configs/providers/nico/config/control-plane.yaml
- isvtest/src/isvtest/catalog_platforms.py
- isvctl/configs/providers/nico/config/iam.yaml
- isvctl/configs/providers/my-isv/config/observability.yaml
- docs/guides/local-development.md
- isvctl/configs/providers/nico/config/observability.yaml
- isvctl/configs/providers/aws/scripts/control-plane/docs/aws-control-plane.md
…ure to it Addresses review feedback on NVIDIA#561. The first item is a live break I introduced in 02acfc0. **Catalog upload was silently dead.** `isvctl.reporting.update_test_run` still did `catalog_document["platforms"]` and passed `platforms=` to `upload_test_catalog`. The envelope key is now `capabilities` and the parameter was renamed in the same commit, so every automatic upload raised KeyError before the call was even made. It is wrapped in `except Exception -> logger.warning`, so nothing failed loudly: coverage data just stopped arriving. `isvreporter report update --test-catalog` had the same break reading a saved catalog file; that path now prefers `capabilities` and falls back to `platforms`, since v1 files on disk outlive the rename. **The test suite could not have caught it.** The fixture in test_forwards_complete_catalog_document is a literal that kept the old `platforms` key, so it exercised a shape no producer emits and stayed green while the real path raised. Updated, and pinned: a new assertion compares the envelope keys against `catalog_document()` itself, so the next rename fails here instead of in production. Also from the review: - `_declared_platform` and `build_catalog` docstrings still described the entry key as `platform`; it is `capability`. - `docs/packages/isvreporter.md` documented `--platform` as taking three values and omitted vm. It takes all four; the flag keeps its spelling because it maps to the service's deprecated `test_target_type`. - `PlatformCommands` claimed skip works "at phase level" -- there is no per-phase skip field, only target and step -- and contradicted the corrected sentence four lines below it. - Two lines over the 120-char guideline, and the regenerated schema. - `resolve_suite` classified every YAML in a provider's config dir without catching SuiteResolutionError, so one malformed file broke `--suite <anything>` for that whole provider. `resolve_suite_name` already skipped such files; this now matches it. Pre-existing, not from the rename. Verified both upload call sites bind against the real signature and a real envelope, including the v1 legacy fallback. 1452 + 58 + 1279 + 113 tests pass, lint and pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The YAML key and catalog envelope already say capability/capabilities; align the local names in the suite wiring validator and catalog builder so the post-rename vocabulary is consistent. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Rationale blocks belong in commit messages, not above the code. Also drops a duplicated skip sentence from the PlatformCommands docstring and regenerates the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
isvctl/src/isvctl/config/schema.py (1)
405-412: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an explicitly empty capability instead of treating it as absent.
if self.capabilitytreatscapability: ""as a plain suite, silently bypassing capability-suite rules and potentially allowingrequires. Distinguish omitted values from empty values withis not None.Suggested fix
- if self.capability and self.capability not in DECLARABLE_CAPABILITIES: + if self.capability is not None and self.capability not in DECLARABLE_CAPABILITIES: raise ValueError(f"tests.capability must be one of: {', '.join(sorted(DECLARABLE_CAPABILITIES))}")🤖 Prompt for 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. In `@isvctl/src/isvctl/config/schema.py` around lines 405 - 412, Update the capability checks in the surrounding validation logic to use `self.capability is not None` instead of truthiness, so an explicitly empty capability is validated and rejected rather than treated as omitted; preserve the plain-suite behavior only when capability is actually absent.
🤖 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.
Outside diff comments:
In `@isvctl/src/isvctl/config/schema.py`:
- Around line 405-412: Update the capability checks in the surrounding
validation logic to use `self.capability is not None` instead of truthiness, so
an explicitly empty capability is validated and rejected rather than treated as
omitted; preserve the plain-suite behavior only when capability is actually
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79724f79-11bf-4fc6-8611-fc94fc44268c
📒 Files selected for processing (8)
isvctl/schemas/config.schema.jsonisvctl/src/isvctl/config/schema.pyisvctl/src/isvctl/config/suite_resolution.pyisvctl/tests/test_reporting.pyisvreporter/src/isvreporter/main.pyisvreporter/src/isvreporter/platform.pyisvtest/src/isvtest/catalog.pyscripts/validate_suite_wiring.py
🚧 Files skipped from review as they are similar to previous changes (7)
- isvreporter/src/isvreporter/platform.py
- isvreporter/src/isvreporter/main.py
- isvctl/tests/test_reporting.py
- isvtest/src/isvtest/catalog.py
- isvctl/schemas/config.schema.json
- isvctl/src/isvctl/config/suite_resolution.py
- scripts/validate_suite_wiring.py
Addresses the last open finding on NVIDIA#561. `_unguarded_references` returned nothing as soon as `default(` appeared anywhere in a string, so a mixed value like {{ steps.a.id | default('') }} {{ steps.b.id }} reported no dependency at all -- the guarded expression vouched for its unguarded neighbour, and the gating test would have passed a step that reads a skipped step's output. Now each `{{ ... }}` is checked on its own. Latent, not masking anything: every provider config still passes under all five contexts with the stricter helper. Adds unit tests for the helper itself, including the mixed-string case, since it had none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
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 `@isvctl/tests/test_capability_step_gating.py`:
- Around line 116-135: Add concise PEP 257-compliant docstrings to the four
named test methods in TestUnguardedReferences:
test_guarded_reference_is_ignored, test_bare_reference_is_reported,
test_reference_outside_an_expression_is_not_a_dependency, and
test_nested_structures_are_walked. Keep the existing assertions and test
behavior unchanged.
🪄 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: CHILL
Plan: Enterprise
Run ID: 43336435-d09a-46ff-8099-a7e03e0b9b34
📒 Files selected for processing (1)
isvctl/tests/test_capability_step_gating.py
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
isvctl/tests/test_test_cli_labels.py (1)
472-508: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that rejected input never reaches orchestration.
This test checks only the exit code and message. Also assert that the existing fake orchestrator recorded no calls, ensuring validation happens before execution.
🤖 Prompt for 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. In `@isvctl/tests/test_test_cli_labels.py` around lines 472 - 508, Update test_platform_suite_rejects_explicit_capability to assert that the existing fake orchestrator recorded no calls after invoking the invalid platform-suite command, while preserving the current exit-code and error-message assertions.isvctl/tests/test_deploy_reporting.py (1)
13-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover suites whose names differ from their capabilities.
The platform test uses
vm/vm, so it cannot catch the reporting bug fork8s.yamlwithtests.capability: kubernetes. Add a non-identical fixture and assert("k8s", "kubernetes").Also applies to: 24-32
🤖 Prompt for 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. In `@isvctl/tests/test_deploy_reporting.py` around lines 13 - 21, Update the _write_catalog fixture to use a platform suite whose filename stem differs from its tests.capability value, such as k8s and kubernetes, while retaining the plain suite. Update the related reporting assertions to explicitly expect the tuple ("k8s", "kubernetes") so this name-to-capability mapping is covered.
🤖 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 `@isvctl/src/isvctl/cli/deploy.py`:
- Around line 62-66: Update _reporting_suite_and_capability to resolve the
suite’s declared tests.capability rather than deriving capability from the suite
name. In isvctl/src/isvctl/cli/deploy.py lines 62-66, preserve the suite as
"k8s" while returning its declared capability. In
isvctl/tests/test_deploy_reporting.py lines 13-21, add a k8s.yaml fixture
declaring capability "kubernetes"; in lines 24-32, assert the result is ("k8s",
"kubernetes").
---
Nitpick comments:
In `@isvctl/tests/test_deploy_reporting.py`:
- Around line 13-21: Update the _write_catalog fixture to use a platform suite
whose filename stem differs from its tests.capability value, such as k8s and
kubernetes, while retaining the plain suite. Update the related reporting
assertions to explicitly expect the tuple ("k8s", "kubernetes") so this
name-to-capability mapping is covered.
In `@isvctl/tests/test_test_cli_labels.py`:
- Around line 472-508: Update test_platform_suite_rejects_explicit_capability to
assert that the existing fake orchestrator recorded no calls after invoking the
invalid platform-suite command, while preserving the current exit-code and
error-message assertions.
🪄 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: CHILL
Plan: Enterprise
Run ID: 45fb7804-3d3e-452c-85f9-08e97127660d
📒 Files selected for processing (6)
isvctl/src/isvctl/cli/deploy.pyisvctl/src/isvctl/cli/test.pyisvctl/tests/test_capability_step_gating.pyisvctl/tests/test_deploy_reporting.pyisvctl/tests/test_reporting.pyisvctl/tests/test_test_cli_labels.py
🚧 Files skipped from review as they are similar to previous changes (3)
- isvctl/tests/test_capability_step_gating.py
- isvctl/tests/test_reporting.py
- isvctl/src/isvctl/cli/test.py
Truthiness treated capability: "" as an omitted plain-suite axis, so an empty value bypassed the vocabulary check and could carry requires. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
6 tasks
abegnoche
added a commit
that referenced
this pull request
Jul 30, 2026
Introduce suites/remediation.yaml covering all open M8 break-fix issues (BFX01-BFX06): validation classes, my-isv demo stubs, and NICo provider wiring. Move BFX03-01 (HardwareSerialCheck) out of bare_metal into the new plain suite with capability gating per PR #561. NICo implements read-only observability where the tenant REST API exposes signals (maintenance events, repair history, serial inventory). Mutating lifecycle actions, GPUd/Sentinel/Maestro agents, NV switch firmware, retirement notices, and tenant notification channels emit structured skips with documented gap IDs. Stacks on PR #561 (capability/requires model). Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
abegnoche
added a commit
that referenced
this pull request
Jul 30, 2026
* feat(catalog): compose suite checks into uniquely named tests A wiring name is a test's identity in the catalog, the report, and the service, so duplicates are not cosmetic: _build_suite_map last-wins, which silently drops entries (bare_metal's GpuCheck lost to vm's). Wiring generic checks under their class names both creates those duplicates and spends two catalog names on one idea, leaving the second holding test_id: "N/A". Add a `compose:` form so a suite names the property under test and lists the generic checks that establish it. The composite is one catalog entry with one test_id; members run against the group's step output as subtests, so failures still name the part that broke and every member runs even after one fails. Migrate iam.yaml as the first suite: five wiring entries become three named ones, clearing all 4 of its uniqueness violations (99 -> 95 repo-wide). This also maps IAM02-01 (Delete user), which no check claimed before, so the plan entry gains the `iam` label the label-sync guardrail requires on both sides. Composites ship unreleased until a release commit adds them to released_tests.json; run with ISVTEST_INCLUDE_UNRELEASED=1 to exercise them. Global uniqueness enforcement stays behind ISVCTL_ENFORCE_UNIQUE_WIRING until the remaining suites are migrated. docs/test-plan.adoc is regenerated by `make plan`; it was already stale, so the diff also picks up a `storage` label on K8S23-04/05 that was in the source YAML but missing from the rendered doc. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): reserve catalog identities for named tests The generic checks name a mechanism, not a property under test, so wiring one directly spends a catalog identity on "StepSuccessCheck" and forces several unrelated tests to share it. Mark them compose_only and have the wiring validator reject them outside a composite's compose list, so a suite has to say what the test proves before it can assert it. Resolving the wiring name first means variant spellings such as StepSuccessCheck-teardown are rejected too; they leave the same mechanism as the test's public identity. The ban is scoped to the canonical suites because the catalog only reads suites/, so provider wiring never becomes a catalog entry. Enforcement joins the existing uniqueness deferral under one flag, since migrating a suite satisfies both rules at once. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(image-registry): name the tests the suite actually proves Every validation group here spent two or three catalog names on one idea, leaving the generic members holding test_id "N/A" and making the plan id land on whichever member happened to carry it. Compose each group into the test it proves, so BOOT01-01..05 and BOOT03-02 attach to a name a reader recognises and each group is one catalog entry. The instance-state members fold in too: "boots from image" is only true if the instance reaches running, and that also stops image-registry from claiming the InstanceStateCheck name that vm and bare_metal already wire. vm_ssh keeps ConnectivityCheck and OsCheck, which already say what they prove. Provider docs that listed the old per-step wiring are updated. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): finish naming the tests each suite proves Migrates the remaining six suites off directly wired generic checks, so no canonical suite spends a catalog identity on StepSuccessCheck and friends any more. Teardown groups become the thing they verify was released, the storage and object-storage groups become the volume or object behaviour they exercise, and CP10-01 and SEC21-01 now attach to a name a reader recognises instead of a bare step-success assertion. This also retires StepSuccessCheck-delete_access_key and StepSuccessCheck-delete_tenant, the variant spellings that put the mechanism in the test's public identity. bare_metal's topology_placement and host_status_logs drop their extra StepSuccessCheck rather than compose it: TopologyPlacementCheck and BmHostStatusLog both fail when the step produces no usable output, so the generic check asserted nothing they did not, and keeping their names holds CNP01-04 and BMAAS07-01 steady. Two catalog tests used StepSuccessCheck as a stand-in for a catalog entry; they now use a name that still has one, and the unreleased-filter test asserts on a composite, which is what actually ships unreleased. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(k8s): name each node-pool operation as its own test The four node-pool assertions all shared the K8sNodePoolCheck wiring name, so the catalog kept only the last one and K8S06-01, K8S06-02 and K8S06-03 had no distinct entry to attach to. Each operation now names what it proves and composes the class, which also removes the reason list form was mandatory (ordering keeps it). Adds a test for a composite forwarding its params to a member listed with none of its own: wrapping a single purpose-built check depends on it, and until now every composite passed params per member. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(storage): name the volume fixture's host check storage wired InstanceStateCheck for the instance that carries the test volume, colliding with bare_metal's setup check of the same name and letting the catalog keep only one of them. Name it for the role the instance plays here so both survive. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(image-registry): name what the booted VM's SSH checks prove vm_ssh wired ConnectivityCheck and OsCheck under their class names, which bare_metal and vm also claim, so the catalog kept one entry for all three suites. Both checks tell one story here - the image boots into an OS you can log into and it is the OS asked for - so they become one named test. This is the group deferred when image-registry was first migrated, and it sets the shape for the ssh groups still to come in vm and bare_metal. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): name each host lifecycle stage as its own test The suite asserted the same five properties at five points in the host lifecycle by wiring the same check names repeatedly. The catalog keys on the wiring name, so those repeats collapsed into one entry each and the frontend showed a single row where the suite proves five distinct things. Give every wiring a name that says which stage it proves, composing the generic check underneath. Reachability and OS are merged per stage since they share labels and only one carries a plan ID; the three host_os checks stay separate because only the PCI one is gpu-labelled and merging would pull the other two into gpu label filtering. AWS overrides start_gpu, power_cycle_gpu, and serial_console by check name, and NICo excludes SSH checks by name - both are retargeted at the new names, otherwise the override merges as a second check instead of configuring the intended one. Test IDs are unchanged, so plan coverage holds. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(vm): name each VM lifecycle stage as its own test Last suite wiring the same check names at several points in the lifecycle, so the catalog collapsed those repeats into one entry each and hid what the suite proves at start and after reboot. Name every wiring for the stage it proves, composing the generic check underneath. Unlike bare_metal, reachability and the OS image are distinct plan items here (VMAAS-XX-01 and VMAAS-XX-05), so they stay two tests per stage rather than merging into one ready check. Test IDs are unchanged. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): enforce unique wiring names and compose-only generics Both rules were written opt-in behind ISVCTL_ENFORCE_WIRING_RULES while the suites were migrated one at a time. Every suite now names what it proves, so the toggle has no remaining use and only offers a way to reintroduce the duplicates it was meant to catch. Enforce unconditionally: the validator reports reused wiring names and directly wired generic checks, and the catalog raises on a duplicate rather than dropping a test through last-wins. Uniqueness had no test of its own while it was opt-in, so add one. Drop the test asserting unmigrated suites still pass, since nothing may be unmigrated now. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(catalog): show suite/capability correctly in catalog list Platform rows were rendering as bare_metal / bare_metal because the CLI treated entry capability as a requirement. Align the column with the post-refactor model: platform suites show the capability alone; plain suites show requires (or core). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): shorten the bare-metal prefix to Bm The suite's new composite names spelled out BareMetal while the checks already in the tree use Bm (BmHostStatusLog, BmGpuHealth) and the vm suite uses Vm, so the catalog listed three spellings of the same idea. Settle on Bm. BareMetalOutput keeps its name: it is the provider step output model, not a check, and renaming it would change the JSON contract stubs implement. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor: tighten composite check wiring Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(nico): give NICo's checks a home in the suites The nico control-plane, iam, and network configs declared their own named checks, so the catalog - which reads only suites/ - never saw them, and a release would have left them permanently unreleased. Two of them restated assertions a suite already made, and the two network probes proved the same property twice behind step names inherited from Carbide that promise traffic tests neither one runs. Name each property in the suite that owns it and let nico import that suite. Checks whose steps nico does not wire skip as step_not_configured, the way nico/config/bare_metal.yaml already works. - control-plane: nico's api_health composite was byte-identical to ControlPlaneApiHealthCheck, so it is gone. That check now declares CP03-01, which an authenticated control-plane request is what proves; DATASVC-XX-01 remains covered end to end by the object-lifecycle check. - iam: add IamCallerIdentityCheck, a whoami on the credentials the run is configured with, for platforms whose IAM is read-only. - network: add a network_inventory group (VpcListedCheck, VpcInfoCheck, SubnetAssignedCheck) for platforms that hand out pre-provisioned VPCs instead of letting a tenant create them. VpcInfoCheck reclaims SDN01-02, which #452 declared and #526 had to drop to the N/A sentinel because an SDN id cannot carry the network label from a provider config. Collapse the duplicate subnet probes into check_subnet_assignment.py and take off the tenant costume: list_vpcs/get_vpc emit vpc_id and vpc_name rather than faking tenant fields so the IAM tenant checks would accept them. list_vpcs now always emits found_target, which closes a hole where a requested VPC absent from the listing still passed. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * docs: explain why the single-node on-host checks are unreleased The catalog became suite-derived in #561, so the six on-host NVIDIA-stack checks wired only in k3s/microk8s/minikube stopped being catalog entries. They still sit in released_tests.json because that file is a pre-#561 snapshot, so the next version bump regenerates it without them and they begin skipping as "unreleased". These are local-dev tools no ISV runs, so staying out of the catalog is correct; record that intent and point developers at the existing ISVTEST_INCLUDE_UNRELEASED escape hatch instead of inventing a mechanism. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(bare_metal): wire DpuNetworkCheck into the suite DpuNetworkCheck arrived in #450 alongside DpuHealthCheck but was never wired, so it has never run and the next manifest regen would drop it from released_tests.json. Publish the contract it expects next to its sibling instead of losing the class. No provider emits interfaces/bgp_enabled/dpu_extension_deployments yet, so it skips as step_not_configured exactly as DpuHealthCheck already does for providers without check_dpu_health. Claims no plan id: BMAAS06-01 is proven over SSH by the nvlink, infiniband, and ethernet checks, and an API reporting an interface "Ready" does not establish that nodes can communicate over it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): give AWS's image check a suite home, drop the dry run Both AWS-local checks predate the rule that checks live in suites, and both overlapped coverage the image-registry suite already owns. BmImageInstallationVerifiedCheck moves into the bare-metal suite claiming BOOT01-03. AWS proves that id from its bare-metal run because its image-registry run launches a VM, not a metal host, so it implements neither install_image_bm nor install_config_bm. BOOT01-03 now has two implementers, so the plan item and both checks carry the union of their labels, as the label-sync guardrail requires. BmInstallConfigUsableCheck is deleted with its step and script: an EC2 DryRun proves only that EC2 would accept the launch template, which is not BOOT01-04's "installed on a BMaaS system" and adds little over BOOT01-02's launch-template CRUD coverage. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): name properties, not mechanisms, in the remaining wirings Review found the branch violating its own README rule: VpcCrudCheck-* and SgCrudCheck-* spent eight catalog identities on a class name plus a suffix while each maps to a distinct plan id. They become named composites over VpcCrudCheck/SgCrudCheck. The other variant names are parameter variants of one property and stay as they are. BmImageInstallationVerifiedCheck overclaimed: verify_image_installed.py describes an already-running host's image id and installs nothing, so it is BmHostRunsExpectedImageCheck. That also stops it competing with BmHostBootedFromCustomImageCheck for BOOT01-03. SDN01-02's two evidence paths now say which is which: VpcReadCheck for the CRUD lifecycle, VpcReadFromInventoryCheck for a standalone read of a pre-provisioned VPC. Also: the teardown pair carries claimed-vs-confirmed in its names and descriptions, boot names take the past tense of their neighbours, and CompositeCheck now fails on a malformed compose member instead of silently running fewer checks than the config names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): prefix every bare-metal and VM check consistently Review noted the prefix rule was "prefix when it would otherwise collide", so a reader could not guess whether a bare-metal test is X or BmX: BmGpusPresentCheck sat beside GpuStressCheck, NcclCheck and NvlinkCheck, and vm.yaml called its subject both Vm and Instance. All 34 classes were wired by exactly one suite, so each is a plain class rename rather than a composite wrapper. Two names change more than their prefix: SpecifiedKeyAccessCheck becomes BmComponentKeyAccessCheck so the AUTH03-01 pair reads as siblings instead of colliding with AUTH02-01's VmLaunchedWithSpecifiedKeyCheck, and DpuNetworkCheck becomes BmDpuNetworkReadinessCheck. bare_metal.yaml and vm.yaml now have no unprefixed wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(iam): prove IAM01-01's login instead of only creating the user IAM01-01 is "create user and log in as that user", but the id sat on a check that only proved the create step succeeded and returned a username and access key. Issuing a key is not logging in. The test_credentials step already reports identity and access as separate probes, and IamCredentialAccessCheck already takes required_tests, so each plan item can prove its own half over the same output: logging in (IAM01-01) and reaching an authorized resource once logged in (IAM03-01). Creation drops to N/A as the supporting check it is, keeping min_req so a min_req-only run still creates the user the credential checks need. The check's pass message named authorized-resource access unconditionally, which was wrong once identity could be required alone; it now reports the probes it actually required. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): keep Bm off checks that are not about a host The prefix sweep stamped Bm onto nine checks whose plan items are not bare-metal-scoped: CAP01-01 fleet-wide governance counts, CAP05-02 aggregation at cluster/nodegroup/reservation level, SDN04-04/05 IB fabric, STG02/03/04/05 storage service, and HWING01-01 fleet inventory. They sit in bare_metal.yaml because that is where NICo's config imports from, not because the requirement is per-host, so the name asserted a scope the plan does not have. Our own labels already said so - the IB pair carries network, the STG four carry sds_controller. It would also have cost us later: names are globally unique and a wiring holds one test_id, so the first of these to need a network.yaml or storage.yaml home would have produced the two-names-one-id split we just undid for BOOT01-03. The prefix rule is now stated in the suites README: it follows the subject (one host, one VM), not the suite and not the test_id's requirement family, which is why BmHardwareSerialCheck (BFX03-01) and BmCloudInitCheck (BOOT02-01) keep theirs. Suite placement for these nine is a separate question, since moving them means NICo importing those suites too. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(suites): enforce compose_only at runtime and fold the VM launch rows compose_only was authoring-time only: validate_suite_wiring rejects a generic check wired under its own name, but an ISV's config is never linted, so it would run and report a pass under a name that says nothing about the property proven. parse_validations now rejects it too, as an invalid-config error naming the fix. Config parsing is the right place, not class resolution: the rule is about how a check is wired, so putting it there leaves internal pre-resolved entries alone. Four fixtures wired the generics directly and now compose them instead, which is what the rule asks of any config. vm.yaml also had two rows on launch_instance for one event - VmRunningCheck at N/A beside VmCreatedCheck at CNP01-09 - where bare_metal.yaml proves the same launch with one. The class becomes VmInstanceIdReportedCheck so the VmCreatedCheck composite can claim CNP01-09 over both the returned id and the running state. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(cli): apply the config's exclude block in the dry-run plan --dry-run read only the CLI's --exclude-label, ignoring the exclude block in the config itself, so it promised checks a real run then skips. The AWS bare-metal plan listed BmSerialConsoleRetentionCheck as [RUN] even though that config excludes it by name, and k3s listed ten workload checks its own exclude labels always drop. Both halves of the block now apply, as the orchestrator already does: labels union with the CLI's, and names are matched first to mirror resolve_entries' precedence. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(orchestrator): say when a step is skipped rather than unconfigured A step carrying skip: true is deliberately left out of the step-phase map so its validations skip with it, but that reused the "no such step" path, so AWS's four reinstall checks reported step_not_configured for a step the config declares. The two cases differ in what an operator can do: skip: true is one flag away from coverage, a step the provider never declares is not. resolve_entries now takes the skipped step names and reports step_skipped with "configured but skipped (skip: true)". Checks bound to a step the provider genuinely lacks - AWS has no check_dpu_health or verify_ingestion - keep reporting step_not_configured. Note for downstream: skip reasons become the type attribute on JUnit <skipped> elements, so step_skipped is a new value there. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * chore(aws): exclude the credential-readiness check from the IAM suite IamCredentialsAuthenticateCheck is a whoami on the run's own credentials, for platforms whose read-only IAM cannot create a user and log in as them. AWS does exactly that, so IAM01-01 and IAM03-01 already prove the stronger property and the check only added a skipped line to every AWS IAM run. Excluded by name rather than removed from the suite: NICo implements check_credentials and still needs it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(catalog): close composite review gaps Keep dry-run filtering and compose-only enforcement aligned with runtime behavior, and synchronize the related suite and operator documentation. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(iam): drop the duplicate NICo credential probe from the suite check_credentials made the same two calls as NICo's control-plane check_api - forge_get on the site, then forge_get_all over sites - with the probes renamed. CP03-01 already covers "an authenticated request scoped to the account reaches the platform API", and it maps to a plan item where the IAM copy was N/A. Hoisting it into the shared suite also made every AWS IAM run report a skipped row for a check AWS has no step for, which needed muting. Removing it from the suite drops both the row and the mute: AWS's IAM run is four checks and no skips again. NICo's script stays on disk unwired, in case its IAM story changes. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * docs(my-isv): stop listing check names in coverage comments Enumerating every validation in header comments drifts as suites change; describe JSON-contract coverage and SSH exclusions instead. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): drop prefixes the subject rule does not earn The prefix sweep and the walk-back after it left four names claiming more than they prove. Each was globally unique before the sweep, so the prefix bought consistency only, and each cost a released_tests.json entry. VirtualDeviceHardeningCheck (CNP01-17) already carries its subject: VmVirtualDeviceHardening stutters, and USB/clipboard/virtual-device passthrough has no bare-metal reading to distinguish it from. SerialConsoleRetentionCheck (CNP06-02) asserts that the console archive keeps a month of history, which belongs to the logging service rather than to a host. It is also the one console plan item that is not platform-scoped, unlike CNP06-01 (bare metal) and CNP06-03 (VM). HostHealthCheck (CAP05-01) is per-host by subject, but it is one half of a pair whose other half - HealthAggregationCheck (CAP05-02) - is fleet-level and already unprefixed. Prefixing one half hid that the two are one requirement seen at two scopes. BmDpuNetworkCheck keeps its prefix, since a DPU is per-host, and drops "Readiness": the check asserts interface status, BGP, and extension deployments, which the vaguer word described less well. It is free to change, carrying test_id N/A with no implementation yet. Three of the four land on names already in released_tests.json, so unreleased wiring names drop 125 -> 122 and dangling manifest entries 25 -> 22. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
added a commit
to abegnoche/ai-cloud-validation
that referenced
this pull request
Jul 30, 2026
The kubernetes storage checks read StorageClass names from steps.setup_cluster.csi, but NVIDIA#561 moved them out of k8s.yaml -- which supplies that fixture via the scaffold stub -- into storage.yaml, which supplied nothing. The consumers moved and the producer did not follow, so the names could only arrive through K8S_CSI_* env vars, and the scaffold's discovery of the classes already installed on a cluster became dead output: nothing reads steps.setup.csi any more. Point storage.yaml at the same stub k8s.yaml uses. It reports what the cluster already has and provisions nothing, so it is safe unattended, and a provider's own steps replace the list wholesale -- verified that aws keeps eks/setup.sh and my-isv keeps storage/setup_cluster.py. teardown_cluster comes along for symmetry with the contract both providers carry. Both halves are gated on kubernetes so they skip as a pair; a teardown outliving its fixture is the hole 80fb1da closed in three other suites. Also add the static_volume_az the scaffold's csi block was missing, which aws already emits and storage.yaml already reads. Pre-existing -- the warning is in v0.9.0 logs -- and hidden while the producer was absent. Verified on a 4-node lab through deploy: setup_cluster runs, the kernel module check probes every node, teardown runs, no masked-default warnings. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: support validation-only suite runs Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat: support providerless suite selection Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix: name a commandless run after its environment, not its context A config with no commands borrowed its identity from the requirement context, so a run with no --capability was labelled `core` -- the CLI's word for "no capability", which names no execution environment. It surfaced in the orchestration logs and in JUnit suite names as `core/test`, and it left VALIDATIONS_ONLY_PLATFORM unreachable. Borrow the context only when it names one of the four capabilities. Also state the invariant behind the kubernetes storage groups carrying no `step:`, since the tempting edit is to bind them back to the provider fixture, which is what broke the validation-only path. The guard test's set-equality assertion passed trivially; assert the groups are wired instead. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(deploy): keep the pytest separator on the remote command line `deploy run ... -- -v -s -k Check` appended those args straight onto the remote `isvctl test run`, with no `--` of its own. That only worked while `test run` swallowed unknown options and forwarded them from ctx.args; b4d7c74 made it reject them, so every deploy carrying pytest args died on "No such option: -s" before a single check ran. Reproduce the separator remotely, and name the args in the echoed command -- the echo omitted them, so the failing line looked like the one that worked and the args looked like they had vanished in transit. Verified against a real jumphost target: the forwarded -k selects the check and it runs. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(deploy): select a suite and capability, and reject unknown options `deploy run` was the last command still accepting unknown options, so `--suite kubernetes` -- a flag it did not have -- was silently folded into the pytest passthrough instead of being rejected. With a config also supplied it would have reached the remote command as `-- --suite kubernetes ...` and failed inside pytest, a long way from the cause. Reject unknown options before `--`, as `test run` already does. Then give deploy the flags it was missing. `--suite` resolves the canonical suite the same way `test run --suite` does with no provider, and `--capability` is forwarded to the remote run. Both are resolved before the archive is built, so a rejected combination costs no SSH round trip. `--capability` also has to reach reporting: the recorded capability was derived from the config alone, so `--suite storage --capability kubernetes` would have recorded a core run while executing the kubernetes checks -- a different check set than the pair says was run. Provider configs still need `-f`: --suite reads the canonical catalog only. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(resolution): do not warn when a reference had nothing to look in The custom default(...) filter warns whenever it swallows an Undefined, so that a typo like `steps.setup.node_cout | default(1)` cannot read as a legitimately empty value. A validation-only run has no steps at all, so every `steps.<name>` in the suites is undefined by construction: running `--suite storage --capability kubernetes` printed the same line 27 times and pushed the results off the screen. A name missing from an empty container cannot be a misspelling of what is in there, so stay quiet in that case only. Looking into something that does have contents and missing still warns -- which is the case the filter was written for, and the one that fires in the field: a k8s.yaml run against a real cluster keeps all four of its warnings (api_endpoint, cluster_autoscaler_deployment/namespace, unauthorized_probe_cmd), where the step ran and did not emit the field. Measured across every plain suite in core/vm/kubernetes and every my-isv demo config: 27 warnings before, 0 after, with the four real ones intact. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(deploy): forward the release gate to the remote test run Only NGC_API_KEY ever crossed: `sudo -E` preserves the target's own environment, not the caller's, and deploy never passed `env=` to SSHClient.execute. So `ISVTEST_INCLUDE_UNRELEASED=1 isvctl deploy run ...` set the flag in the local process and the remote run applied the release gate anyway, silently dropping every unreleased check -- the storage team's NFS checks were unreleased when they wrote their runs, so the deploy equivalent of their command would have exercised nothing. Take the variable name from isvtest.release_manifest rather than repeating the string, and keep the NGC_NIM_API_KEY alias intact while collecting both assignments in one place. Deliberately not widened to every variable: path-valued ones like ISVCTL_CONFIG name files that exist only on the calling side, and a remote command line is visible to `ps` there, which is exposure NGC_API_KEY already carries but AWS and ISV secrets should not inherit without asking. The guide claimed every listed variable was forwarded; the four ISV_* upload variables are read locally, since deploy reports the run itself. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(cli): apply --color to the results summary, not only to pytest The flag was forwarded to pytest and nowhere else, so isvctl's own output kept click's terminal auto-detection. Under `deploy` the remote stdout is a pipe -- `ssh -T` allocates no pty and the command is piped into tee -- which produced a log with colored pytest output next to an unstyled orchestration summary, making a PASSED validation look unlike the PASSED pytest reported for the same check. Set it on the click context rather than per call site: echo resolves its color default from the active context, so one assignment covers the whole summary instead of a color= argument on each of its echoes. `--color=no` now also suppresses color in a terminal, which it previously could not do for anything but pytest. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor: state the suite selection rules once each The providerless-suite work grew a second copy of several rules. Fold each back to one statement, with no change in behavior: - `--suite` selection was implemented twice, once in `test run` and once in `deploy run`, down to two verbatim user-facing strings. `select_suite` now owns the rule it excludes and the wording that announces the choice. - `_reporting_suite_and_capability` decided whether a config was a platform suite, discarded the answer, and left its caller to re-derive it from a second walk of the suite catalog. The whole capability rule is now one function with one lookup. - The NGC key's legacy-alias fallback was retyped over `isvtest.core.ngc.get_ngc_api_key`, which documents it. - `has_lifecycle` restated "are there commands" as an if/else, two `and` guards, and a ternary; one fork answers all four at once. - `resolve_suite` asked `provider is not None` three times in a row, which hid the fact that only the provider branch validates its directory. Also inline a single-use temporary in `_detect_platform`, and lift the ANSI escape pattern the CLI tests strip with into `conftest`. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix: fail a commandless run that wires no validations Addresses review feedback on #564. Validations are all a run without commands has to offer, so a config that wires none asserts nothing -- and the phase loop reported that as `[PASS] All phases completed successfully` with an empty results table and exit 0. Newly reachable: before this branch the same config died at platform detection, so an empty or miswired `-f` could not reach the loop. A lifecycle run still has its steps to answer for, so the guard is scoped to validation-only mode rather than requiring validations everywhere. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(storage): give the suite the csi fixture its checks read The kubernetes storage checks read StorageClass names from steps.setup_cluster.csi, but #561 moved them out of k8s.yaml -- which supplies that fixture via the scaffold stub -- into storage.yaml, which supplied nothing. The consumers moved and the producer did not follow, so the names could only arrive through K8S_CSI_* env vars, and the scaffold's discovery of the classes already installed on a cluster became dead output: nothing reads steps.setup.csi any more. Point storage.yaml at the same stub k8s.yaml uses. It reports what the cluster already has and provisions nothing, so it is safe unattended, and a provider's own steps replace the list wholesale -- verified that aws keeps eks/setup.sh and my-isv keeps storage/setup_cluster.py. teardown_cluster comes along for symmetry with the contract both providers carry. Both halves are gated on kubernetes so they skip as a pair; a teardown outliving its fixture is the hole 80fb1da closed in three other suites. Also add the static_volume_az the scaffold's csi block was missing, which aws already emits and storage.yaml already reads. Pre-existing -- the warning is in v0.9.0 logs -- and hidden while the producer was absent. Verified on a 4-node lab through deploy: setup_cluster runs, the kernel module check probes every node, teardown runs, no masked-default warnings. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
added a commit
that referenced
this pull request
Aug 4, 2026
Introduce suites/remediation.yaml covering all open M8 break-fix issues (BFX01-BFX06): validation classes, my-isv demo stubs, and NICo provider wiring. Move BFX03-01 (HardwareSerialCheck) out of bare_metal into the new plain suite with capability gating per PR #561. NICo implements read-only observability where the tenant REST API exposes signals (maintenance events, repair history, serial inventory). Mutating lifecycle actions, GPUd/Sentinel/Maestro agents, NV switch firmware, retirement notices, and tenant notification channels emit structured skips with documented gap IDs. Stacks on PR #561 (capability/requires model). Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
abegnoche
added a commit
that referenced
this pull request
Aug 7, 2026
…562) * feat(remediation): add BFX break-fix suite with NICo gaps documented Introduce suites/remediation.yaml covering all open M8 break-fix issues (BFX01-BFX06): validation classes, my-isv demo stubs, and NICo provider wiring. Move BFX03-01 (HardwareSerialCheck) out of bare_metal into the new plain suite with capability gating per PR #561. NICo implements read-only observability where the tenant REST API exposes signals (maintenance events, repair history, serial inventory). Mutating lifecycle actions, GPUd/Sentinel/Maestro agents, NV switch firmware, retirement notices, and tenant notification channels emit structured skips with documented gap IDs. Stacks on PR #561 (capability/requires model). Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> * refactor(breakfix): fold BFX suite into bare_metal and k8s Break-fix is mostly bare-metal plus k8s-only cordon/GPU reset, so a plain remediation suite is not justified the way storage is. Wire BM BFX into bare_metal, k8s actions into k8s, and rename scripts/modules to breakfix. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(nico): merge key_access config into bare_metal Both nico configs imported suites/bare_metal.yaml, so `--provider nico --suite bare_metal` could not resolve and demanded an explicit -f. This was the only such collision in the repo. Fold key_access.yaml's setup/teardown steps into bare_metal.yaml and delete the file. The test step was already identical in both; only the throwaway-key provisioning was unique. Those two steps carry `skip: true` so a routine run stays read-only -- setup_key_access mutates the site (POST sshkey + sshkeygroup, best-effort PATCH of the SSH-key SOL flag), which should not be a side effect of ordinary bare-metal validation. Unskip both to make AUTH-XX-03 pass without a pre-synced key. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(breakfix): sync test-plan labels with BFX suite wiring The test-plan coverage guardrail requires each plan entry's labels to equal the union of labels across the suite wirings declaring that test_id. Folding the BFX checks into bare_metal.yaml and k8s.yaml added platform labels to the wiring but not to docs/test-plan.yaml, leaving `make test` red on all 13 BFX entries. Add bare_metal to the eleven bare-metal entries, and kubernetes to BFX01-01 and BFX01-04, which live in the k8s suite. Also apply the pre-commit import ordering and whitespace fixes the BFX commits missed. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(breakfix): require real evidence for BFX02 maintenance and repair checks Against a live NICo site both checks passed while proving nothing. events_queryable and history_queryable are set by the provider script the moment the machine-list call succeeds, so MaintenanceEventsCheck passed with zero events -- it would pass identically against a site with no maintenance capability at all. Skip instead of passing when the evidence list is empty. An empty list cannot distinguish a working query API from one that returns nothing, and a provider should not clear BFX02 without demonstrating the capability. A failed or absent API still fails, so the stricter reading only affects the vacuous case. Also stop counting InUse as repair history. _REPAIR_STATUSES is {Maintenance, Reset, Error, Repairing}, but the filter admitted InUse too -- a normal lifecycle state -- so machines that had never been repaired produced repair records. Verified on a NICo dev site: BFX02-01 now skips (site has no maintenance events), BFX02-03 still passes on genuine Error history across all 6 machines. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(breakfix): require real evidence for BFX02-02 retirement notices RetirementNoticesCheck kept the vacuous-pass pattern that BFX02-01 and BFX02-03 shed in 3c2df67: notices_queryable is set by the provider script whenever the API call succeeds, so the check passed on an empty notice list. The my-isv demo made this visible by passing with "0 notice(s) at site". Skip on an empty list, matching the other two BFX02 checks. Give the my-isv stub a sample notice so the demo still exercises the pass path. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(breakfix): address CodeRabbit review on the BFX suite Correctness: - CordonNodeCheck passed when `existing_workloads_running` was absent, since the guard only rejected an explicit False. BFX01-04 needs positive evidence that existing workloads kept running, so require exactly True. - NodeHealthAgentCheck passed with "0 node(s)" when `agents` was empty. BFX04-01 asks whether GPUd/Sentinel is running; no agent records means none is. - `list_site_machines` caught only NicoAuthError, so an HTTPError or a decoding failure from `forge_get_all` escaped and killed the script before `emit()`, leaving the orchestrator with no JSON at all. Catch URLError and ValueError (covering HTTPError and JSONDecodeError) and return a structured failure. Tests: - `test_planned_notification_passes` instantiated FailureNotificationCheck, so PlannedMaintenanceNotificationCheck had no coverage. Test both, and add cases for the two correctness fixes above. Docstrings: add the PEP 257 docstrings required by python-standards.mdc to the break-fix validations and the my-isv/NICo provider scripts. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * test(breakfix): add docstrings to the break-fix unit tests Docstring coverage across the PR's Python files was 78%, below the 80% threshold. The remaining gaps were all in test_breakfix.py: 7 test classes and 8 test methods. Every changed Python file is now at 100%. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(breakfix): reduce duplication across the BFX suite Quality pass over the break-fix suite. No behavior change: every BFX check emits the same message it did before, verified end to end with ISVCTL_DEMO_MODE=1 ISVTEST_INCLUDE_UNRELEASED=1. validations/breakfix.py: - Merge _maybe_skip and _require_success into one _step_output helper, dropping a two-hop indirection and the preamble repeated at 13 call sites. - Collapse nine copy-paste classes into three ClassVar-parameterized bases (_QueryableRecordsCheck, _OperationCheck, _NotificationChannelCheck), following the _AttestationCheck pattern. Each test ID keeps its own discoverable class; the bases set _exclude_from_discovery. The "empty record list must skip, not pass" invariant now lives in one place. NICo scripts: - Replace nine gap stubs (two dialects, ~250 lines) with one table-driven gap_stub.py. An unknown gap ID is now rejected by argparse instead of silently emitting a wrong payload. - reset_gpus.py was wired in no NICo config; drop it and its gap row. - Make skip_result's gap argument required (it was never omitted) and move the repeated skip/error preamble into list_site_machines. my-isv scaffold: - Replace the three-function _stub.py with a single emit_stub in scripts/common/stub.py, establishing the scripts/common/ package that AGENTS.md documents, and use the conventional parents[1] path insert. The scaffold copies the tree wholesale, so generated providers inherit it. Contract trimming, per the "keep output minimal" discipline: drop fields no validation reads (hardware_id, opened_at, entry_count, action, sample_event, and the duplicate machine_id alias in reset_gpus). Dead code: NICO_MACHINE_ID/NICO_RACK_ID settings, redundant default filters over already-declared settings, a vestigial `_ = args`, and a docstring pointing at the deleted key_access config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(nico): run the throwaway key-provisioning steps by default setup_key_access / teardown_key_access mint and remove an ephemeral SSH key so AUTH-XX-03 (BmComponentKeyAccessCheck) can pass without manual key setup. They were wired but carried `skip: true`, which left query_key_access with no synced key group to read and made it skip on a routine run. Enabling them by hand meant editing this tracked file: deep_merge replaces lists wholesale, so a multi-`-f` overlay cannot flip `skip` on a single step without restating the whole `steps` list. This makes the config mutate the site (POST sshkey + sshkeygroup, best-effort PATCH of the site's SSH-key SOL flag). teardown_key_access reverses all of it and runs even after a setup or test failure, per the lifecycle invariants. Comments and prerequisites updated to drop the opt-in framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(nico): make the AUTH-XX-03 key check self-contained The throwaway SSH key was provisioned in a setup step and removed in a teardown step, so anything that skipped teardown -- `--phase test`, `teardown_on_failure` disabled, an early exit -- leaked a key and a key group onto the site. Fold it into one test-phase step instead, matching the create/test/remove pattern the AWS scripts already use (sg_crud_test.py, provision_storage_test.py): query_key_access.py now mints the key when the site has none synced, observes the access path it unlocks, and removes it in a `finally`. A leaked key fails the step via `cleanup_errors` rather than passing silently. - New auth/_key_access.py holds provision/remove. `provision` records each ID into the caller's ThrowawayKey as it goes, including on a mid-provision failure, so the `finally` can always clean up what was actually created. - setup_key_access.py and teardown_key_access.py are deleted along with their config steps; the config is now test-only (phases: ["test"]). - Timeout raised 120s -> 420s. subprocess.run SIGKILLs on timeout, so a timeout shorter than the 180s key-group sync poll would strand the key -- the exact leak this change removes. - --no-provision keeps a run strictly read-only; --skip-cleanup leaves the key for debugging. Dropping the setup->teardown handoff also removes the Jinja round-trip that turned a Python None into the string "None", so teardown no longer has to parse a stringified tri-state flag. A hard kill can still strand the key; it is named isvtest-auth-xx-03-<hex> and its private half is never kept, so it grants nobody access. Tests cover provision/remove directly plus the self-contained flow: cleanup on the failure path, cleanup failure failing the step, --skip-cleanup, and --no-provision. The script loader now registers modules in sys.modules while executing, which @DataClass needs to resolve deferred annotations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(breakfix): require real evidence for BFX05/BFX06 notifications BFX05-01 and BFX06-01 passed on a single provider-emitted boolean, notification_channel_observable. A provider could set it true for a channel it never queried and the check would pass with zero evidence -- the same vacuous pass removed from BFX02 in 3c2df67 and b537792. my-isv's own demo stub did exactly that. Hold both to the BFX02 bar: the step must report the channel observable *and* return at least one notification record. That makes them subclasses of _QueryableRecordsCheck, so _NotificationChannelCheck goes away and one policy now covers every "is this signal observable" requirement. Contract adds notifications[].{machine_id,type,message,notified_at}; the NICo gap payloads carry the key as an empty list, and my-isv's stubs emit a sample record so a demo run still evidences a pass. Also close the same hole one level down in BFX02-03: RepairHistoryCheck counted machine records without inspecting them, so records: [{}] or a record with entries: [] passed. The shared base now filters on an _is_evidence predicate (default: any non-empty record) and RepairHistoryCheck requires non-empty entries, so entryless records no longer inflate the count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * build(security): stop TruffleHog scanning artifacts and the Lob detector `make security-trufflehog` scanned the whole working tree, including .venv, .git and .pytest_cache, so it covered 332MB of vendored dependencies and cached test IDs that CI's fresh checkout never sees. Add .trufflehog-exclude and pass it via --exclude-paths: 35957 chunks drop to 1208. That alone still failed. Every finding came from the Lob detector, whose key pattern is `test_` + 35 characters -- so its verifier reports any 40-character pytest function name as a verified secret, 51 of them across pre-existing test files. Nothing here uses Lob (a postal mail API), so the detector can only produce false positives; exclude it and the scan is clean. Also rename the two 40-character test names this branch added, so the suite stops contributing to the pattern regardless of detector configuration. SECURITY_SKIP_DIRS is left alone: it is unreferenced, and its comma-separated form is Trivy's --skip-dirs syntax, not a TruffleHog flag. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * ci(security): exclude the Lob detector from the TruffleHog scan The CI job calls the shared dsx-github-actions scanner, so it never reads the Makefile flags added in 9d0f7e4 and kept failing on the same false positives: Lob's key pattern is `test_` + 35 characters, so its verifier reports every 40-character pytest function name as a verified secret. There are 51 such names across pre-existing test files, and because verification depends on live API responses the check was flaky rather than consistently red. Nothing in this repository uses Lob, a postal mail API, so the detector can only produce false positives. Excluding it loses no coverage and leaves every other detector, including the SSH and private-key ones, active. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(nico): fail provisioning when a create response carries no id provision() stored `key.get("id") or ""`, and remove() skips a resource whose id is empty. A create that returned a body without an id therefore left the key on the org permanently -- the exact leak the self-contained key flow was written to eliminate. The empty id also propagated into the group's sshKeyIds. Raise on a missing id for both the key and the group; the group id has the same problem and additionally reaches _wait_for_sync, which would poll `sshkeygroup/`. Also drop the hardcoded 2026-07-01 date from the my-isv planned-notification fixture: it is now in the past, so the demo described completed maintenance as planned_maintenance. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
What
Introduces the suite / capability model for selecting which validations an ISV runs, replacing the per-check
platforms:axis.kubernetes,vm,bare_metal,slurm) declaretests.capability; plain suites (storage,iam,network, …) declare per-checkrequires: [...].requiresis any-match: empty means core (runs for everyone), otherwise it names the capability in play. AND across capabilities is deliberately inexpressible — the four are mutually exclusive execution environments.isvctl test run --provider P --suite S [--capability C].--capabilityis single-valued for the same reason.schemaVersion: 2, carryingsuite/requiresper entry and thecapabilities/suitesvocabularies.(suite, capability)pair rather than a single inferredtest_target_type.Vocabulary: platform → capability
Supersedes #560 (being closed).
tests.platform:→tests.capability:, and the catalog wire'splatforms/platform→capabilities/capability.commands.<key>keys are unchanged but described as test target, since they span capabilities and plain suites (iam,network,security).requiresdeliberately keeps its name: it is the predicate on a check,capabilityis the value.requires: [kubernetes]reads correctly;capabilities: [kubernetes]on a check would read as "this check has kubernetes".tests.platform:is now rejected rather than ignored — the model allows extras, so an unmigrated config would otherwise silently demote to a plain suite and gate every check on a capability it no longer declares.The backend accepts both spellings on upload, so a lab on an older isvctl is unaffected.
Depends on the service side
Requires the corresponding ISV Lab Service change (V18) deployed first — a v2 catalog uploaded to an older service loses the new fields.
Review feedback (3bfae25)
update_test_runstill readcatalog_document["platforms"]and passedplatforms=, raising KeyError into its ownexcept Exception → warning— no failure, just no coverage data.--test-cataloghad the same break, and now preferscapabilitieswith aplatformsfallback for v1 files on disk.catalog_document()itself.platformdocstrings, an incorrect--platformdoc bullet, a docstring contradicting itself on skip granularity, two over-long lines.resolve_suiteclassified a provider's configs without catchingSuiteResolutionError, so one malformed file broke--suitefor that whole provider. Pre-existing.Known deferrals
ISVCTL_ENFORCE_UNIQUE_WIRINGgates an intentionally inert uniqueness check;CORE_REQUIREMENT_CONTEXTis a CLI-local sentinel translated toNonebefore upload;--dry-rununder-reports some terminal skips.isvreporter --platformandisvtest --platformkeep their spelling — both are bound to the deprecatedtest_target_typeand the legacy isvtest schema.Testing
make test(1452 isvctl + 1279 isvtest + 58 isvreporter + 113 script),make lint,make demo-test,validate_suite_wiring.py --checkanduvx pre-commit run -aall pass. Both catalog-upload call sites verified to bind against the real signature and a real envelope, including the v1 fallback. Every commit DCO signed off.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary
New Features
requires, plusisvctl test runcapability context threaded through orchestration and reporting.capabilities/suites/requiresmetadata; reporting now records suite/capability.Improvements
--exclude-label;--dry-runnow shows a human-readable plan.Documentation
capability/requiresand refreshed demo-mode instructions.