feat(breakfix): BFX break-fix suite (BFX01-BFX06) with NICo coverage - #562
feat(breakfix): BFX break-fix suite (BFX01-BFX06) with NICo coverage#562abegnoche wants to merge 15 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded BFX01–BFX06 validation checks for bare-metal and Kubernetes workflows. Added NICo break-fix queries and temporary SSH-key lifecycle handling. Added my-isv break-fix configurations, executable scaffold scripts, and configurable TruffleHog exclusions. ChangesBreak-fix validation contracts and coverage
NICo temporary key lifecycle
NICo break-fix workflows
my-isv break-fix templates
TruffleHog scan configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant SuiteConfig
participant NicoMaintenanceQuery
participant NICoAPI
participant MaintenanceEventsCheck
TestRunner->>SuiteConfig: Execute maintenance-event validation
SuiteConfig->>NicoMaintenanceQuery: Pass organization, site, and API base
NicoMaintenanceQuery->>NICoAPI: List site machines
NICoAPI-->>NicoMaintenanceQuery: Return machine status data
NicoMaintenanceQuery-->>MaintenanceEventsCheck: Emit normalized maintenance events
MaintenanceEventsCheck-->>TestRunner: Report pass, skip, or failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
85d9bae to
52bc942
Compare
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
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>
52bc942 to
4a7e1b9
Compare
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>
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>
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>
… 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>
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>
🔐 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-08-06 16:12:24 UTC | Commit: 9d0f7e4 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
isvctl/configs/providers/nico/config/bare_metal.yaml (1)
565-571: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
machine_idandrack_idare declared but never passed to a step.No step in this file consumes
{{machine_id}}or{{rack_id}}. The mutating break-fix scripts accept only--org,--site-id, and--api-base. If these settings exist for the node and rack maintenance workflows, wire them into the matching stepargs. If they are placeholders for a later change, add a short comment stating that.🤖 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/providers/nico/config/bare_metal.yaml` around lines 565 - 571, Update the settings and related maintenance steps in the provider configuration so machine_id and rack_id are either passed through their matching step args using the existing template values, or marked with a brief comment as intentional placeholders if no current step supports them. Ensure no declared setting remains unused without that clarification.isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py (2)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour break-fix stub scripts duplicate the same inline gap-reporting block. Each script repeats the same argparse setup,
parse_known_argscall, skip payload, andsys.exit(emit(...))inside the module guard, with the namesp,a, andr. Only the skip reason, gap ID, andoperationkeys differ. Add one shared helper inbreakfix/_common.pythat takes the reason, gap ID, and operation payload, then have each script call it from a typedmain().
isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27: call the shared helper with gapBFX01-05and{"requested": False, "node_removed_from_pool": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py#L15-L27: call the shared helper with gapBFX01-01and{"requested": False, "completed": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py#L15-L27: call the shared helper with gapBFX01-02and{"requested": False, "accepted": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py#L15-L27: call the shared helper with gapBFX01-03and{"requested": False, "accepted": False}from amain() -> intfunction.🤖 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/providers/nico/scripts/breakfix/request_host_replacement.py` around lines 15 - 27, Replace the duplicated module-guard logic with a shared typed helper in isvctl/configs/providers/nico/scripts/breakfix/_common.py that accepts the skip reason, gap ID, and operation payload, parses the existing --org, --site-id, and --api-base arguments, emits the skip result, and returns an exit code. In isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27, call it from main() -> int with gap BFX01-05 and operation {"requested": False, "node_removed_from_pool": False}; apply the same main() structure in reset_gpus.py#L15-L27 with BFX01-01 and {"requested": False, "completed": False}, return_node_maintenance.py#L15-L27 with BFX01-02 and {"requested": False, "accepted": False}, and return_rack_maintenance.py#L15-L27 with BFX01-03 and {"requested": False, "accepted": False}.
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the stub logic into a typed
main()function.
query_bmc_kernel_logs.pyin the same directory definesdef main() -> int:and callssys.exit(main()). This script puts all logic in the module guard and uses the namesp,a, andr. The same shape repeats in the other break-fix stub scripts. Align the stubs with themain()convention and use descriptive names.🤖 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/providers/nico/scripts/breakfix/request_host_replacement.py` around lines 15 - 27, Move the argument parsing and stub-result construction from the __main__ guard into a typed main() -> int function, then invoke it with sys.exit(main()). Within main(), replace the abbreviated p, a, and r variables with descriptive names while preserving the existing required arguments, skip_result call, operation payload, and emitted exit status.isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py (1)
23-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe generic
"log"keyword makes the signal test broad.
_LOG_KEYWORDScontains"log", so any BMC probe whose text mentions logging counts as a kernel-log entry.entry_countthen reports matched probes, not log entries. Consider dropping"log"or renaming the reported field tomatched_probe_countsoBmcKernelLogCheckevidence stays accurate.🤖 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/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py` at line 23, Remove the generic "log" token from _LOG_KEYWORDS so BmcKernelLogCheck only matches specific kernel-log indicators and entry_count continues to represent log entries rather than broad probe matches.isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 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/providers/nico/scripts/breakfix/reset_gpus.py` around lines 15 - 27, Refactor the __main__ block into a typed main() function, matching the structure used by query_bmc_kernel_logs.py. Replace abbreviated variables such as p, a, and r with descriptive names, preserve the existing argument parsing, skip_result call, operation payload, and emitted exit status, and invoke main() from the __main__ guard.isvctl/configs/providers/nico/scripts/breakfix/_common.py (1)
59-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to
emit,machine_labels, andhistory_entries.The coding guidelines require a docstring on every function.
list_site_machinesandskip_resulthave one; these three do not.♻️ Proposed docstrings
def emit(result: dict[str, Any]) -> int: + """Print the result as JSON and return the process exit code.""" print(json.dumps(result, indent=2)) return 0 if result.get("success") else 1 def machine_labels(machine: dict[str, Any]) -> dict[str, str]: + """Return the machine labels as a string-to-string mapping, dropping null values.""" labels = machine.get("labels") or {} if not isinstance(labels, dict): return {} return {str(k): str(v) for k, v in labels.items() if v is not None} def history_entries(machine: dict[str, Any]) -> list[dict[str, Any]]: + """Return the machine status-history entries that are dict objects.""" history = machine.get("statusHistory") or [] return [entry for entry in history if isinstance(entry, dict)]As per coding guidelines: "Every function and class must have docstrings following PEP 257".
🤖 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/providers/nico/scripts/breakfix/_common.py` around lines 59 - 73, Add concise PEP 257-compliant docstrings to the emit, machine_labels, and history_entries functions, documenting each function’s purpose and return value while preserving their existing behavior and logic.Source: Coding guidelines
isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 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/providers/nico/scripts/breakfix/return_node_maintenance.py` around lines 15 - 27, Refactor the module-level CLI logic in the __main__ block into a typed main() function, using descriptive names instead of p, a, and r while preserving the existing arguments, skip_result call, operation payload, and emitted exit status. Update the entry point to invoke main(), matching the structure used by query_bmc_kernel_logs.py.isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 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/providers/nico/scripts/breakfix/return_rack_maintenance.py` around lines 15 - 27, Refactor the __main__ block into a typed main() function, moving argument parsing and result construction into it. Replace abbreviated variables such as p, a, and r with descriptive names, matching the structure and naming style used by query_bmc_kernel_logs.py, while preserving the existing arguments, skip_result call, operation values, and exit behavior.
🤖 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/configs/providers/my-isv/scripts/breakfix/_stub.py`:
- Around line 21-30: Add concise PEP 257 docstrings to the new functions:
document the return contract of base_result and demo_or_not_implemented in
isvctl/configs/providers/my-isv/scripts/breakfix/_stub.py (lines 21-30), and
document the behavior of each main function in cordon_node.py (17-33),
query_bmc_kernel_logs.py (17-29), query_failure_notifications.py (17-30),
query_maintenance_events.py (17-39), query_planned_notifications.py (17-30),
query_repair_history.py (17-42), query_retirement_notices.py (17-38), and
query_serial_numbers.py (17-42). Ensure every function has a concise PEP
257-compliant docstring without changing its behavior.
In
`@isvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.py`:
- Around line 17-30: Add concise PEP 257-compliant docstrings immediately inside
each main function, describing its template operation:
query_node_health_agents.py (lines 17-30) for the node health-agent query,
query_switch_firmware.py (lines 17-29) for the switch-firmware query,
request_host_replacement.py (lines 17-35) for host replacement, reset_gpus.py
(lines 17-31) for GPU reset, return_node_maintenance.py (lines 17-36) for node
maintenance, and return_rack_maintenance.py (lines 17-31) for rack maintenance.
Preserve the existing argument parsing and result flow.
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 17-29: Update list_site_machines around the forge_get_all call to
catch the expected HTTP, URL, response-decoding, and JSON exceptions in addition
to NicoAuthError. For each handled failure, set result["error"] to the exception
text and return [], result so emit() always receives structured JSON.
In `@isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py`:
- Around line 24-32: Add concise PEP 257-compliant docstrings to every specified
new function: `_opened_at` and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py`,
`_repair_entries` and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_repair_history.py`, `main`
in `isvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.py`,
and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.py`;
preserve each function’s existing behavior.
In `@isvtest/src/isvtest/validations/breakfix.py`:
- Around line 33-38: Add concise PEP 257-compliant docstrings to every function
and class in the file, including _record_label, _maybe_skip, and each run
method. Ensure each docstring briefly describes the symbol’s purpose, while
preserving all existing behavior.
- Around line 330-336: Update the health-agent validation around the agents list
and not_running check so an empty agents result is treated as failure when
agents_observable is true. Report that no health-agent records were found, while
preserving the existing failure for non-running records and success behavior
when at least one agent is running.
- Around line 285-288: Update the existing-workload validation in the
surrounding BFX01-04 flow to require an exact True value for
operation.get("existing_workloads_running"). Treat missing, false, or any
non-boolean value as failure via set_failed, and only call set_passed when the
value is exactly True.
In `@isvtest/tests/test_breakfix.py`:
- Around line 10-17: Update the validation imports and tests in test_breakfix.py
to include PlannedMaintenanceNotificationCheck. Change
test_planned_notification_passes to instantiate the planned-maintenance check,
and retain a separate test that instantiates FailureNotificationCheck so both
validations are covered.
---
Nitpick comments:
In `@isvctl/configs/providers/nico/config/bare_metal.yaml`:
- Around line 565-571: Update the settings and related maintenance steps in the
provider configuration so machine_id and rack_id are either passed through their
matching step args using the existing template values, or marked with a brief
comment as intentional placeholders if no current step supports them. Ensure no
declared setting remains unused without that clarification.
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 59-73: Add concise PEP 257-compliant docstrings to the emit,
machine_labels, and history_entries functions, documenting each function’s
purpose and return value while preserving their existing behavior and logic.
In `@isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py`:
- Line 23: Remove the generic "log" token from _LOG_KEYWORDS so
BmcKernelLogCheck only matches specific kernel-log indicators and entry_count
continues to represent log entries rather than broad probe matches.
In `@isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py`:
- Around line 15-27: Replace the duplicated module-guard logic with a shared
typed helper in isvctl/configs/providers/nico/scripts/breakfix/_common.py that
accepts the skip reason, gap ID, and operation payload, parses the existing
--org, --site-id, and --api-base arguments, emits the skip result, and returns
an exit code. In
isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27,
call it from main() -> int with gap BFX01-05 and operation {"requested": False,
"node_removed_from_pool": False}; apply the same main() structure in
reset_gpus.py#L15-L27 with BFX01-01 and {"requested": False, "completed":
False}, return_node_maintenance.py#L15-L27 with BFX01-02 and {"requested":
False, "accepted": False}, and return_rack_maintenance.py#L15-L27 with BFX01-03
and {"requested": False, "accepted": False}.
- Around line 15-27: Move the argument parsing and stub-result construction from
the __main__ guard into a typed main() -> int function, then invoke it with
sys.exit(main()). Within main(), replace the abbreviated p, a, and r variables
with descriptive names while preserving the existing required arguments,
skip_result call, operation payload, and emitted exit status.
In `@isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py`:
- Around line 15-27: Refactor the __main__ block into a typed main() function,
matching the structure used by query_bmc_kernel_logs.py. Replace abbreviated
variables such as p, a, and r with descriptive names, preserve the existing
argument parsing, skip_result call, operation payload, and emitted exit status,
and invoke main() from the __main__ guard.
In `@isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py`:
- Around line 15-27: Refactor the module-level CLI logic in the __main__ block
into a typed main() function, using descriptive names instead of p, a, and r
while preserving the existing arguments, skip_result call, operation payload,
and emitted exit status. Update the entry point to invoke main(), matching the
structure used by query_bmc_kernel_logs.py.
In `@isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py`:
- Around line 15-27: Refactor the __main__ block into a typed main() function,
moving argument parsing and result construction into it. Replace abbreviated
variables such as p, a, and r with descriptive names, matching the structure and
naming style used by query_bmc_kernel_logs.py, while preserving the existing
arguments, skip_result call, operation values, and exit behavior.
🪄 Autofix
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: 802eaa2e-d6ae-47d0-bea7-ce76f3e17235
📒 Files selected for processing (38)
docs/test-plan.yamlisvctl/configs/providers/my-isv/config/bare_metal.yamlisvctl/configs/providers/my-isv/config/k8s.yamlisvctl/configs/providers/my-isv/scripts/breakfix/_stub.pyisvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/config/key_access.yamlisvctl/configs/providers/nico/scripts/breakfix/_common.pyisvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/nico/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/nico/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/nico/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/nico/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/nico/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/suites/README.mdisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/k8s.yamlisvtest/src/isvtest/validations/breakfix.pyisvtest/tests/test_breakfix.py
💤 Files with no reviewable changes (1)
- isvctl/configs/providers/nico/config/key_access.yaml
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
isvctl/configs/providers/nico/scripts/breakfix/_common.py (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the stale exception-handling comment.
The
except (URLError, ValueError)block converts these failures into a structured result. The preceding comment says that the failures escape and produce no JSON. Replace it with wording that describes the current handled path.🤖 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/providers/nico/scripts/breakfix/_common.py` around lines 32 - 34, Update the stale comment immediately before the except (URLError, ValueError) block to state that these errors are caught and converted into a structured failure result, preserving the existing exception-handling behavior.
🤖 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.
Nitpick comments:
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 32-34: Update the stale comment immediately before the except
(URLError, ValueError) block to state that these errors are caught and converted
into a structured failure result, preserving the existing exception-handling
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12913a45-cf7d-4776-a6e1-2cdc24e60bb7
📒 Files selected for processing (23)
isvctl/configs/providers/my-isv/scripts/breakfix/_stub.pyisvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/providers/nico/scripts/breakfix/_common.pyisvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/nico/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.pyisvtest/src/isvtest/validations/breakfix.pyisvtest/tests/test_breakfix.py
🚧 Files skipped from review as they are similar to previous changes (18)
- isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py
- isvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.py
- isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py
- isvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.py
- isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.py
- isvctl/configs/providers/nico/scripts/breakfix/query_repair_history.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.py
- isvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.py
- isvctl/configs/providers/my-isv/scripts/breakfix/_stub.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.py
- isvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.py
- isvtest/tests/test_breakfix.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.py
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>
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>
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>
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>
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>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
isvctl/configs/providers/nico/scripts/breakfix/_common.py (1)
36-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlso catch
OSErrorso a socket timeout still produces JSON.
HTTPErrorandURLErrorare covered, becauseHTTPErrorsubclassesURLError. A read timeout can surface asTimeoutError(anOSError), which is not aURLErrorand not aValueError. In that case the script exits without printing, and the orchestrator receives no contract.URLErroris itself anOSError, so widening the tuple keeps the current behavior.🛡️ Proposed fix
- except (URLError, ValueError) as exc: + except (OSError, ValueError) as exc: result["error"] = f"{type(exc).__name__}: {exc}" return [], resultThis is the same failure mode as the earlier request-failure finding, applied to the remaining exception class.
🤖 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/providers/nico/scripts/breakfix/_common.py` around lines 36 - 45, Extend the exception tuple in the shared error-handling block after NicoAuthError to also catch OSError, preserving the existing structured result and JSON-return behavior for URLError and ValueError while covering socket timeouts such as TimeoutError.
🤖 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/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.py`:
- Around line 24-34: Update the planned-notification fixture returned by
emit_stub in query_planned_notifications so it no longer uses the past
2026-07-01 maintenance date. Replace the message with a date-independent
planned-maintenance description or use a clearly future scheduled date, while
preserving the existing payload structure.
In `@isvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.py`:
- Around line 24-41: Update the CPU fixture in query_serial_numbers to use a
serial-like identifier instead of the model name "Intel Demo CPU"; preserve the
existing component structure and replace only the CPU identifiers value with a
clearly synthetic CPU serial.
In `@isvctl/configs/providers/nico/scripts/auth/_key_access.py`:
- Around line 116-128: Update the key creation flow around forge_post("sshkey")
to require a non-empty response id before assigning created.sshkey_id or
creating the group. Raise an appropriate error when the id is missing, while
preserving the existing group creation path for valid ids.
---
Outside diff comments:
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 36-45: Extend the exception tuple in the shared error-handling
block after NicoAuthError to also catch OSError, preserving the existing
structured result and JSON-return behavior for URLError and ValueError while
covering socket timeouts such as TimeoutError.
🪄 Autofix
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: a6496430-f277-4205-b35b-c6f88bc7a5ef
📒 Files selected for processing (33)
isvctl/configs/providers/my-isv/config/k8s.yamlisvctl/configs/providers/my-isv/scripts/README.mdisvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/providers/my-isv/scripts/common/__init__.pyisvctl/configs/providers/my-isv/scripts/common/stub.pyisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/scripts/auth/_key_access.pyisvctl/configs/providers/nico/scripts/auth/query_key_access.pyisvctl/configs/providers/nico/scripts/auth/setup_key_access.pyisvctl/configs/providers/nico/scripts/auth/teardown_key_access.pyisvctl/configs/providers/nico/scripts/breakfix/_common.pyisvctl/configs/providers/nico/scripts/breakfix/gap_stub.pyisvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/nico/scripts/breakfix/query_repair_history.pyisvctl/configs/suites/README.mdisvctl/tests/providers/nico/test_nico_provider.pyisvctl/tests/test_provider_scaffold_cli.pyisvtest/src/isvtest/validations/breakfix.pyisvtest/tests/test_breakfix.py
💤 Files with no reviewable changes (2)
- isvctl/configs/providers/nico/scripts/auth/teardown_key_access.py
- isvctl/configs/providers/nico/scripts/auth/setup_key_access.py
🚧 Files skipped from review as they are similar to previous changes (12)
- isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py
- isvctl/configs/providers/my-isv/config/k8s.yaml
- isvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.py
- isvctl/configs/providers/nico/scripts/breakfix/query_repair_history.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.py
- isvctl/configs/suites/README.md
- isvtest/src/isvtest/validations/breakfix.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.py
- isvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.py
`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>
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>
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>
Summary
Adds the break-fix validation suite covering BFX01–BFX06, wired into the existing
bare_metalandk8ssuites rather than a standalone suite.isvtest/validations/breakfix.py— 12 new validation classes (BFX03-01 reuses the existingBmHardwareSerialCheck)suites/bare_metal.yaml— 11 checks;suites/k8s.yaml— GPU reset + cordonmy-isv— demo stubs for every step, exercised bymake demo-testnico— 4 read-only implementations; the rest emit a structured skip carrying agap: BFXxx-yymarker rather than a vacuous passValidated against live NICo
Run on two independent NICo deployments:
az51-dev3(6 CPU-only machines) andaz51-dev3-dh1(18 GPU hosts).Errorhistory on 18/18 machinesThe 8 gaps reproduced with identical reasons on both deployments, so they are platform gaps rather than one site's configuration. On NICo only BFX03-01 and BFX02-03 currently prove anything; the rest document missing API surface.
Every break-fix step reads only the
machineresource, so the suite runs with infrastructure-scope access and needs no tenant-level API permissions.Also in this PR
nico/config/key_access.yamlfolded intobare_metal.yaml— both imported the same suite, which made--provider nico --suite bare_metalunresolvable. The key-provisioning steps carryskip: trueso a routine run stays read-only.make test).InUseno longer counts as repair history.Testing
make test(124 passed) ·make demo-test·make lint·uvx pre-commit run -a·scripts/validate_suite_wiring.py --checkNew checks are unreleased; run with
ISVTEST_INCLUDE_UNRELEASED=1.Issues
Closes #213 — observed passing against live NICo, on 18/18 machines at
az51-dev3-dh1Closes #211 — implemented against the NICo API; skipped only because no test site had a machine in maintenance
The rest get the test and wiring, but NICo cannot satisfy them yet, so they stay open:
Part of #207
Part of #208
Part of #210
Part of #212
Part of #214
Part of #215
#206 and #209 also stay open — both are wired into
k8s.yamland have not been exercised end-to-end.#567 tracks each remaining NICo gap. #568 tracks the orphaned
reset_gpus.py.Summary by CodeRabbit
New Features
Documentation