From 25ebc53914780a1df38a1f5e49ddca07a3ad04b6 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Fri, 30 Oct 2026 20:49:11 -0400 Subject: [PATCH] fix(bugsnag): group validation failures by field and rule Keep the failed rule on Violation, report failures on client-filled fields (addresses, address, user_id) at WARNING, and set the Bugsnag grouping hash for ValidationException to the field path without list indexes plus the rule. --- .../internal/startup/BugsnagErrorReporter.kt | 11 +++ .../startup/BugsnagErrorReporterTest.kt | 43 ++++++++++++ .../opencode/utils/ThrowableExtensionsTest.kt | 67 ++++++++++++++++++- .../model/core/errors/ValidationException.kt | 40 ++++++++++- .../com/getcode/opencode/utils/Throwable.kt | 2 +- 5 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 apps/flipcash/app/src/test/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporterTest.kt diff --git a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporter.kt b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporter.kt index e45e57f2ab..6025c1fbb3 100644 --- a/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporter.kt +++ b/apps/flipcash/app/src/main/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporter.kt @@ -2,6 +2,7 @@ package com.flipcash.app.internal.startup import com.bugsnag.android.Bugsnag import com.bugsnag.android.Severity +import com.getcode.opencode.model.core.errors.ValidationException import com.getcode.utils.ErrorReporter class BugsnagErrorReporter : ErrorReporter { @@ -11,7 +12,17 @@ class BugsnagErrorReporter : ErrorReporter { // WARNING = needs active attention (also drives the Slack filter); // INFO = recorded for reference only, excluded from Slack. event.severity = if (isNotifiable) Severity.WARNING else Severity.INFO + groupingHashFor(error, cause)?.let { event.groupingHash = it } true } } + + internal companion object { + /** + * Validation failures all share one `CodeServerError.` frame, so Bugsnag would fold + * every field into one group. Group by field path and rule instead. + */ + fun groupingHashFor(error: Throwable, cause: Throwable): String? = + ((error as? ValidationException) ?: (cause as? ValidationException))?.groupingHash + } } diff --git a/apps/flipcash/app/src/test/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporterTest.kt b/apps/flipcash/app/src/test/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporterTest.kt new file mode 100644 index 0000000000..4502da26f0 --- /dev/null +++ b/apps/flipcash/app/src/test/kotlin/com/flipcash/app/internal/startup/BugsnagErrorReporterTest.kt @@ -0,0 +1,43 @@ +package com.flipcash.app.internal.startup + +import com.getcode.opencode.model.core.errors.ValidationException +import com.getcode.opencode.model.core.errors.Violation +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +class BugsnagErrorReporterTest { + + private fun validation(field: String, rule: String) = + ValidationException(listOf(Violation(field = field, rule = rule, message = "m"))) + + @Test + fun `grouping hash comes from the validation error or its cause`() { + val error = validation("messages[3].content", "string.max_len") + val wrapped = RuntimeException("wrapped", error) + + assertEquals(error.groupingHash, BugsnagErrorReporter.groupingHashFor(error, error)) + assertEquals(error.groupingHash, BugsnagErrorReporter.groupingHashFor(wrapped, error)) + } + + @Test + fun `messages indexes with the same rule share a hash and other rules do not`() { + val a = BugsnagErrorReporter.groupingHashFor( + validation("messages[3].content", "string.max_len"), RuntimeException() + ) + val b = BugsnagErrorReporter.groupingHashFor( + validation("messages[7].content", "string.max_len"), RuntimeException() + ) + val c = BugsnagErrorReporter.groupingHashFor( + validation("messages[7].content", "string.min_len"), RuntimeException() + ) + + assertEquals(a, b) + assert(a != c) + } + + @Test + fun `other errors keep Bugsnag default grouping`() { + assertNull(BugsnagErrorReporter.groupingHashFor(RuntimeException(), RuntimeException())) + } +} diff --git a/services/opencode/src/androidHostTest/kotlin/com/getcode/opencode/utils/ThrowableExtensionsTest.kt b/services/opencode/src/androidHostTest/kotlin/com/getcode/opencode/utils/ThrowableExtensionsTest.kt index b42df5b24b..d37d60deeb 100644 --- a/services/opencode/src/androidHostTest/kotlin/com/getcode/opencode/utils/ThrowableExtensionsTest.kt +++ b/services/opencode/src/androidHostTest/kotlin/com/getcode/opencode/utils/ThrowableExtensionsTest.kt @@ -7,6 +7,9 @@ import dev.bmcreations.protovalidate.FieldViolation import dev.bmcreations.protovalidate.ProtoValidationException import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotEquals +import kotlin.test.assertTrue import kotlin.test.assertIs class ThrowableExtensionsTest { @@ -53,8 +56,70 @@ class ThrowableExtensionsTest { assertIs(result) assertEquals( - listOf(Violation(field = "phone.value", message = "must match pattern")), + listOf(Violation(field = "phone.value", rule = "string.pattern", message = "must match pattern")), result.violations ) } + + @Test + fun `rule and field path survive into the exception`() { + val proto = ProtoValidationException( + listOf(FieldViolation(field = "user_id.value", rule = "bytes.min_len", message = "must be at least 1 byte")) + ) + + val result: CodeServerError = proto.toValidationOrElse { FallbackError(it) } + + assertIs(result) + assertEquals("bytes.min_len", result.violations.single().rule) + assertEquals("user_id.value", result.violations.single().field) + } + + private fun validation(field: String, rule: String, message: String = "m") = + ValidationException(listOf(Violation(field = field, rule = rule, message = message))) + + @Test + fun `same field and rule at different list indexes share a grouping hash`() { + val a = validation("message_ids.message_ids[3].value", "int64.gte") + val b = validation("message_ids.message_ids[7].value", "int64.gte") + + assertEquals(a.groupingHash, b.groupingHash) + } + + @Test + fun `grouping hash ignores the message so a changed limit does not split the group`() { + val a = validation("username.value", "string.pattern", "must match {2,15}") + val b = validation("username.value", "string.pattern", "must match {2,30}") + + assertEquals(a.groupingHash, b.groupingHash) + } + + @Test + fun `different rules on one field have different grouping hashes`() { + val tooShort = validation("display_name", "string.min_len") + val tooLong = validation("display_name", "string.max_len") + + assertNotEquals(tooShort.groupingHash, tooLong.groupingHash) + } + + @Test + fun `different fields have different grouping hashes`() { + assertNotEquals( + validation("addresses[0].value", "bytes.min_len").groupingHash, + validation("address.value", "bytes.min_len").groupingHash, + ) + } + + @Test + fun `client-filled fields are notifiable`() { + listOf("addresses[0].value", "address.value", "user_id.value").forEach { + assertTrue(validation(it, "bytes.min_len").isNotifiable, it) + } + } + + @Test + fun `user-input fields and username keep their current treatment`() { + listOf("group.title", "display_name", "name", "username.value", "message_ids.message_ids[1].value").forEach { + assertFalse(validation(it, "string.max_len").isNotifiable, it) + } + } } diff --git a/services/opencode/src/commonMain/kotlin/com/getcode/opencode/model/core/errors/ValidationException.kt b/services/opencode/src/commonMain/kotlin/com/getcode/opencode/model/core/errors/ValidationException.kt index 3405a43d82..96e3452952 100644 --- a/services/opencode/src/commonMain/kotlin/com/getcode/opencode/model/core/errors/ValidationException.kt +++ b/services/opencode/src/commonMain/kotlin/com/getcode/opencode/model/core/errors/ValidationException.kt @@ -1,6 +1,7 @@ package com.getcode.opencode.model.core.errors import com.getcode.utils.CodeServerError +import com.getcode.utils.ConditionallyNotifiable class ValidationException( val violations: List, @@ -8,9 +9,44 @@ class ValidationException( ) : CodeServerError( message = field?.let { "Validation failed for $it" } ?: "Request validation failed: ${violations.joinToString { "${it.field}: ${it.message}" }}" -) +), ConditionallyNotifiable { + + /** + * Reported when a violation sits on a field the client fills itself. Those failures are client + * bugs (or contract pin drift). Failures on user-typed fields stay out: the UI owns those + * checks, and `username` is left as it was. + */ + override val isNotifiable: Boolean + get() = violations.any { it.isClientFilled } + + /** + * Stable Bugsnag grouping key: the sorted set of `:`. The + * message is left out because it embeds the contract limit, which would split the group each + * time a limit changes. + */ + val groupingHash: String + get() = violations + .map { "${it.fieldPath}:${it.rule}" } + .distinct() + .sorted() + .joinToString(",") +} data class Violation( val field: String, + val rule: String, val message: String, -) \ No newline at end of file +) { + /** [field] with list indexes (`[N]`) removed. */ + val fieldPath: String + get() = this.field.replace(ListIndex, "") + + /** True when the request builder, not the user, supplies the value. */ + val isClientFilled: Boolean + get() = fieldPath.substringBefore('.') in ClientFilledFields + + private companion object { + val ListIndex = Regex("""\[\d+]""") + val ClientFilledFields = setOf("addresses", "address", "user_id") + } +} diff --git a/services/opencode/src/commonMain/kotlin/com/getcode/opencode/utils/Throwable.kt b/services/opencode/src/commonMain/kotlin/com/getcode/opencode/utils/Throwable.kt index 6191d1c769..c0007a16e3 100644 --- a/services/opencode/src/commonMain/kotlin/com/getcode/opencode/utils/Throwable.kt +++ b/services/opencode/src/commonMain/kotlin/com/getcode/opencode/utils/Throwable.kt @@ -9,7 +9,7 @@ import dev.bmcreations.protovalidate.ProtoValidationException fun Throwable.toValidationOrElse(fallback: (Throwable) -> E): E = when (this) { is ProtoValidationException -> ValidationException( - violations.map { Violation(field = it.field, message = it.message) } + violations.map { Violation(field = it.field, rule = it.rule, message = it.message) } ) as E else -> fallback(this) }