[PM-26063] Replace BitwardenErrors with FlightRecorderErrors in FlightRecorder

[matt-livefront]

🎟️ Tracking

PM-26063

📔 Objective

Updates the usages of BitwardenError within FlightRecorder to use FlightRecorderError. This provides more consistency around flight recorder errors and removes the BitwardenError dependency prior to moving the flight recorder into BitwardenKit.

⏰ 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 @matt-livefront's task —— View job

---

Code Review Complete ✅

Todo List:

• Read changed files to understand modifications
• Read architecture and testing documentation
• Perform comprehensive code review
• Post detailed review feedback

---

Summary of Changes

This PR successfully replaces BitwardenError.generalError() calls with dedicated FlightRecorderError cases within the FlightRecorder implementation. The changes include:

• Added 4 new error cases with associated values: fileSizeError, logLifecycleTimerError, removeExpiredLogError, writeMessageError
• Implemented CustomNSError conformance with stable error codes
• Implemented Equatable conformance for error comparison
• Updated 4 error reporting sites to use the new error types
• Updated tests to validate the new error types

Files Changed:

• BitwardenShared/Core/Platform/Services/FlightRecorder.swift (+74/-21)
• BitwardenShared/Core/Platform/Services/FlightRecorderTests.swift (+6/-9)

---

Critical Issues Found

🔴 Missing Test Coverage for New Error Extensions (lines 108-162)

Location: FlightRecorder.swift:108-162

Issue: The new CustomNSError and Equatable conformance extensions have zero test coverage. Codecov reports 6 lines missing coverage in this file, and these are likely the untested portions of the new error extensions.

Why this matters:

1. errorCode mapping is critical for error tracking/analytics - wrong codes could break error monitoring
2. The comment states "New cases should be appended (vs alphabetized)...This ensures the code for existing errors doesn't change" - without tests, future developers might accidentally reorder cases and break error code stability
3. Equatable implementation for error comparison needs validation, especially the String(reflecting:) comparison for associated errors
4. errorUserInfo dictionary construction needs verification

Recommendation:
Add comprehensive unit tests for:

/// `FlightRecorderError.errorCode` returns stable error codes
func test_flightRecorderError_errorCode() {
    XCTAssertEqual(FlightRecorderError.dataUnavailable.errorCode, 1)
    XCTAssertEqual(FlightRecorderError.deletionNotPermitted.errorCode, 2)
    XCTAssertEqual(FlightRecorderError.fileSizeError(BitwardenTestError.example).errorCode, 3)
    XCTAssertEqual(FlightRecorderError.logLifecycleTimerError(BitwardenTestError.example).errorCode, 4)
    XCTAssertEqual(FlightRecorderError.logNotFound.errorCode, 5)
    XCTAssertEqual(FlightRecorderError.removeExpiredLogError(BitwardenTestError.example).errorCode, 6)
    XCTAssertEqual(FlightRecorderError.writeMessageError(BitwardenTestError.example).errorCode, 7)
}

/// `FlightRecorderError.errorDomain` returns the correct domain
func test_flightRecorderError_errorDomain() {
    XCTAssertEqual(FlightRecorderError.errorDomain, "FlightRecorderError")
}

/// `FlightRecorderError.errorUserInfo` includes underlying errors
func test_flightRecorderError_errorUserInfo() {
    let underlyingError = BitwardenTestError.example
    let error = FlightRecorderError.fileSizeError(underlyingError)
    let userInfo = error.errorUserInfo
    
    XCTAssertEqual(userInfo["Error Type"] as? String, String(reflecting: error))
    XCTAssertNotNil(userInfo[NSUnderlyingErrorKey])
}

/// `FlightRecorderError` equatable works correctly for simple cases
func test_flightRecorderError_equatable_simple() {
    XCTAssertEqual(FlightRecorderError.dataUnavailable, .dataUnavailable)
    XCTAssertNotEqual(FlightRecorderError.dataUnavailable, .logNotFound)
}

/// `FlightRecorderError` equatable works correctly for associated value cases
func test_flightRecorderError_equatable_associatedValues() {
    let error1 = FlightRecorderError.fileSizeError(BitwardenTestError.example)
    let error2 = FlightRecorderError.fileSizeError(BitwardenTestError.example)
    let error3 = FlightRecorderError.fileSizeError(NSError(domain: "test", code: 1))
    
    XCTAssertEqual(error1, error2)
    XCTAssertNotEqual(error1, error3)
}

📊 Coverage Impact Analysis

Current Codecov report shows:

• Patch coverage: 86.04% (6 lines missing)
• Project coverage: 85.28% (unchanged)

The missing 6 lines are likely:

• Lines in errorCode switch statement (lines 115-122)
• Lines in errorUserInfo switch statement (lines 131-138)
• Lines in Equatable implementation (lines 148-160)

