Add public error contract tests

[tolgahan-arikan]

Summary

• Normalize SDK error classification so public errors preserve upstream details, nullable retry semantics, transaction boundary codes, and operation-specific surfaces.
• Add a centralized PublicErrorContractsTest matrix covering SDK public runtime error families equivalent to the non-connector TypeScript SDK work.
• Document the public error contract matrix and update API/testing/agent guidance so future public error assertions stay centralized instead of duplicated in focused tests.

Notes

• Removed/trimmed duplicate public-field assertions from focused tests where the central matrix now owns the contract.
• Kept focused edge-case coverage for generated WaaS status normalization and generated public-wallet endpoint behavior.

Validation

• ./gradlew --build-cache :oms-client-kotlin-sdk:testDebugUnitTest :oms-client-kotlin-sdk:lintDebug :app:lintDebug :app:assembleDebug :oms-client-kotlin-sdk:checkPublicApiDoesNotExposeGeneratedWaas
• git diff --check
• pre-push ./gradlew ktlintCheck hook

[claude]

Claude finished @tolgahan-arikan's task —— View job

---

PR Review: Add public error contract tests

Comprehensive review completed. This is a well-architected PR that significantly improves error handling and testing coverage.

Review Findings

✅ Overall Assessment: LGTM

This PR successfully normalizes SDK error classification and establishes comprehensive error contract testing. The implementation is secure, well-tested, and follows established patterns.

Strengths

1. Excellent Error Architecture: The normalized error hierarchy in OmsSdkError.kt provides clear, stable error codes while preserving upstream diagnostic details through upstreamError.

2. Comprehensive Test Coverage: PublicErrorContractsTest.kt (1428 lines) provides extensive matrix coverage of all public error surfaces, matching the audit matrix in docs/error-contracts.md.

3. Security-Conscious Design:

   • Error handling preserves transaction boundaries and retry semantics appropriately
   • No sensitive information leaked through error messages
   • Proper upstream error isolation via OmsUpstreamError

4. Robust Client Updates: IndexerClient.kt and WalletClient.kt properly wrap operations with runOmsOperation to ensure consistent error classification.

Technical Analysis

Error Handling (OmsSdkError.kt:186-220):

• runOmsOperation properly catches and classifies exceptions
• Transaction boundary detection prevents nested operation overrides
• Upstream error normalization handles WebRPC edge cases correctly

Client Integration:

• IndexerClient properly creates upstream errors for transport failures (IndexerClient.kt:148-152)
• WalletClient maintains existing auth/signing logic while adding error wrapping
• No changes to security-sensitive credential or session handling paths

Test Quality:

• Tests exercise real public APIs with mocked external boundaries
• Error contract assertions focus on stable public fields only
• Comprehensive coverage of transport failures, domain errors, and local validation

Documentation & Standards

• New docs/error-contracts.md provides clear audit matrix for error behavior
• API documentation properly updated in docs/api.md
• Code follows existing patterns and KDoc standards
• AGENTS.md and TESTING.md appropriately updated with error contract guidance

Code Style

• Consistent with existing Kotlin conventions
• Proper enum/data class usage for stable public API
• Clear separation between public contracts and internal implementation

Minor Observations

1. Nit: Some test helper functions could benefit from more descriptive names, but this doesn't impact functionality.

2. Nit: PublicErrorContractsTest is quite large (1428 lines) - consider if some test groupings could be split, though the current organization works well for comprehensive contract coverage.

Security Analysis

• ✅ No changes to wallet/keystore/signing core logic
• ✅ Error messages don't expose sensitive data
• ✅ Upstream error details properly isolated
• ✅ Transaction retry semantics preserve safety boundaries

Public API Impact

All error-related API additions are backward compatible and properly documented.

Recommendation: Approve and Merge

This PR establishes excellent error handling foundations and comprehensive test coverage without introducing security risks or breaking changes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d371bd502

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @tolgahan-arikan's task —— View job

---

PR Review: Add public error contract tests

Comprehensive review completed. This is a well-architected PR that significantly improves error handling and testing coverage.

Review Findings:

✅ Overall Assessment: LGTM with One Minor Issue

