feat(MTV-6351): test Critical DI concerns block warm migration

[MiriSafra]

Summary

• Verify that Deep Inspection Critical concerns actually prevent warm migration from proceeding — migration must fail at the PreflightInspection step with a "critical concerns" error, not silently continue
• Uses VM with /dev/disk/by-path/ entries in fstab (fstab-by-path-device concern) on vmware-7
• Add verify_di_concerns_block_migration() utility to validate the pipeline error after migration failure
• Add test_warm_di_concerns config entry

Jira: MTV-6351

Summary by CodeRabbit

• Bug Fixes

  • Added coverage confirming that critical Deep Inspection concerns prevent warm VM migrations.
  • Improved validation that affected VMs show the expected preflight inspection errors and blocking reasons.

• Tests

  • Added an end-to-end test scenario covering storage and network mapping, migration plan creation, failed execution, and verification of blocked VMs.

[coderabbitai]

Warning

Review limit reached

@MiriSafra, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 79697b42-93bd-4bcd-ac2c-af7949e813eb

📥 Commits

Reviewing files that changed from the base of the PR and between 2a27595 and 2f48c8d.

📒 Files selected for processing (3)

• tests/deep_inspection/test_plan_driven_di.py
• tests/tests_config/config.py
• utilities/deep_inspection.py

Walkthrough

The change adds a validator for Critical Deep Inspection migration blocks and a fixture-backed warm migration test. The test configuration enables one powered-on source VM, creates migration resources, expects plan execution to fail, and verifies the blocked VMs.

Changes

Warm migration DI blocking

Layer / File(s)	Summary
Critical DI blocking validation utilities/deep_inspection.py	Adds verify_di_concerns_block_migration, which validates VM status, the PreflightInspection error, and a reason containing “critical concerns.”
Warm migration test scenario tests/tests_config/config.py, tests/deep_inspection/test_plan_driven_di.py	Adds the test_warm_di_concerns configuration and a parametrized test that creates maps and a migration plan, expects MigrationPlanExecError, and verifies the blocked VMs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• RedHatQE/mtv-api-tests#575: Extends the same Deep Inspection utility and adds related inspection automation.
• RedHatQE/mtv-api-tests#590: Covers warm migration Deep Inspection configuration and behavior.
• RedHatQE/mtv-api-tests#608: Extends the Deep Inspection migration capture and validation workflow.

Suggested labels: size/XL

Suggested reviewers: krcmarik, solenoci

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: testing that Critical Deep Inspection concerns block warm migration.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[redhat-qe-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (PR author or approvers)
• /hold cancel - Unblock PR merging (PR author or approvers)
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message
• /security-override - Set security check runs to pass (maintainers only)
• /security-override cancel - Re-run security checks

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest build-container - Rebuild and test container image
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Container Operations

• /build-and-push-container - Build and push container image (tagged with PR number)
  • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3
• /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

• /rebase - Rebase this PR branch onto its base branch

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. Status Checks: All required status checks must pass
3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• krcmarik
• myakove
• solenoci

Reviewers:

• krcmarik
• myakove
• solenoci

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)
• Test Oracle: Triggers: approved (cursor/gpt-5.4-xhigh-fast); /test-oracle can be used anytime

Security Checks

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Container Builds: Container images are automatically tagged with the PR number
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[qodo-code-review]

PR Summary by Qodo

Test warm migration is blocked by Critical Deep Inspection concerns

🧪 Tests ✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Add a warm-migration e2e test ensuring Critical DI concerns stop migration at PreflightInspection
• Assert migration fails (not silently continues) and error reasons mention "critical concerns"
• Introduce a reusable DI verification helper and a dedicated test config VM

Diagram

graph TD
  E["tests_config: test_warm_di_concerns"] --> A["Pytest: warm DI gate test"] --> B["execute_migration()"] --> C[("Plan CR status")] --> F["PreflightInspection error: critical concerns"]
  A --> D["verify_di_concerns_block_migration()"] --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Assert via Plan conditions/events instead of pipeline steps

• ➕ Less coupled to internal per-VM pipeline structure (step names/fields)
• ➕ Can be more stable if pipeline representation changes
• ➖ May be less precise about where the block occurred (PreflightInspection)
• ➖ Events/conditions can be noisier and harder to map per-VM

2. Validate DI output payload (concerns list) in addition to gating

• ➕ Stronger coverage: confirms the specific Critical concern exists, not just the gating message
• ➕ Helps diagnose regressions in DI detection vs gating logic
• ➖ More brittle if DI output schema/messages change
• ➖ Adds test runtime and more moving parts (DI result collection APIs)

