PM-28053: Ensure any exception thrown during re-auth is an IO exception

[david-livefront]

🎟️ Tracking

PM-28053

📔 Objective

This PR updates the AuthTokenManager to ensure that all exceptions throw are handled by Retrofit.

The main issue here was that HttpExceptions could be throw from network errors and Retrofit can only handle IOExceptions.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @david-livefront's task —— View job

---

Code Review Complete

Overall Assessment: REQUEST CHANGES

Critical Issues:

• Finding 1: Incomplete test coverage for toIoException() extension function (60% coverage, 2 lines missing)
• Finding 2: Missing KDoc documentation for the new extension function

See inline comments for detailed feedback and code suggestions.

---

Review Summary

This PR successfully addresses the core issue of ensuring Retrofit can handle all exceptions by converting them to IOException. The implementation is clean and follows established project patterns. However, there are two issues that need to be addressed before approval:

1. Test coverage is incomplete - Codecov reports 60% patch coverage with 2 lines missing in the new toIoException() function
2. Documentation is missing - The new extension function lacks KDoc documentation required by project guidelines

Findings:

Finding 1: ❌ Incomplete test coverage for toIoException() extension function

The new extension function at lines 128-132 of AuthTokenManager.kt needs direct unit tests. While you added a good integration test showing HttpException wrapping (line 225-245 in the test file), the function itself needs coverage for both branches:

• Line 130: IOException passthrough case
• Line 131: Non-IOException wrapping case

See inline comment on AuthTokenManager.kt:128-132 for suggested test implementation.

Finding 2: ⚠️ Missing KDoc documentation

Per STYLE_AND_BEST_PRACTICES.md: "All public classes, functions, and properties should include documentation in the KDoc style." While this function is file-private, it's a reusable utility that should explain:

• Why the conversion is necessary (Retrofit compatibility requirement)
• The behavior for each case (passthrough vs wrapping)

See inline comment on AuthTokenManager.kt:128-132 for suggested documentation.

Finding 3: 💭 Test organization suggestion

Consider adding a dedicated nested test class (e.g., ToIoExceptionExtension) for direct unit tests of the extension function, while keeping the existing integration test. This provides both unit-level confidence and integration validation.

---

Positive Observations

• ✅ Clean implementation following established error handling patterns
• ✅ Proper use of @Throws(IOException::class) annotations
• ✅ Good integration test demonstrating HttpException wrapping behavior
• ✅ Follows Kotlin idioms with expression function and when expression
• ✅ Smart use of getOrElse instead of getOrThrow() for proper exception conversion
• ✅ No security concerns introduced (Checkmarx passed)

---

Action Items

Required:

1. Add direct unit tests for toIoException() to achieve 100% coverage
2. Add KDoc documentation to the extension function

Optional:
3. Organize tests in a dedicated nested class for better structure

[github-actions]

Checkmarx One – Scan Summary & Details – 368f1db1-c217-49ae-8209-1f2a46d53c28

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.99%. Comparing base (21afa81) to head (1c00c1a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6175      +/-   ##
==========================================
+ Coverage   84.97%   84.99%   +0.02%     
==========================================
  Files         735      723      -12     
  Lines       53087    52858     -229     
  Branches     7678     7678              
==========================================
- Hits        45113    44929     -184     
+ Misses       5291     5246      -45     
  Partials     2683     2683              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[claude]

❌ Missing test coverage and documentation

Details

This new extension function needs:

1. Direct unit tests - Codecov reports 60% patch coverage. Add tests for:

   • IOException passthrough case (line 130)
   • Non-IOException wrapping case (line 131)

2. KDoc documentation - Per project guidelines, explain:

   • Why this conversion is needed (Retrofit compatibility)
   • Behavior for each case

Suggested tests:

@Nested
inner class ToIoExceptionExtension {
    @Test
    fun `toIoException returns IOException unchanged`() {
        val original = IOException("test error")
        val result = original.toIoException()
        assertSame(original, result)
    }

    @Test
    fun `toIoException wraps non-IOException in IOException`() {
        val original = IllegalStateException("test error")
        val result = original.toIoException()
        assertTrue(result is IOException)
        assertSame(original, result.cause)
    }
}

Suggested documentation:

/**
 * Converts this [Throwable] to an [IOException] for Retrofit compatibility.
 * 
 * Retrofit's error handling requires [IOException] instances. This extension ensures
 * all exceptions thrown during token refresh are properly wrapped.
 */

[david-livefront]

Thanks @SaintPatrck