This PR successfully normalizes SDK error classification and establishes comprehensive error contract testing. The implementation is secure, well-tested, and follows established patterns.

Strengths:

1. Excellent Error Architecture: The normalized error hierarchy in OmsSdkError.kt provides clear, stable error codes while preserving upstream diagnostic details through upstreamError.

2. Comprehensive Test Coverage: PublicErrorContractsTest.kt (1428 lines) provides extensive matrix coverage of all public error surfaces, matching the audit matrix in docs/error-contracts.md.

3. Security-Conscious Design:

   • Error handling preserves transaction boundaries and retry semantics appropriately
   • No sensitive information leaked through error messages
   • Proper upstream error isolation via OmsUpstreamError

4. Robust Client Updates: IndexerClient.kt and WalletClient.kt properly wrap operations with runOmsOperation to ensure consistent error classification.

Technical Analysis:

Error Handling (OmsSdkError.kt:186-220):

• runOmsOperation properly catches and classifies exceptions
• Transaction boundary detection prevents nested operation overrides
• Upstream error normalization handles WebRPC edge cases correctly

Client Integration:

• IndexerClient properly creates upstream errors for transport failures (IndexerClient.kt:148-152)
• WalletClient maintains existing auth/signing logic while adding error wrapping
• No changes to security-sensitive credential or session handling paths

Test Quality:

• Tests exercise real public APIs with mocked external boundaries
• Error contract assertions focus on stable public fields only
• Comprehensive coverage of transport failures, domain errors, and local validation

Issues Found:

🔴 Priority Issue: Flow Collector Exception Handling (WalletClient.kt:977)

The codex feedback is valid and represents a genuine bug:

fun listAccessPages(pageSize: UInt? = null): Flow<ListAccessResponse> =
    flow {
        runOmsOperation(OmsSdkOperation.WalletListAccessPages) {
            // ... pagination logic
            emit(response) // ❌ Problem: emit() inside runOmsOperation
        }
    }

Problem: If downstream collectors throw IllegalArgumentException or IllegalStateException while processing emitted responses, these exceptions are caught by runOmsOperation and rethrown as SDK validation/session errors for the wrong operation.

Solution: Move only the requestListAccessPage call inside runOmsOperation, keep emit() outside:

fun listAccessPages(pageSize: UInt? = null): Flow<ListAccessResponse> =
    flow {
        var cursor: String? = null
        do {
            val response = runOmsOperation(OmsSdkOperation.WalletListAccessPage) {
                requestListAccessPage(pageSize = pageSize, cursor = cursor)
            }
            emit(response)
            cursor = response.page?.cursor?.takeIf { it.isNotBlank() }
        } while (cursor != null)
    }

Documentation & Standards:

• New docs/error-contracts.md provides clear audit matrix for error behavior
• API documentation properly updated in docs/api.md
• Code follows existing patterns and KDoc standards
• AGENTS.md and TESTING.md appropriately updated with error contract guidance

Code Style:

• Consistent with existing Kotlin conventions
• Proper enum/data class usage for stable public API
• Clear separation between public contracts and internal implementation

Minor Observations:

1. Nit: Some test helper functions could benefit from more descriptive names, but this doesn't impact functionality.

2. Nit: PublicErrorContractsTest is quite large (1428 lines) - consider if some test groupings could be split, though the current organization works well for comprehensive contract coverage.

Security Analysis:

• ✅ No changes to wallet/keystore/signing core logic
• ✅ Error messages don't expose sensitive data
• ✅ Upstream error details properly isolated
• ✅ Transaction retry semantics preserve safety boundaries

Public API Impact:

All error-related API additions are backward compatible and properly documented.

Recommendation: Approve After Fixing Flow Issue

This PR establishes excellent error handling foundations. The flow collector issue should be fixed before merge, but it's a straightforward change that doesn't affect the overall architecture.

---

View job

[claude]

Claude finished @tolgahan-arikan's task —— View job

---

PR Review: Add public error contract tests

Comprehensive review completed. This is a well-architected PR that significantly improves error handling and testing coverage.

Review Findings:

✅ Overall Assessment: LGTM with One Minor Issue

