Let ContactFields.from raise on an unparseable card
Review feedback on the PR: swallowing VCard::ParseError and returning
an empty ContactFields hid a case that shouldn't be silent — every PUT
already checks VCard.card? before a card is stored, so one reaching
here unparseable is a bug or a corrupt row, not the ordinary case.
Letting it raise sends it through Sentry::Rack::CaptureExceptions,
already wrapping every request, instead of rendering a blank card with
no trace of what went wrong.
Also adds the general rule to AGENTS.md: don't rescue a failure just
because it might happen, only ones with a real, anticipated fallback
(format_birthday's ArgumentError rescue is the kept example).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SYiigu3aFNaiTP9SyKW9mX
diff --git a/AGENTS.md b/AGENTS.md
index 23b973a..3609ad6 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -85,6 +85,16 @@ not a fixture; edit a `.vcf` to change what the replay serves.
- Anything added to a request body must be assumed to reach Sentry. Card
content is redacted by `ProTacts::SentryScrubber`; a new kind of sensitive
field would need its own rule there.
+- Don't hide errors. A `rescue` that swallows an exception and returns a
+ fallback value reads as defensive but is actually the opposite: it turns
+ a bug or corrupt data into a silently wrong screen instead of a loud one.
+ Every request already runs under `Sentry::Rack::CaptureExceptions`, so an
+ unexpected failure that's allowed to raise is reported and visible —
+ rescuing it locally "to be safe" makes it neither. Rescue only what's an
+ ordinary, anticipated case with a real fallback to show (e.g. a birthday
+ whose stored value is well-shaped but calendar-nonsense — see
+ `ContactFields#format_birthday`), never "this might fail, better catch
+ it."
- `config.ru` must stay ASCII-only; a test enforces it, because boot crashes
under a C locale otherwise. Watch for em dashes in comments.
- `RUBYOPT=--enable-frozen-string-literal` is set in `.ramekin/config.kdl`.
diff --git a/lib/pro_tacts/admin/contact_fields.rb b/lib/pro_tacts/admin/contact_fields.rb
index bbf0b29..45763a4 100644
--- a/lib/pro_tacts/admin/contact_fields.rb
+++ b/lib/pro_tacts/admin/contact_fields.rb
@@ -18,12 +18,18 @@ module ProTacts
attr_reader :name, :phones, :emails, :addresses, :birthday, :notes
+ # Lets VCard::ParseError raise rather than showing an empty card:
+ # a stored card that fails to parse here is not the ordinary case
+ # #format_birthday's rescue handles (a well-shaped but nonsense
+ # value that round-trips on purpose) — every PUT already checks
+ # VCard.card? before the store accepts a card, so one reaching
+ # this unparseable is a bug or a corrupt row, and Sentry (already
+ # wired into every request, see ProTacts::Web) is where that
+ # should surface, not a silently blank screen.
#: (Contact contact) -> ContactFields
def self.from(contact)
properties = VCard::Parser.parse(contact.vcard)
new(properties)
- rescue VCard::ParseError
- new([])
end
#: (Array[VCard::Property] properties) -> void
diff --git a/test/pro_tacts/admin/test_contact_fields.rb b/test/pro_tacts/admin/test_contact_fields.rb
index 3c67454..f5e44d9 100644
--- a/test/pro_tacts/admin/test_contact_fields.rb
+++ b/test/pro_tacts/admin/test_contact_fields.rb
@@ -57,13 +57,13 @@ class ContactFieldsTest < Minitest::Test
assert_nil fields.notes
end
- # Fail open, the same contract Store#rebuild_index has: a card the
- # parser cannot read still shows something rather than raising.
- def test_a_card_that_will_not_parse_shows_no_fields
- fields = fields("not a vCard at all")
-
- assert_nil fields.name
- assert_empty fields.phones
+ # Unlike Store#rebuild_index, which fails a card open because a
+ # write already guarantees every stored card parses (VCard.card? is
+ # checked before a PUT is accepted) — a card that reaches here and
+ # still won't parse is a bug or a corrupt row, not the ordinary case,
+ # and it should raise into Sentry rather than render a blank card.
+ def test_a_card_that_will_not_parse_raises
+ assert_raises(ProTacts::VCard::ParseError) { fields("not a vCard at all") }
end
# RFC 2426 section 3.1.5 lets BDAY carry a date-time, and a client is