Crash: SecurityScanner throws on Blockaid 'Simulation Error' response, kills main thread (#4334) by Vaulty-bot · Pull Request #4336 · vultisig/vultisig-android

Vaulty-bot · 2026-04-30T19:03:07Z

Fixes #4334

Changes

SecurityScannerExtensions.kt — runSecurityScan changed from a generic <T> returning T to returning SecurityScannerResult directly; the catch block now returns a fallback SecurityScannerResult(isSecure=false, riskLevel=MEDIUM, description="Scan unavailable") instead of rethrowing SecurityScannerException, preventing crashes from propagating to the main thread.
BlockaidExtensions.kt — toValidationRiskLevel() no longer throws SecurityScannerException when Blockaid returns an error/simulation-failure status; it now logs a warning via Timber.w and returns SecurityRiskLevel.MEDIUM gracefully.
BlockaidExtensionsTest.kt — new test class with three cases verifying that toSecurityScannerResult does not throw and returns isSecure=false + MEDIUM risk when the response has validation.status="Error", resultType="Error", or top-level status="Error".

Checklist

Summary by CodeRabbit

Bug Fixes
- Security scanner now fails gracefully: when scans are unavailable or error, the app returns a consistent medium-risk result and logs the issue instead of aborting the operation.
New Features
- Keys signing flow shows an explicit "waiting for peers" state with a loading screen, displays missing peers, prevents screen dimming, and reflects in-progress progress.
Tests
- Added tests covering security scanner error scenarios to ensure stable behavior.

…ponse, kills main thread (#4334)

coderabbitai · 2026-04-30T19:03:23Z

Warning

Rate limit exceeded

@Vaulty-bot has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 43 minutes and 26 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5ef7a060-1415-4dbc-b3e5-99899df7baf8

📥 Commits

Reviewing files that changed from the base of the PR and between a3f22b6 and 7f65c1d.

📒 Files selected for processing (3)

data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt
data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensions.kt
data/src/test/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensionsTest.kt

📝 Walkthrough

Walkthrough

Refactors scanner error handling to return a deterministic SecurityScannerResult (scan-unavailable MEDIUM risk) on failures instead of throwing, adjusts Blockaid mappings to treat validation errors as soft failures, adds tests for Blockaid error responses, and introduces a WaitingForPeer keysign state with UI handling.

Changes

Cohort / File(s)	Summary
Security scanner boundary `data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt`	Changed `runSecurityScan` signature to return `SecurityScannerResult`; catches non-cancellation exceptions, logs them, and returns a default "scan unavailable" result instead of throwing.
Blockaid mapping & handling `data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensions.kt`	Mapped Blockaid validation `Error`/result `Error`/top-level `status = "Error"` to `SecurityRiskLevel.MEDIUM` and log instead of throwing; added KDoc to mapping functions.
Tests `data/src/test/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensionsTest.kt`	Added unit tests covering three Blockaid error scenarios confirming results are non-secure and map to `MEDIUM` risk.
Keysign state & UI `app/src/main/java/com/vultisig/wallet/ui/models/keysign/KeysignViewModel.kt`, `app/src/main/java/com/vultisig/wallet/ui/screens/keysign/Keysign.kt`	Added `KeysignState.WaitingForPeer(missingPeers)`, wired `onWaitingForPeers` callback into DKLS keysign flow, and rendered a loading UI for the waiting state (prevents screen dimming).

Sequence Diagram(s)

sequenceDiagram
    participant UI as User / UI
    participant VM as KeysignViewModel
    participant Scanner as SecurityScannerService
    participant Blockaid as BlockaidExtensions
    participant Mapper as SecurityScannerExtensions

    UI->>VM: initiate pre-sign (start scan)
    VM->>Scanner: request scan (transaction)
    Scanner->>Blockaid: call Blockaid API & parse response
    Blockaid->>Mapper: toSecurityScannerResult(response)
    Mapper->>Mapper: map status -> if error log & return MEDIUM unavailable
    Mapper-->>Scanner: SecurityScannerResult(MEDIUM, isSecure=false)
    Scanner-->>VM: scan result
    VM->>UI: update state (proceed or warn)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 ✨
When simulations fail and errors bloom,
I nibble logs instead of sounding doom,
Return a medium flag, let users resume,
Hop forward gently — no crash, just room. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes in KeysignViewModel.kt and Keysign.kt for WaitingForPeer state are unrelated to the linked issue `#4334` about Blockaid error handling.	Remove WaitingForPeer state changes (KeysignViewModel.kt and Keysign.kt) or link them to a separate issue, as they fall outside the scope of the Blockaid crash fix.
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main issue being fixed: a crash in SecurityScanner triggered by Blockaid error responses.
Linked Issues check	✅ Passed	All primary objectives from issue `#4334` are met: error handling in toValidationRiskLevel, catch-all at scanner boundary, and unit tests covering error scenarios.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch agent/4334-crash--securityscanner-throws-on-blockai

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 26 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt`:
- Around line 15-25: The catch block in SecurityScannerExtensions.kt currently
swallows all Throwables (catch (t: Throwable)) and returns a fallback
SecurityScannerResult, which hides cancellation; update the handler to rethrow
CancellationException (or propagate it) before logging/returning the fallback so
coroutine cancellation is preserved—i.e., in the catch for Throwable check if t
is
java.util.concurrent.CancellationException/kotlinx.coroutines.CancellationException
and rethrow it, otherwise log with Timber.e(t, ...) and return the fallback
SecurityScannerResult as before (references: the catch block, Timber.e,
SecurityScannerResult, SecurityRiskLevel).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: edd5c2f5-3666-4cd6-a7ec-b949c9a6e70d

📥 Commits

Reviewing files that changed from the base of the PR and between 168e581 and 70a476a.

📒 Files selected for processing (3)

data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt
data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensions.kt
data/src/test/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensionsTest.kt

Vaulty-bot · 2026-04-30T19:10:17Z

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Vaulty-bot · 2026-04-30T19:15:12Z

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Vaulty-bot · 2026-04-30T19:19:15Z

Added KDoc to the three public extension functions in the changed files (, , ) to address the Docstring Coverage pre-merge warning.

Vaulty-bot · 2026-04-30T19:19:23Z

Added KDoc to the three public extension functions in the changed files (isChainSupported, toSolanaSecurityScannerResult, toSecurityScannerResult) to address the Docstring Coverage pre-merge warning.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

app/src/main/java/com/vultisig/wallet/ui/models/keysign/KeysignViewModel.kt (1)

332-334: ⚡ Quick win

Snapshot missingPeers before writing state.

Line 333 stores the callback list reference directly. If the source list is reused/mutated, UI state can drift unexpectedly. Store an immutable snapshot.

Proposed fix

                             onWaitingForPeers = { missingPeers ->
-                                currentState.value = KeysignState.WaitingForPeer(missingPeers)
+                                currentState.value =
+                                    KeysignState.WaitingForPeer(missingPeers.toList())
                             },

As per coding guidelines "Use StateFlow/MutableStateFlow for UI state in ViewModels with immutable Immutable data classes".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/com/vultisig/wallet/ui/models/keysign/KeysignViewModel.kt`
around lines 332 - 334, The onWaitingForPeers callback in KeysignViewModel
currently assigns the mutable missingPeers list reference directly to
currentState.value via KeysignState.WaitingForPeer, which can lead to UI state
drift if the source list is mutated; fix by taking an immutable snapshot (e.g.,
create a new list/immutable copy) of missingPeers inside the onWaitingForPeers
lambda before creating KeysignState.WaitingForPeer so the state holds its own
stable copy instead of the original mutable reference.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/src/main/java/com/vultisig/wallet/ui/screens/keysign/Keysign.kt`:
- Around line 109-112: The waiting message is built inline in the
KeysignState.WaitingForPeer branch and can produce "Waiting for …" when
state.missingPeers is empty; change it to use a localized string resource (e.g.,
R.string.waiting_for_peers) and pass a safe fallback: if
state.missingPeers.isEmpty() use a generic localized string like
R.string.waiting_for_any_peer, otherwise format the string with
state.missingPeers.joinToString(). Update the KeysignLoadingScreen invocation to
use the localized/fallback text and keep KeepScreenOn() unchanged.

---

Nitpick comments:
In `@app/src/main/java/com/vultisig/wallet/ui/models/keysign/KeysignViewModel.kt`:
- Around line 332-334: The onWaitingForPeers callback in KeysignViewModel
currently assigns the mutable missingPeers list reference directly to
currentState.value via KeysignState.WaitingForPeer, which can lead to UI state
drift if the source list is mutated; fix by taking an immutable snapshot (e.g.,
create a new list/immutable copy) of missingPeers inside the onWaitingForPeers
lambda before creating KeysignState.WaitingForPeer so the state holds its own
stable copy instead of the original mutable reference.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 266136e9-1286-4dfb-aa2f-abb21812b700

📥 Commits

Reviewing files that changed from the base of the PR and between 70a476a and a3f22b6.

📒 Files selected for processing (4)

app/src/main/java/com/vultisig/wallet/ui/models/keysign/KeysignViewModel.kt
app/src/main/java/com/vultisig/wallet/ui/screens/keysign/Keysign.kt
data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt
data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/blockaid/BlockaidExtensions.kt

Vaulty-bot · 2026-05-01T00:30:11Z

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

…4334) - Remove KeysignState.WaitingForPeer and its onWaitingForPeers callback from KeysignViewModel.kt (DklsKeysign.kt was reverted; callback caused a compilation error) and its UI branch from Keysign.kt - Add KDoc to runSecurityScan, private BlockaidExtensions helpers, and the three new unit tests to satisfy the 80% docstring coverage threshold Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Vaulty-bot · 2026-05-01T00:37:36Z

Addressed both pre-merge check failures:

Out of Scope Changes: Removed KeysignState.WaitingForPeer and its onWaitingForPeers callback from KeysignViewModel.kt, and its UI branch from Keysign.kt. These changes were unrelated to issue #4334 and also caused a compilation error — DklsKeysign.kt was already reverted to main and does not have an onWaitingForPeers parameter.

Docstring Coverage: Added KDoc to runSecurityScan, the four private helper functions in BlockaidExtensions.kt (toSolanaValidationRiskLevel, toValidationRiskLevel, toWarningType, toRecommendations), and all three new unit test methods, bringing coverage above the 80% threshold.

johnnyluo

LGTM

Vaulty-bot added 2 commits April 30, 2026 18:56

style: apply ktfmt formatting (#4334)

9fba37a

fix: Crash: SecurityScanner throws on Blockaid 'Simulation Error' res…

70a476a

…ponse, kills main thread (#4334)

Vaulty-bot requested review from aminsato, johnnyluo and rkokhatskyi as code owners April 30, 2026 19:03

coderabbitai Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread data/src/main/kotlin/com/vultisig/wallet/data/securityscanner/SecurityScannerExtensions.kt

fix: rethrow CancellationException in runSecurityScan catch block (#4334

7eda798

) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add KDoc to public SecurityScanner extension functions (#4334)

59dd8c9

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

style: apply ktfmt formatting (#4334)

983cf08

johnnyluo reviewed May 1, 2026

View reviewed changes

Comment thread app/src/main/java/com/vultisig/wallet/data/keygen/DklsKeysign.kt Outdated

revert: restore DklsKeysign.kt to main (unrelated to this PR) (#4334)

a3f22b6

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

coderabbitai Bot reviewed May 1, 2026

View reviewed changes

Comment thread app/src/main/java/com/vultisig/wallet/ui/screens/keysign/Keysign.kt Outdated

Vaulty-bot requested a review from johnnyluo May 1, 2026 00:38

johnnyluo approved these changes May 1, 2026

View reviewed changes

johnnyluo enabled auto-merge May 1, 2026 01:35

johnnyluo added this pull request to the merge queue May 1, 2026

Merged via the queue into main with commit bde1e0b May 1, 2026
2 checks passed

johnnyluo deleted the agent/4334-crash--securityscanner-throws-on-blockai branch May 1, 2026 03:24

Conversation

Vaulty-bot commented Apr 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vaulty-bot commented Apr 30, 2026

Uh oh!

Vaulty-bot commented Apr 30, 2026

Uh oh!

Vaulty-bot commented Apr 30, 2026

Uh oh!

Vaulty-bot commented Apr 30, 2026

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vaulty-bot commented May 1, 2026

Uh oh!

Vaulty-bot commented May 1, 2026

Uh oh!

johnnyluo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Vaulty-bot commented Apr 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 30, 2026 •

edited

Loading