These extensions are publicly accessible through protocol conformance and should be tested to ensure stability.

---

❌ Code Issues & Improvements

1. ⚠️ Potential Information Loss in Error Handling

Location: FlightRecorder.swift:287 (previously included file/line context)

Before:

errorReporter.log(error: BitwardenError.generalError(
    type: "Flight Recorder Log Error",
    message: "\(fileName):\(line) Unable to write message to log: \(message)",
    error: error,
))

After:

errorReporter.log(error: FlightRecorderError.writeMessageError(error))

Issue: The new implementation loses valuable debugging context:

• ❌ Lost: fileName - which file called the log function
• ❌ Lost: line - which line number called the log function
• ❌ Lost: message - the actual log message content that failed to write

Why this matters: When debugging write failures in production, knowing what message failed to write and where it was called from is crucial for diagnosing the root cause.

Recommendation: Consider one of these approaches:

Option 1: Enhanced Error Case (Preferred)

case writeMessageError(message: String, file: String, line: UInt, underlyingError: Error)

Update the error site:

errorReporter.log(error: FlightRecorderError.writeMessageError(
    message: message,
    file: file,
    line: line,
    underlyingError: error
))

Update CustomNSError conformance:

case let .writeMessageError(message, file, line, underlyingError):
    userInfo[NSUnderlyingErrorKey] = underlyingError
    userInfo["message"] = message
    userInfo["file"] = file
    userInfo["line"] = line

Option 2: Preserve Context in ErrorReporter

If the ErrorReporter already captures file/line context through its own logging, this may be acceptable. Verify that the error reporter preserves sufficient context.

Question for PR author: Does the ErrorReporter.log() method capture sufficient context (file, line, message) automatically, or is this information now lost?

---

2. 📝 Missing DocC Documentation

Location: FlightRecorder.swift:83-104, 108-162

Issue: The new FlightRecorderError enum cases and protocol conformance extensions are missing DocC documentation.

Current state:

/// An enumeration of errors thrown by a `FlightRecorder`.
///
enum FlightRecorderError: Error {
    /// The stored flight recorder data doesn't exist.
    case dataUnavailable
    
    // ... existing cases with docs ...
    
    case fileSizeError(Error)  // ❌ No documentation
    case logLifecycleTimerError(Error)  // ❌ No documentation
    case removeExpiredLogError(Error)  // ❌ No documentation
    case writeMessageError(Error)  // ❌ No documentation
}

// MARK: - FlightRecorderError + CustomNSError
// ❌ No documentation for extension

extension FlightRecorderError: CustomNSError {
    // ❌ No documentation for properties
}

Required per CLAUDE.md:

Document Everything: Everything in the code requires DocC documentation except for protocol properties/functions implementations as the docs for them will be in the protocol.

Recommendation: Add complete DocC documentation:

enum FlightRecorderError: Error {
    /// The stored flight recorder data doesn't exist.
    case dataUnavailable

    /// Deletion of the log isn't permitted if the log is the active log.
    case deletionNotPermitted

    /// Unable to determine the log's file size.
    ///
    /// - Parameter Error: The underlying error that occurred while attempting to determine the file size.
    case fileSizeError(Error)

    /// Error waiting for next flight recorder log lifecycle.
    ///
    /// - Parameter Error: The underlying error that occurred while waiting for the next log lifecycle event.
    case logLifecycleTimerError(Error)

    /// The specified log wasn't found in the stored flight recorder data.
    case logNotFound

    /// Unable to remove file for expired log.
    ///
    /// - Parameter Error: The underlying error that occurred while attempting to remove the expired log file.
    case removeExpiredLogError(Error)

    /// Unable to write message to log.
    ///
    /// - Parameter Error: The underlying error that occurred while attempting to write to the log file.
    case writeMessageError(Error)
}

// MARK: - FlightRecorderError + CustomNSError

/// Provides `NSError` representation for `FlightRecorderError`.
extension FlightRecorderError: CustomNSError {
    /// The error domain for flight recorder errors.
    static var errorDomain: String { "FlightRecorderError" }

    /// Returns a stable error code for each error case.
    ///
    /// Error codes are assigned sequentially and must never change once assigned
    /// to maintain compatibility with error tracking systems.
    var errorCode: Int {
        // ... implementation
    }

    /// Returns user info dictionary including the error type and any underlying errors.
    var errorUserInfo: [String: Any] {
        // ... implementation
    }
}

// MARK: - FlightRecorderError + Equatable

/// Provides equality comparison for `FlightRecorderError`.
///
/// Associated errors are compared using their string representation to enable
/// testing and error matching.
extension FlightRecorderError: Equatable {
    // ... implementation
}

---

3. 🤔 Error Code Stability Documentation

Location: FlightRecorder.swift:112-114

Current code:

// NOTE: New cases should be appended (vs alphabetized) to this switch statement with an
// incremented integer. This ensures the code for existing errors doesn't change.

Good: The comment explains the requirement clearly.

Improvement opportunity: Consider making this more enforceable:

var errorCode: Int {
    // IMPORTANT: Error codes must remain stable once assigned.
    // When adding new error cases:
    // 1. Append new cases to the end of this switch
    // 2. Assign the next sequential integer
    // 3. NEVER reorder or change existing codes
    //
    // These codes are used by error tracking systems and must not change
    // across app versions to maintain historical error data integrity.
    switch self {
    case .dataUnavailable: 1
    case .deletionNotPermitted: 2
    case .fileSizeError: 3
    case .logLifecycleTimerError: 4
    case .logNotFound: 5
    case .removeExpiredLogError: 6
    case .writeMessageError: 7
    }
}

Also add a test that explicitly validates error code values (see Critical Issue #1 above) to prevent accidental changes.

---

4. 🎨 Equatable Implementation Consideration

Location: FlightRecorder.swift:156-157

Current code:

case let (.fileSizeError(lhsError), .fileSizeError(rhsError)),
     let (.logLifecycleTimerError(lhsError), .logLifecycleTimerError(rhsError)),
     let (.removeExpiredLogError(lhsError), .removeExpiredLogError(rhsError)),
     let (.writeMessageError(lhsError), .writeMessageError(rhsError)):
    String(reflecting: lhsError) == String(reflecting: rhsError)

Consideration: Using String(reflecting:) for error comparison is a pragmatic approach for testing, but be aware:

Pros:

• ✅ Works for testing scenarios
• ✅ Simple implementation
• ✅ Handles errors without Equatable conformance

Cons:

• ⚠️ String representation may not be stable across Swift versions
• ⚠️ May produce false negatives for semantically equal errors with different string representations
• ⚠️ Performance overhead (string allocation/comparison)

Recommendation: This is acceptable for the current use case (testing), but document the limitation:

/// Provides equality comparison for `FlightRecorderError`.
///
/// Associated errors are compared using their string representation via `String(reflecting:)`.
/// This enables testing and error matching without requiring underlying errors to conform to `Equatable`.
///
/// - Note: This comparison may produce false negatives for semantically equal errors that have
///   different string representations.
extension FlightRecorderError: Equatable {
    // ... implementation
}

---

✅ Good Practices Observed

• Stable error codes: Error code assignment follows append-only pattern with clear documentation
• Test coverage: Existing error reporting call sites have corresponding test updates
• Error context preservation: Underlying errors are properly wrapped with NSUnderlyingErrorKey
• Type safety: Dedicated error types replace stringly-typed general errors

---

Action Items

🔴 Must Address Before Merge:

1. Add comprehensive tests for CustomNSError conformance - Test error codes, error domain, and errorUserInfo dictionary construction (see Critical Issue Dependency Dashboard #1)
2. Add tests for Equatable conformance - Test both simple and associated value cases (see Critical Issue Dependency Dashboard #1)
3. Add DocC documentation - Document all new error cases and extensions per project guidelines (see Issue re-open Update bitwarden/gh-actions digest to 67ab95d #2)

🟡 Recommended Before Merge:

4. Evaluate information loss in error handling - Verify that file/line/message context is preserved through ErrorReporter or enhance the error case (see Issue Dependency Dashboard #1)
5. Enhance error code stability documentation - Strengthen the comment explaining why error codes must remain stable (see Issue Update dependency prettier to v3 #3)
6. Document Equatable implementation limitations - Add note about String(reflecting:) comparison approach (see Issue Lock file maintenance #4)

🟢 Optional/Future Improvements:

7. Consider adding error description or localized description conformance for better error messages in logs

---

Architecture & Guidelines Compliance

✅ Architecture compliance: Changes follow established patterns for error types in the Core/Platform/Services layer

✅ Testing guidelines: Test updates properly validate error types, though coverage for new extensions is missing

⚠️ Code style: Missing DocC documentation violates project requirement: "Everything in the code requires DocC documentation except for protocol properties/functions implementations"

✅ Dependency management: No new dependencies introduced; proper use of existing error reporting infrastructure

✅ Security: No security implications - error types don't expose sensitive information

---

[github-actions]

Checkmarx One – Scan Summary & Details – bacf45b8-4ee9-48d0-a924-edd0de50a7d3

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

[codecov]

Codecov Report

❌ Patch coverage is 89.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.29%. Comparing base (b686247) to head (3d3b891).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...Shared/Core/Platform/Services/FlightRecorder.swift	88.00%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2088   +/-   ##
=======================================
  Coverage   85.28%   85.29%           
=======================================
  Files        1695     1695           
  Lines      144592   144618   +26     
=======================================
+ Hits       123319   123351   +32     
+ Misses      21273    21267    -6     

☔ 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.