fix(#985): add missing return after ResolveTCPAddr error

[fullsend-ai-coder]

Summary

Fix nil pointer panic in checkIfIpIsLive when DNS resolution fails for an IBM Z or Power host IP address. The missing return err after a failed net.ResolveTCPAddr call caused execution to continue and dereference the nil *net.TCPAddr pointer.

Changes

• pkg/ibm/ibmz_helpers.go: Add return err after the error log in the net.ResolveTCPAddr error branch, matching the error-handling pattern used elsewhere in the function (lines 89 and 93).
• pkg/ibm/ibmz_helpers_test.go: Add a Ginkgo test that calls checkIfIpIsLive with an unresolvable address and verifies it returns an error instead of panicking.

Testing

• make lint passes (0 issues)
• go test ./pkg/ibm/... passes (116 of 116 specs)
• Secret scan passes

Coverage

Patch adds one line of production code (the return err) and one test that exercises the new code path. Patch coverage is 100%.

Notes

One-line fix following the existing error-handling pattern in the same function.

---

Closes #985

Post-script verification

• Branch is not main/master (agent/985-fix-nil-panic-checkifipislive)
• Secret scan passed (gitleaks — 6683f88b167b28e8bda2ea80e4545031a1853307..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:16 AM UTC · Completed 10:26 AM UTC
Commit: 37b10e4 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [test-assertion-pattern] pkg/ibm/ibmz_helpers_test.go:23 — Error assertion uses ShouldNot(Succeed()) instead of the established codebase pattern Should(HaveOccurred()). The codebase has 29 uses of Should(HaveOccurred()) across 8 files for error assertions and zero uses of ShouldNot(Succeed()). While functionally equivalent, this is a genuine consistency issue.
  Remediation: Change Expect(checkIfIpIsLive(ctx, addr)).ShouldNot(Succeed()) to the two-line pattern:

  err := checkIfIpIsLive(ctx, addr)
  Expect(err).Should(HaveOccurred())

Previous run

Review

Findings

Low

• [commit-convention-violation] — PR title fix(#985): add missing return after ResolveTCPAddr error does not follow documented commit convention. Per AGENTS.md, commits must start with a Jira ID (e.g., KFLUXINFRA-1234 description). The title uses conventional-commit style instead. Consider updating the PR title to include the Jira ticket ID, or creating a Jira ticket if one does not exist for this bug.

Previous run (2)

Looks good to me

Previous run (3)

Review

Findings

Low

• [test-signature-convention] pkg/ibm/ibmz_helpers_test.go:23 — Test function signature uses func(ctx context.Context) instead of the codebase convention func(ctx SpecContext). All existing Ginkgo v2 unit tests in this repository (pkg/aws, pkg/metrics) consistently use SpecContext. The context.Context form appears only in e2e tests under test/e2e/, which is a different test layer with different conventions.
  Remediation: Change the test signature from func(ctx context.Context) to func(ctx SpecContext) and remove the "context" import.

• [commit-convention-partial] PR title uses issue reference format fix(#985): rather than Jira ID format (e.g., KFLUXINFRA-XXXX). AGENTS.md specifies "Commits: Jira ID at start". Verify whether GitHub issue references are acceptable for bug fixes or if Jira tracking is required.

---

Labels: Bug fix in IBM cloud provider package (pkg/ibm); patch-level severity change

Previous run (4)

Looks good to me

---

Labels: Bug fix in IBM cloud provider package — prevents nil pointer panic in checkIfIpIsLive

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.21%. Comparing base (5ad3bb0) to head (5b34b92).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
+ Coverage   76.92%   77.21%   +0.29%     
==========================================
  Files          26       26              
  Lines        2817     2818       +1     
==========================================
+ Hits         2167     2176       +9     
+ Misses        452      444       -8     
  Partials      198      198              

Flag	Coverage Δ
unit-tests	74.13% <100.00%> (+0.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pkg/ibm/ibmz_helpers.go	77.05% <100.00%> (+3.68%)	⬆️

... and 3 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ad3bb0...5b34b92. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[meyrevived]

/agentic-review

[meyrevived]

/agentic_review

[qodo-app-for-konflux-ci]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 2 rules

1. Unverified resolve-error path ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new test only asserts checkIfIpIsLive returns a non-nil error and does not confirm the error
came from the net.ResolveTCPAddr failure path, so the test can pass due to a later
net.DialTimeout error without actually validating the intended early-return behavior. Capture the
returned error and assert it is a resolver/address-parsing failure (e.g., by using a deterministic
invalid address string and errors.As to a *net.AddrError/*net.DNSError).

Code

pkg/ibm/ibmz_helpers_test.go[R24-25]

+				addr := "invalid-host-that-does-not-resolve.example.invalid"
+				Expect(checkIfIpIsLive(ctx, addr)).ShouldNot(Succeed())

Relevance

●●● Strong

Team often strengthens tests by asserting specific errors/messages, not just
HaveOccurred/Not(Succeed). See accepted reviews in #612/#872.

PR-#612
PR-#872
PR-#625

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test uses a generic non-success assertion and never inspects the returned error. In production
code, checkIfIpIsLive returns an error both when ResolveTCPAddr fails and when DialTimeout
fails, so the test as written cannot ensure it covered the specific resolve-error behavior the PR
intends to protect; other tests in the repo typically capture and assert error details rather than
just “not succeed.”

pkg/ibm/ibmz_helpers_test.go[21-26]
pkg/ibm/ibmz_helpers.go[78-91]
pkg/aws/ec2_helpers_test.go[24-38]
PR-#612

Agent prompt

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

## Issue description
The test currently uses `ShouldNot(Succeed())`, which accepts *any* error from `checkIfIpIsLive` and does not prove the code exercised the `net.ResolveTCPAddr` error branch this PR is meant to cover.

## Issue Context
`checkIfIpIsLive` can return an error either from `net.ResolveTCPAddr` or from `net.DialTimeout`. To make the test validate the intended behavior, it should capture the error and assert it matches a deterministic ResolveTCPAddr failure mode.

## Fix Focus Areas
- pkg/ibm/ibmz_helpers_test.go[24-25]

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

---

2. DNS-dependent unit test 🐞 Bug ☼ Reliability

Description

The new test uses a real hostname, which forces net.ResolveTCPAddr to consult the system DNS
resolver and makes the unit test timing/environment-dependent. In restricted or misconfigured CI
environments this can lead to slow or flaky test execution even though the production fix is
correct.

Code

pkg/ibm/ibmz_helpers_test.go[R25-26]

+				err := checkIfIpIsLive(context.TODO(), "invalid-host-that-does-not-resolve.example.invalid")
+				Expect(err).Should(HaveOccurred())

Relevance

●● Moderate

No direct DNS-flaky-test precedent; team often avoids real network in tests via injectable pingFunc
(#883/#897).

PR-#883
PR-#897

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test passes a resolvable-form hostname, which triggers DNS lookup inside net.ResolveTCPAddr;
the implementation does not use the provided context to bound resolution time, so runtime depends on
the environment’s resolver configuration.

pkg/ibm/ibmz_helpers_test.go[22-27]
pkg/ibm/ibmz_helpers.go[78-88]

Agent prompt

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

### Issue description
The added unit test calls `checkIfIpIsLive` with a syntactically valid hostname. That causes `net.ResolveTCPAddr` to perform real DNS resolution using the host’s resolver configuration, making the test slower and less deterministic in some CI environments.

### Issue Context
`checkIfIpIsLive` resolves `ip+":22"` via `net.ResolveTCPAddr` and does not apply any context-based deadline/cancellation to name resolution.

### Fix Focus Areas
- pkg/ibm/ibmz_helpers_test.go[22-29]

### Suggested fix
Change the test input to a *syntactically invalid* host string that fails locally during parsing (no DNS), e.g. include a space or other illegal character:
- Example: `"invalid host"` (so the function resolves `"invalid host:22"` and immediately errors)

This still exercises the `ResolveTCPAddr` error branch (and proves there is no panic) without relying on external/system DNS behavior.

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

---

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

Previous review results

Review updated until commit 5b34b92 ⚖️ Balanced

Results up to commit d51c0ec ⚖️ Balanced

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

1. DNS-dependent unit test 🐞 Bug ☼ Reliability

Description

The new test uses a real hostname, which forces net.ResolveTCPAddr to consult the system DNS
resolver and makes the unit test timing/environment-dependent. In restricted or misconfigured CI
environments this can lead to slow or flaky test execution even though the production fix is
correct.

Code

pkg/ibm/ibmz_helpers_test.go[R25-26]

+				err := checkIfIpIsLive(context.TODO(), "invalid-host-that-does-not-resolve.example.invalid")
+				Expect(err).Should(HaveOccurred())

Relevance

●● Moderate

No direct DNS-flaky-test precedent; team often avoids real network in tests via injectable pingFunc
(#883/#897).

PR-#883
PR-#897

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test passes a resolvable-form hostname, which triggers DNS lookup inside net.ResolveTCPAddr;
the implementation does not use the provided context to bound resolution time, so runtime depends on
the environment’s resolver configuration.

pkg/ibm/ibmz_helpers_test.go[22-27]
pkg/ibm/ibmz_helpers.go[78-88]

Agent prompt

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

### Issue description
The added unit test calls `checkIfIpIsLive` with a syntactically valid hostname. That causes `net.ResolveTCPAddr` to perform real DNS resolution using the host’s resolver configuration, making the test slower and less deterministic in some CI environments.

### Issue Context
`checkIfIpIsLive` resolves `ip+":22"` via `net.ResolveTCPAddr` and does not apply any context-based deadline/cancellation to name resolution.

### Fix Focus Areas
- pkg/ibm/ibmz_helpers_test.go[22-29]

### Suggested fix
Change the test input to a *syntactically invalid* host string that fails locally during parsing (no DNS), e.g. include a space or other illegal character:
- Example: `"invalid host"` (so the function resolves `"invalid host:22"` and immediately errors)

This still exercises the `ResolveTCPAddr` error branch (and proves there is no panic) without relying on external/system DNS behavior.

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

---

Results up to commit b398b21 ⚖️ Balanced

No changes from previous review

Results up to commit 8b849c6 ⚖️ Balanced

No changes from previous review

Results up to commit 08207a9 ⚖️ Balanced

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

1. Unverified resolve-error path ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new test only asserts checkIfIpIsLive returns a non-nil error and does not confirm the error
came from the net.ResolveTCPAddr failure path, so the test can pass due to a later
net.DialTimeout error without actually validating the intended early-return behavior. Capture the
returned error and assert it is a resolver/address-parsing failure (e.g., by using a deterministic
invalid address string and errors.As to a *net.AddrError/*net.DNSError).

Code

pkg/ibm/ibmz_helpers_test.go[R24-25]

+				addr := "invalid-host-that-does-not-resolve.example.invalid"
+				Expect(checkIfIpIsLive(ctx, addr)).ShouldNot(Succeed())

Relevance

●●● Strong

Team often strengthens tests by asserting specific errors/messages, not just
HaveOccurred/Not(Succeed). See accepted reviews in #612/#872.

PR-#612
PR-#872
PR-#625

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test uses a generic non-success assertion and never inspects the returned error. In production
code, checkIfIpIsLive returns an error both when ResolveTCPAddr fails and when DialTimeout
fails, so the test as written cannot ensure it covered the specific resolve-error behavior the PR
intends to protect; other tests in the repo typically capture and assert error details rather than
just “not succeed.”

pkg/ibm/ibmz_helpers_test.go[21-26]
pkg/ibm/ibmz_helpers.go[78-91]
pkg/aws/ec2_helpers_test.go[24-38]
PR-#612

Agent prompt

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

## Issue description
The test currently uses `ShouldNot(Succeed())`, which accepts *any* error from `checkIfIpIsLive` and does not prove the code exercised the `net.ResolveTCPAddr` error branch this PR is meant to cover.

## Issue Context
`checkIfIpIsLive` can return an error either from `net.ResolveTCPAddr` or from `net.DialTimeout`. To make the test validate the intended behavior, it should capture the error and assert it matches a deterministic ResolveTCPAddr failure mode.

## Fix Focus Areas
- pkg/ibm/ibmz_helpers_test.go[24-25]

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

---

Results up to commit 802fefa ⚖️ Balanced

No changes from previous review

[qodo-app-for-konflux-ci]

PR Summary by Qodo

Fix nil panic in checkIfIpIsLive on ResolveTCPAddr failure

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Return early when DNS/TCP address resolution fails to avoid nil dereference panic.
• Add a unit test ensuring unresolvable hosts return a *net.DNSError (no panic).

Diagram

graph TD
A["checkIfIpIsLive"] --> B["ResolveTCPAddr"] --> C{error?}
C -- "yes" --> D["return err"]
C -- "no" --> E["DialTimeout :22"] --> F["return nil / dial err"]
G["Test: unresolvable host"] --> A

Loading

High-Level Assessment

The chosen fix (return immediately on ResolveTCPAddr error) is the most direct and consistent with existing error-handling in the function. Alternatives like adding additional nil checks or restructuring the flow would be more verbose and less idiomatic than returning the encountered error at the source.

Files changed (2) +15 / -0

Bug fix (1) +1 / -0

ibmz_helpers.goReturn on ResolveTCPAddr failure in checkIfIpIsLive +1/-0

Return on ResolveTCPAddr failure in checkIfIpIsLive

• Adds the missing 'return err' in the ResolveTCPAddr error branch so the function stops after logging. This prevents dereferencing a nil '*net.TCPAddr' when resolution fails.

pkg/ibm/ibmz_helpers.go

Tests (1) +14 / -0

ibmz_helpers_test.goTest DNS resolution failure path for checkIfIpIsLive +14/-0

Test DNS resolution failure path for checkIfIpIsLive

• Introduces a Ginkgo spec that calls 'checkIfIpIsLive' with an unresolvable hostname and asserts an error is returned. The test uses 'errors.As' to verify the error is a '*net.DNSError', proving it exercises the ResolveTCPAddr failure branch rather than a later dial failure.

pkg/ibm/ibmz_helpers_test.go

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 9:17 AM UTC · Ended 9:26 AM UTC
Commit: 37b10e4 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit b398b21

[meyrevived]

/lgtm

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:17 AM UTC · Completed 9:26 AM UTC
Commit: 37b10e4 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 12:49 PM UTC · Ended 12:56 PM UTC
Commit: 37b10e4 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 8b849c6

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:49 PM UTC · Completed 12:56 PM UTC
Commit: 37b10e4 · View workflow run →

[filariow]

/fs-fix

in tests we prefer to declare variables outside the function call and we prefer to use Expect(function(var1, var2)).ShouldNot(Succeed())

[filariow]

/fs-fix in tests we prefer to declare variables outside the function call and we prefer to use Expect(function(var1, var2)).ShouldNot(Succeed())

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 3:29 PM UTC · Completed 3:45 PM UTC
Commit: 37b10e4 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed review feedback: refactored checkIfIpIsLive test to declare variables outside the function call and use the Expect(func(vars)).ShouldNot(Succeed()) assertion pattern. All 116 tests pass.

Fixed (1):

1. declare variables outside the function call and use Expect(func(vars)).ShouldNot(Succeed()) pattern (pkg/ibm/ibmz_helpers_test.go): Extracted address string to a separate variable and changed assertion from Expect(err).Should(HaveOccurred()) to Expect(checkIfIpIsLive(ctx, addr)).ShouldNot(Succeed()) to match the team's preferred test assertion style

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 3:46 PM UTC · Ended 3:55 PM UTC
Commit: 37b10e4 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 08207a9

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:46 PM UTC · Completed 3:55 PM UTC
Commit: 37b10e4 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 6:09 AM UTC · Ended 6:19 AM UTC
Commit: 37b10e4 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 802fefa

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:09 AM UTC · Completed 6:19 AM UTC
Commit: 37b10e4 · View workflow run →

[meyrevived]

/retest

[meyrevived]

/fs-fix
The test in pkg/ibm/ibmz_helpers_test.go:24-25 currently uses ShouldNot(Succeed()), which accepts any error from checkIfIpIsLive and does not prove the code exercised the net.ResolveTCPAddr error branch this PR is meant to cover.
checkIfIpIsLive can return an error either from net.ResolveTCPAddr or from net.DialTimeout. To make the test validate the intended behavior, it should capture the error and assert it matches a deterministic ResolveTCPAddr failure mode.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 9:36 AM UTC · Completed 9:49 AM UTC
Commit: 37b10e4 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Addressed 1 review finding: strengthened the checkIfIpIsLive test assertion to verify the error is a *net.DNSError, proving the net.ResolveTCPAddr error branch is exercised rather than accepting any error.

Fixed (1):

1. Test uses ShouldNot(Succeed()) which accepts any error and does not prove the ResolveTCPAddr branch was exercised (pkg/ibm/ibmz_helpers_test.go): Replaced ShouldNot(Succeed()) with error capture and errors.As(*net.DNSError) type assertion. This proves the error came from net.ResolveTCPAddr (DNS resolution failure) rather than net.DialTimeout. Added 'errors' and 'net' imports to support the assertion.

Tests: passed

Decision points

• Chose errors.As with *net.DNSError type assertion over string matching on error message (alternatives: String matching on error message (e.g. ContainSubstring("no such host")), Custom matcher for net.OpError; rationale: errors.As is the idiomatic Go approach for error type checking, and *net.DNSError is the precise type returned by net.ResolveTCPAddr failures. String matching would be fragile across Go versions and platforms.)

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 9:50 AM UTC · Ended 9:52 AM UTC
Commit: 37b10e4 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 5b34b92

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:50 AM UTC · Completed 9:52 AM UTC
Commit: 37b10e4 · View workflow run →