This PR successfully normalizes SDK error classification and establishes comprehensive error contract testing. The implementation is secure, well-tested, and follows established patterns.

Strengths:

1. Excellent Error Architecture: The normalized error hierarchy in OmsSdkError.kt provides clear, stable error codes while preserving upstream diagnostic details through upstreamError.

2. Comprehensive Test Coverage: PublicErrorContractsTest.kt (1428 lines) provides extensive matrix coverage of all public error surfaces, matching the audit matrix in docs/error-contracts.md.

3. Security-Conscious Design:

   • Error handling preserves transaction boundaries and retry semantics appropriately
   • No sensitive information leaked through error messages
   • Proper upstream error isolation via OmsUpstreamError

4. Robust Client Updates: IndexerClient.kt and WalletClient.kt properly wrap operations with runOmsOperation to ensure consistent error classification.

Technical Analysis:

Error Handling (OmsSdkError.kt:186-220):

• runOmsOperation properly catches and classifies exceptions
• Transaction boundary detection prevents nested operation overrides
• Upstream error normalization handles WebRPC edge cases correctly

Client Integration:

• IndexerClient properly creates upstream errors for transport failures (IndexerClient.kt:148-152)
• WalletClient maintains existing auth/signing logic while adding error wrapping
• No changes to security-sensitive credential or session handling paths

Test Quality:

• Tests exercise real public APIs with mocked external boundaries
• Error contract assertions focus on stable public fields only
• Comprehensive coverage of transport failures, domain errors, and local validation

Issues Found:

🔴 Priority Issue: Flow Collector Exception Handling (WalletClient.kt:977)

The codex feedback is valid and represents a genuine bug:

fun listAccessPages(pageSize: UInt? = null): Flow<ListAccessResponse> =
    flow {
        runOmsOperation(OmsSdkOperation.WalletListAccessPages) {
            // ... pagination logic
            emit(response) // ❌ Problem: emit() inside runOmsOperation
        }
    }

Problem: If downstream collectors throw IllegalArgumentException or IllegalStateException while processing emitted responses, these exceptions are caught by runOmsOperation and rethrown as SDK validation/session errors for the wrong operation.

Solution: Move only the requestListAccessPage call inside runOmsOperation, keep emit() outside:

fun listAccessPages(pageSize: UInt? = null): Flow<ListAccessResponse> =
    flow {
        var cursor: String? = null
        do {
            val response = runOmsOperation(OmsSdkOperation.WalletListAccessPage) {
                requestListAccessPage(pageSize = pageSize, cursor = cursor)
            }
            emit(response)
            cursor = response.page?.cursor?.takeIf { it.isNotBlank() }
        } while (cursor != null)
    }

Documentation & Standards:

• New docs/error-contracts.md provides clear audit matrix for error behavior
• API documentation properly updated in docs/api.md
• Code follows existing patterns and KDoc standards
• AGENTS.md and TESTING.md appropriately updated with error contract guidance

Code Style:

• Consistent with existing Kotlin conventions
• Proper enum/data class usage for stable public API
• Clear separation between public contracts and internal implementation

Minor Observations:

1. Nit: Some test helper functions could benefit from more descriptive names, but this doesn't impact functionality.

2. Nit: PublicErrorContractsTest is quite large (1428 lines) - consider if some test groupings could be split, though the current organization works well for comprehensive contract coverage.

Security Analysis:

• ✅ No changes to wallet/keystore/signing core logic
• ✅ Error messages don't expose sensitive data
• ✅ Upstream error details properly isolated
• ✅ Transaction retry semantics preserve safety boundaries

Public API Impact:

All error-related API additions are backward compatible and properly documented.

Recommendation: Approve After Fixing Flow Issue

This PR establishes excellent error handling foundations. The flow collector issue should be fixed before merge, but it's a straightforward change that doesn't affect the overall architecture.

---

[tolgahan-arikan]

about issue mentioned in #47 (comment)

Fixed in 29476e1.

Current PR head keeps only requestListAccessPage(...) inside runOmsOperation(OmsSdkOperation.WalletListAccessPages). emit(response) now happens outside the SDK error wrapper, so downstream collector/operator exceptions are no longer remapped as SDK errors.