3. Fold into existing DI verification helpers (extend verify_captured_di_results)

• ➕ Avoids creating another helper with overlapping responsibilities
• ➕ Encourages a single canonical DI assertion API
• ➖ May over-generalize an assertion that is intentionally specific to migration blocking semantics
• ➖ Could complicate the existing helper’s API for callers that don’t need gating checks

Recommendation: Current approach is good for the stated goal because it verifies the user-visible contract: migration must fail at PreflightInspection with an explicit "critical concerns" reason. If this area becomes brittle due to pipeline schema changes, consider switching to Plan conditions/events as the primary assertion and keep the pipeline check as a secondary, best-effort diagnostic.

Files changed (3) +211 / -0

Enhancement (1) +50 / -0

deep_inspection.pyAdd verifier for DI concerns blocking migration at PreflightInspection +50/-0

Add verifier for DI concerns blocking migration at PreflightInspection

• Adds verify_di_concerns_block_migration(plan_resource, expected_vm_names) to inspect Plan migration status per VM. The helper asserts a PreflightInspection pipeline step exists and that its error reasons mention "critical concerns" for all expected VMs.

utilities/deep_inspection.py

Tests (1) +157 / -0

test_plan_driven_di.pyNew warm-migration test that must fail on Critical DI concerns +157/-0

New warm-migration test that must fail on Critical DI concerns

• Introduces a vsphere tier1 warm migration test using a VM that triggers a Critical DI concern (fstab /dev/disk/by-path). The test creates StorageMap/NetworkMap/Plan, runs migration expecting MigrationPlanExecError, then asserts the failure is attributable to PreflightInspection "critical concerns" gating via a shared verifier.

tests/deep_inspection/test_plan_driven_di.py

Other (1) +4 / -0

config.pyAdd test_warm_di_concerns plan config for DI gating scenario +4/-0

Add test_warm_di_concerns plan config for DI gating scenario

• Adds a dedicated tests_config entry pointing to a VM that triggers the DI concern and enables warm migration. This config is used by the new plan-driven DI test.

tests/tests_config/config.py

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/tests_config/config.py`:
- Around line 821-824: Update the "test_warm_di_concerns" configuration to
explicitly set run_preflight_inspection to True alongside warm_migration,
ensuring create_plan_resource() always enables the preflight inspection
condition this test verifies.

🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: f2d9ab19-9cfb-44a1-93cc-dd0d556e704c

📥 Commits

Reviewing files that changed from the base of the PR and between a170cd2 and 2a27595.

📒 Files selected for processing (3)

• tests/deep_inspection/test_plan_driven_di.py
• tests/tests_config/config.py
• utilities/deep_inspection.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. None pipeline TypeError ✓ Resolved 🐞 Bug ☼ Reliability

Description

verify_di_concerns_block_migration() iterates vm_status.pipeline after getattr(..., []), but
if the pipeline attribute exists and is None, the loop raises TypeError and the test errors
instead of producing a clear VM-specific validation failure.

Code

utilities/deep_inspection.py[R754-757]

+        pipeline = getattr(vm_status, "pipeline", [])
+        di_step = None
+        for step in pipeline:
+            if getattr(step, "name", "") == "PreflightInspection":

Relevance

●●● Strong

Team frequently accepts defensive None/shape guards to prevent TypeError and improve error messages
in utilities.

PR-#305
PR-#608

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper currently iterates the pipeline value without guarding against None. Another utility
in the codebase explicitly treats missing/falsey pipeline as a valid runtime state and raises a
controlled error, indicating pipeline can be absent/empty and should not be assumed iterable.

utilities/deep_inspection.py[754-761]
utilities/mtv_migration.py[72-85]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_di_concerns_block_migration()` uses `pipeline = getattr(vm_status, "pipeline", [])` and then iterates it. If the attribute exists but is `None`, iteration raises `TypeError: 'NoneType' object is not iterable`, which hides the intended validation error message.

### Issue Context
Elsewhere in the repo, pipeline is treated as potentially missing/falsey and handled explicitly.

### Fix Focus Areas
- utilities/deep_inspection.py[754-761]

### Suggested fix
- Normalize pipeline to an iterable:
 - `pipeline = getattr(vm_status, "pipeline", None) or []`
- Optionally add an explicit guard for non-list values and raise a clear `ValueError` (e.g., `pipeline is None` or not iterable) so failures remain actionable and consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Class attrs read via self ✗ Dismissed 📘 Rule violation ▣ Testability

Description

