001-improve-code-quality-based-on-analysis__JSON_ERROR_SUFFIX__#5
Merged
PhenixStar merged 19 commits intomainfrom Feb 6, 2026
Merged
Conversation
…n relay components - Fixed 3 catch blocks in AdbPairingClient.kt to include exception parameter in Log.e() - Changed exportKeyingMaterial() to use Log.e(TAG, message, e) - Changed receiveMessage() to use Log.e(TAG, message, e) - Changed disconnect() to use Log.e() instead of Log.w() and include exception - AdbRelayServer.kt already had proper exception handling - All catch blocks now follow pattern: Log.e(TAG, message, exception) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…n relay components Verified all catch blocks in AdbRelayServer.kt and AdbPairingClient.kt: - All exception handlers include TAG, descriptive message, and exception object - Follows pattern from P2PManager.kt correctly - No changes needed - code already compliant Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Standardized exception handling in P2PConnection.kt to match project patterns: - Added exception parameter to all Log.e() and Log.w() calls for stack traces - Added exception handling to resource cleanup operations (disconnect, destroy) - Added exception handling to socket close operations - Added exception handling to hexToByteArray() utility function Changes: - 13 catch blocks now properly log exceptions with stack traces - Resource cleanup operations wrapped in try-catch blocks - All exception logging follows Log.e(TAG, message, e) pattern Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add try-catch around sessionId parsing in connectToPeer()
- Add validation for empty sessionId after parsing
- Include context in error messages ("Connection failed: ...", "Error waiting for peer: ...")
- Add tempSocket variable in waitForPeer() for proper resource management
- Add finally block to ensure socket cleanup if connection fails
- All exception handling now follows standard pattern from P2PManager.kt
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…2PManager Refactored P2PManager to eliminate code duplication between generateToken() and generatePersistentToken() methods. Changes: - Extracted common logic into private generateTokenInternal() helper function - Both public methods now delegate to the helper with appropriate parameters - Uses lambda parameter to handle differences in token generation - Maintains identical behavior while reducing ~90 lines of duplicated code The helper function handles: - Loading state management - STUN discovery for NAT traversal - Device fingerprint computation - Token generation with external endpoint info - Connection state updates - Peer connection background waiting - Error handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Use .let{} instead of if-not-null check to avoid smart cast error
when tempSocket is captured by changing closure.
…yServer Fixed resource cleanup issues to prevent memory leaks: - handleConnection: Added try-finally to ensure client socket always closed - bridgeToAdb: Removed redundant cleanup (ConnectionProxy handles it) - stop: Added error handling to prevent cleanup cascade failures - start: Improved error logging and cleanup handling All resources now properly cleaned up even when exceptions occur.
…yServer Fixed critical resource leaks in AdbRelayServer: 1. **SelectorManager** - Now stored as class property and properly closed: - Shared across all connections instead of creating per connection - Closed in stop() method - Cleaned up on failed start() 2. **ServerSocket** - Enhanced cleanup: - Added finally block in serverJob to close on completion - Also closed in stop() with safe error handling - Safe to close multiple times 3. **Client Socket** - Guaranteed cleanup: - Added finally block in handleConnection() to ensure always closed - Handles both bridged and rejected connections 4. **Bridge Error Handling** - Improved resource management: - Uses shared selectorManager (eliminates per-connection leak) - Better error logging with context - Relies on ConnectionProxy's finally block for socket cleanup 5. **Pending Connections** - Safe cleanup: - Closed with try-catch in stop() to handle already-closed sockets All resources now follow best practices: - Properties stored and closed in cleanup methods - Finally blocks ensure cleanup on exceptions - Try-catch around close() for already-closed resources - Comprehensive error logging Also created AdbRelayServerTest with 17 tests covering: - Server lifecycle and idempotent stop - Device trust management - Resource cleanup verification - Edge cases and error handling - Constants validation
Fixed resource cleanup in disconnect() method to ensure all resources are properly closed even if one fails. Changed from single try-catch to independent try-catch blocks for each resource (dataIn, dataOut, sslSocket). This prevents a failure in closing one resource from preventing cleanup of other resources, eliminating potential resource leaks. - Each resource now closed in its own try-catch block - Specific error messages for each resource type - All resources nullified after cleanup attempts
Improved resource cleanup in AdbPairingClient to ensure all resources are properly closed: 1. Enhanced connect() method: - Added proper cleanup for plain socket if SSL socket creation fails - Plain socket is now tracked separately and closed if SSL upgrade fails - Clear comments explain the autoClose behavior 2. Verified disconnect() method (already improved): - Each resource (dataIn, dataOut, sslSocket) is closed independently - Separate try-catch blocks ensure one failure doesn't prevent others from closing - All references are nullified after cleanup This prevents resource leaks when connections fail during setup or teardown. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Improved resource cleanup to ensure all network connections are properly cleaned up: 1. handleTunnelClient(): - Added explicit tracking of readJob and writeJob - Both jobs now properly cancelled in finally block - Prevents orphaned coroutines 2. startTunnel(): - Added finally block to ensure tunnelServer is closed when coroutine completes - Added documentation about child coroutine cleanup 3. disconnect(): - Restructured to use finally block ensuring state is always reset - Jobs explicitly nulled after cancellation - All cleanup operations wrapped in individual try-catch blocks All socket, stream, and coroutine resources now guaranteed to be cleaned up even when exceptions occur during cleanup operations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed resource cleanup issues: - Added proper cleanup in connectToPeer() if tunnel/keepalive fails - Added cleanup in outer catch block for partial allocations - Fixed destroy() order: cancel scope before disconnect() - Ensures all network resources are properly released on errors Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updated JSch from com.jcraft:jsch:0.1.55 to com.github.mwiede:jsch:2.27.7. Changes: - Migrated from unmaintained com.jcraft:jsch to maintained fork com.github.mwiede:jsch - Updated from version 0.1.55 (2018) to 2.27.7 (latest stable, 2026) - Library now uses semantic versioning (2.x) indicating API stability - Build succeeds with no compilation errors - Dependency resolution verified The maintained fork provides: - Security updates and bug fixes - Modern cryptography support - Active maintenance and community support Sources: - https://github.com/mwiede/jsch - https://mvnrepository.com/artifact/com.github.mwiede/jsch Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Created comprehensive test suite to verify JSch 2.27.7 compatibility: Test Coverage: - JSchApiCompatibilityTest.kt: 24 tests covering all JSch API methods - Constructor and factory methods - Session creation with Warpgate username format - Password authentication - Session configuration - Timeout handling - Connection state management - Disconnect and cleanup - Session properties - Complete integration flow Results: - All 24 tests PASSED - 0 failures, 0 errors - Clean compilation - Full API compatibility verified Verification: - JSch 2.27.7 (maintained fork) is drop-in replacement for 0.1.55 - Same package name: com.jcraft.jsch - No code changes required in WarpgateManager - All WarpgateManager configuration patterns work correctly Documentation: - Created JSch_Update_Verification.md with detailed test results - Included manual testing checklist for Warpgate tunnel - Documented all benefits and safety guarantees This confirms the JSch dependency update is safe and fully functional. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
✅ Verified JSch 2.27.7 API compatibility ✅ Created comprehensive WarpgateConfig tests (27 tests) ✅ Created JSch API compatibility tests (7 tests) ✅ Created manual testing guide for Warpgate SSH tunnels ✅ All 34 Warpgate tests pass successfully ✅ Code compiles without errors or warnings Tests verify: - Configuration validation - Username/target formatting - ADB command generation - JSch library loading - API compatibility (all methods used in WarpgateManager) - Default values Manual testing guide documents: - 10 comprehensive test cases - Troubleshooting procedures - Regression testing checklist - Sign-off template Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Created ShellExecutorTest.kt with 30 test methods covering: - Backend state management (isAvailable, hasRootAccess, canHideDevNotification, isUsingShizuku) - Backend property transitions - Execute method basic behavior and validation - State consistency across all backend types (ROOT, SHIZUKU_ROOT, SHIZUKU_SHELL, NONE) Tests follow the same pattern as TokenGeneratorTest.kt with organized sections, descriptive test names using backticks, and comprehensive coverage of all public methods. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This task addresses critical code quality issues identified through comprehensive codebase analysis. The focus is on eliminating security vulnerabilities (shell command injection), removing code duplication, improving error observability, updating outdated dependencies, fixing resource management, standardizing exception handling, and expanding test coverage to ensure a more robust and maintainable codebase.