The test class stores shared state on class attributes (e.g., self.__class__.storage_map) but
later reads/passes them via instance access (self.storage_map, self.plan_resource). This
violates the shared-state convention and can lead to inconsistent behavior if instance attributes
are ever introduced/mutated.

Code

tests/deep_inspection/test_plan_driven_di.py[79]

+        assert self.storage_map, "StorageMap creation failed"

Relevance

●●● Strong

Repo enforces class-shared state conventions in tests; switching to self.__class__ access is trivial
and low-risk.

PR-#274
PR-#619

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 70454 requires shared state in test classes to be accessed via
self.__class__.<attr> (or ClassName.<attr>). In the new test, shared attributes are set on the
class but then read/passed via self.<attr>, e.g., assert self.storage_map and
plan=self.plan_resource.

Rule 70454: Use class attributes for shared state in test classes
tests/deep_inspection/test_plan_driven_di.py[70-79]
tests/deep_inspection/test_plan_driven_di.py[118-129]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shared test state is written via `self.__class__.<attr>` but later accessed via `self.<attr>`.

## Issue Context
Compliance rule requires shared state in test classes to be accessed and mutated consistently via the class (`self.__class__.<attr>` or `ClassName.<attr>`), not instance attributes.

## Fix Focus Areas
- tests/deep_inspection/test_plan_driven_di.py[70-79]
- tests/deep_inspection/test_plan_driven_di.py[94-105]
- tests/deep_inspection/test_plan_driven_di.py[118-129]
- tests/deep_inspection/test_plan_driven_di.py[139-156]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Unqualified failure assertion ✓ Resolved 🐞 Bug ≡ Correctness

Description

test_migrate_vms_fails() asserts only that MigrationPlanExecError is raised, so any unrelated
migration failure would satisfy the test and could mask regressions in DI blocking at
PreflightInspection. This repo already uses stage-specific matching for negative migrations, so
this new test should also assert the expected failure signal.

Code

tests/deep_inspection/test_plan_driven_di.py[R139-142]

+        with pytest.raises(MigrationPlanExecError):
+            execute_migration(
+                ocp_admin_client=ocp_admin_client,
+                fixture_store=fixture_store,

Relevance

●●● Strong

Repo precedent: tighten negative migration tests by matching expected failing stage, not just
MigrationPlanExecError.

PR-#557

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test currently only asserts an exception type, while an existing negative migration test
asserts the expected failure step name via match, demonstrating the suite’s established pattern
for tightening negative tests.

tests/deep_inspection/test_plan_driven_di.py[131-147]
tests/luks/test_luks_cold_migration.py[293-300]
PR-#557

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`tests/deep_inspection/test_plan_driven_di.py::test_migrate_vms_fails` only checks that `MigrationPlanExecError` is raised. This can allow unrelated failures to pass the test, reducing the test's ability to verify that *Critical DI concerns* specifically block the migration at `PreflightInspection`.

### Issue Context
Other negative tests in the suite assert the expected pipeline stage via `pytest.raises(..., match=...)`.

### Fix Focus Areas
- tests/deep_inspection/test_plan_driven_di.py[138-146]

### Suggested change
- Update the `pytest.raises` to match an expected signature (e.g., `match="PreflightInspection|critical concerns"`), or capture the exception and assert expected content from `plan.instance.status` / pipeline.
- Optionally, merge the verification into the same test (so it only runs after the expected exception is observed), improving diagnostics when the migration unexpectedly succeeds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Brittle pipeline error parsing 🐞 Bug ☼ Reliability

Description

verify_di_concerns_block_migration() dereferences plan_resource.instance.status.migration.vms
without guarding for missing intermediate fields, which can raise AttributeError and hide the real
migration state. It also only checks PreflightInspection.error.reasons for the text, ignoring
other possible error representations (e.g., message/string form), making the helper prone to false
negatives when the error text is not stored under reasons.

Code

utilities/deep_inspection.py[R744-748]

+    vms_status = plan_resource.instance.status.migration.vms
+    for vm_name in expected_vm_names:
+        vm_status = None
+        for vs in vms_status:
+            if getattr(vs, "name", "") == vm_name or getattr(vs, "id", "") == vm_name:

Relevance

● Weak

Very similar defensive-attribute-guard suggestion on deep_inspection status path was explicitly
rejected previously.

PR-#608

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper directly dereferences a deep status path without null checks, and it hard-codes
error.reasons parsing. In contrast, the repo’s existing pipeline parsing logic only checks for the
presence of step.error and logs it as an opaque value, indicating there is no established in-repo
contract that error.reasons exists.

utilities/deep_inspection.py[727-772]
utilities/mtv_migration.py[72-90]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_di_concerns_block_migration()` assumes a fully populated status path (`status.migration.vms`) and a specific error shape (`error.reasons`). If any intermediate status is missing, the helper can crash with `AttributeError`. If the error text is present in another field (or only in `str(error)`), the helper will incorrectly fail even though the pipeline has the expected failure.

### Issue Context
Elsewhere in the repo (migration pipeline parsing) the code treats `step_error` as opaque and logs it, rather than assuming subfields.

### Fix Focus Areas
- utilities/deep_inspection.py[744-772]
- utilities/mtv_migration.py[72-90]

### Suggested change
1. Add defensive validation:
  - `status = getattr(plan_resource.instance, "status", None)`
  - `migration = getattr(status, "migration", None)`
  - `vms_status = getattr(migration, "vms", None)`
  - Raise a clear `ValueError` if any is missing (include `plan_resource.name` and `plan_resource.instance` snippet).
2. Make the error-text check robust:
  - Collect candidate text from `getattr(step_error, "reasons", None)`, `getattr(step_error, "message", "")`, and `str(step_error)`.
  - Search for the substring in the combined text.
3. (Optional) Refresh the Plan before reading status if the resource wrapper supports it, to reduce stale-status flakes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 100 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 2f48c8d ⚖️ Balanced

Results up to commit 2a27595 ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Unqualified failure assertion ✓ Resolved 🐞 Bug ≡ Correctness

Description

test_migrate_vms_fails() asserts only that MigrationPlanExecError is raised, so any unrelated
migration failure would satisfy the test and could mask regressions in DI blocking at
PreflightInspection. This repo already uses stage-specific matching for negative migrations, so
this new test should also assert the expected failure signal.

Code

tests/deep_inspection/test_plan_driven_di.py[R139-142]

+        with pytest.raises(MigrationPlanExecError):
+            execute_migration(
+                ocp_admin_client=ocp_admin_client,
+                fixture_store=fixture_store,

Relevance

●●● Strong

Repo precedent: tighten negative migration tests by matching expected failing stage, not just
MigrationPlanExecError.

PR-#557

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test currently only asserts an exception type, while an existing negative migration test
asserts the expected failure step name via match, demonstrating the suite’s established pattern
for tightening negative tests.

tests/deep_inspection/test_plan_driven_di.py[131-147]
tests/luks/test_luks_cold_migration.py[293-300]
PR-#557

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`tests/deep_inspection/test_plan_driven_di.py::test_migrate_vms_fails` only checks that `MigrationPlanExecError` is raised. This can allow unrelated failures to pass the test, reducing the test's ability to verify that *Critical DI concerns* specifically block the migration at `PreflightInspection`.

### Issue Context
Other negative tests in the suite assert the expected pipeline stage via `pytest.raises(..., match=...)`.

### Fix Focus Areas
- tests/deep_inspection/test_plan_driven_di.py[138-146]

### Suggested change
- Update the `pytest.raises` to match an expected signature (e.g., `match="PreflightInspection|critical concerns"`), or capture the exception and assert expected content from `plan.instance.status` / pipeline.
- Optionally, merge the verification into the same test (so it only runs after the expected exception is observed), improving diagnostics when the migration unexpectedly succeeds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Brittle pipeline error parsing 🐞 Bug ☼ Reliability

Description

verify_di_concerns_block_migration() dereferences plan_resource.instance.status.migration.vms
without guarding for missing intermediate fields, which can raise AttributeError and hide the real
migration state. It also only checks PreflightInspection.error.reasons for the text, ignoring
other possible error representations (e.g., message/string form), making the helper prone to false
negatives when the error text is not stored under reasons.

Code

utilities/deep_inspection.py[R744-748]

+    vms_status = plan_resource.instance.status.migration.vms
+    for vm_name in expected_vm_names:
+        vm_status = None
+        for vs in vms_status:
+            if getattr(vs, "name", "") == vm_name or getattr(vs, "id", "") == vm_name:

Relevance

● Weak

Very similar defensive-attribute-guard suggestion on deep_inspection status path was explicitly
rejected previously.

PR-#608

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper directly dereferences a deep status path without null checks, and it hard-codes
error.reasons parsing. In contrast, the repo’s existing pipeline parsing logic only checks for the
presence of step.error and logs it as an opaque value, indicating there is no established in-repo
contract that error.reasons exists.

utilities/deep_inspection.py[727-772]
utilities/mtv_migration.py[72-90]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_di_concerns_block_migration()` assumes a fully populated status path (`status.migration.vms`) and a specific error shape (`error.reasons`). If any intermediate status is missing, the helper can crash with `AttributeError`. If the error text is present in another field (or only in `str(error)`), the helper will incorrectly fail even though the pipeline has the expected failure.

### Issue Context
Elsewhere in the repo (migration pipeline parsing) the code treats `step_error` as opaque and logs it, rather than assuming subfields.

### Fix Focus Areas
- utilities/deep_inspection.py[744-772]
- utilities/mtv_migration.py[72-90]

### Suggested change
1. Add defensive validation:
  - `status = getattr(plan_resource.instance, "status", None)`
  - `migration = getattr(status, "migration", None)`
  - `vms_status = getattr(migration, "vms", None)`
  - Raise a clear `ValueError` if any is missing (include `plan_resource.name` and `plan_resource.instance` snippet).
2. Make the error-text check robust:
  - Collect candidate text from `getattr(step_error, "reasons", None)`, `getattr(step_error, "message", "")`, and `str(step_error)`.
  - Search for the substring in the combined text.
3. (Optional) Refresh the Plan before reading status if the resource wrapper supports it, to reduce stale-status flakes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 523ffae ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. None pipeline TypeError ✓ Resolved 🐞 Bug ☼ Reliability

Description

verify_di_concerns_block_migration() iterates vm_status.pipeline after getattr(..., []), but
if the pipeline attribute exists and is None, the loop raises TypeError and the test errors
instead of producing a clear VM-specific validation failure.

Code

utilities/deep_inspection.py[R754-757]

+        pipeline = getattr(vm_status, "pipeline", [])
+        di_step = None
+        for step in pipeline:
+            if getattr(step, "name", "") == "PreflightInspection":

Relevance

●●● Strong

Team frequently accepts defensive None/shape guards to prevent TypeError and improve error messages
in utilities.

PR-#305
PR-#608

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper currently iterates the pipeline value without guarding against None. Another utility
in the codebase explicitly treats missing/falsey pipeline as a valid runtime state and raises a
controlled error, indicating pipeline can be absent/empty and should not be assumed iterable.

utilities/deep_inspection.py[754-761]
utilities/mtv_migration.py[72-85]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_di_concerns_block_migration()` uses `pipeline = getattr(vm_status, "pipeline", [])` and then iterates it. If the attribute exists but is `None`, iteration raises `TypeError: 'NoneType' object is not iterable`, which hides the intended validation error message.

### Issue Context
Elsewhere in the repo, pipeline is treated as potentially missing/falsey and handled explicitly.

### Fix Focus Areas
- utilities/deep_inspection.py[754-761]

### Suggested fix
- Normalize pipeline to an iterable:
 - `pipeline = getattr(vm_status, "pipeline", None) or []`
- Optionally add an explicit guard for non-list values and raise a clear `ValueError` (e.g., `pipeline is None` or not iterable) so failures remain actionable and consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Class attrs read via self ✗ Dismissed 📘 Rule violation ▣ Testability

Description

The test class stores shared state on class attributes (e.g., self.__class__.storage_map) but
later reads/passes them via instance access (self.storage_map, self.plan_resource). This
violates the shared-state convention and can lead to inconsistent behavior if instance attributes
are ever introduced/mutated.

Code

tests/deep_inspection/test_plan_driven_di.py[79]

+        assert self.storage_map, "StorageMap creation failed"

Relevance

●●● Strong

Repo enforces class-shared state conventions in tests; switching to self.__class__ access is trivial
and low-risk.

PR-#274
PR-#619

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 70454 requires shared state in test classes to be accessed via
self.__class__.<attr> (or ClassName.<attr>). In the new test, shared attributes are set on the
class but then read/passed via self.<attr>, e.g., assert self.storage_map and
plan=self.plan_resource.

Rule 70454: Use class attributes for shared state in test classes
tests/deep_inspection/test_plan_driven_di.py[70-79]
tests/deep_inspection/test_plan_driven_di.py[118-129]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shared test state is written via `self.__class__.<attr>` but later accessed via `self.<attr>`.

## Issue Context
Compliance rule requires shared state in test classes to be accessed and mutated consistently via the class (`self.__class__.<attr>` or `ClassName.<attr>`), not instance attributes.

## Fix Focus Areas
- tests/deep_inspection/test_plan_driven_di.py[70-79]
- tests/deep_inspection/test_plan_driven_di.py[94-105]
- tests/deep_inspection/test_plan_driven_di.py[118-129]
- tests/deep_inspection/test_plan_driven_di.py[139-156]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 523ffae

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2f48c8d

[MiriSafra]

/verified