web: report unreadable lines at the PUT
Ordinary parse failures were the silent half of fail-open — stored
verbatim, missing from the index, reported nowhere. BrokenAssumption
lines stay excluded so a line reports once, as an error.

Assisted-by: GLM-5.3 via pi
change qkyusnukmqvrvoptwtmvnyzskmmvqusq
commit 72ce7dc3ae6213d068887a1fd847723b21b4ca4e
author Alpha Chen <alpha@kejadlen.dev>
date
parent muxtzzot
diff --git a/AGENTS.md b/AGENTS.md
index ee6b835..a8b527a 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -174,11 +174,13 @@ not a fixture; edit a `.vcf` to change what the replay serves.
   its bytes regardless, `VCard` and `Contact` answer from the lines
   that read, and the index holds what this server understood. There is
   no repair to make, which is why no caller is asked to look for one.
-- The one exception is `Web#report_broken_assumptions`, and it is not
+- The one exception is the arrival reports at the PUT, and it is not
   handling: a broken assumption is news that something this server was
-  built on is wrong. It lives at the PUT because that is the arrival —
-  a read happens on every page load, and one bad card must not alert
-  once per page view.
+  built on is wrong and reports as an error, and an unreadable line is
+  ordinary bad input and warns (`Web#report_broken_assumptions`,
+  `Web#report_unreadable_lines`). They live at the PUT because that is
+  the arrival — a read happens on every page load, and one bad card
+  must not alert once per page view.
 - `vcard.rb` no longer raises on what it does not recognize, and must
   not start again. It used to render a hand-edited format, where an
   unknown key meant a typo silently losing data; it now reads stored
diff --git a/lib/pro_tacts/vcard/parser.rb b/lib/pro_tacts/vcard/parser.rb
index e1729d8..8500f86 100644
--- a/lib/pro_tacts/vcard/parser.rb
+++ b/lib/pro_tacts/vcard/parser.rb
@@ -104,6 +104,14 @@ module ProTacts
         # know the error taxonomy to tell the two apart.
         #: () -> bool
         def broke_assumption? = error.is_a?(BrokenAssumption)
+
+        # Whether this line failed to read without breaking an
+        # assumption: ordinary bad input, against the news a broken
+        # assumption is. Asked here for the same reason
+        # broke_assumption? is, so no caller has to know the error
+        # taxonomy to tell the two apart.
+        #: () -> bool
+        def unreadable? = !error.nil? && !broke_assumption?
       end
 
       # The content line, RFC 2426 section 4:
diff --git a/lib/pro_tacts/web.rb b/lib/pro_tacts/web.rb
index ad20b54..07bc75d 100644
--- a/lib/pro_tacts/web.rb
+++ b/lib/pro_tacts/web.rb
@@ -485,6 +485,7 @@ module ProTacts
       if_none_match = request.env["HTTP_IF_NONE_MATCH"]
       return plain_412 if if_none_match && if_none_match_failed?(if_none_match, existing)
 
+      report_unreadable_lines(card)
       report_broken_assumptions(card)
 
       stored = store.put(id, vcard)
@@ -507,9 +508,11 @@ module ProTacts
 
     # A card that breaks one of the parser's assumptions is not bad
     # input, it is news that the assumption is wrong and every
-    # simplification resting on it is now suspect. Nothing else would
-    # say so: the card is accepted, stored, and served either way, and
-    # a line that will not read costs it only its index rows.
+    # simplification resting on it is now suspect — an error, where a
+    # line that merely would not read is ordinary bad input and warns
+    # instead (report_unreadable_lines, below). The card is accepted,
+    # stored, and served either way, and a line that will not read
+    # costs it only its index rows.
     #
     # Here rather than in the parser, which is where the break is
     # found: a read happens on every look at a card — the admin index
@@ -529,6 +532,22 @@ module ProTacts
       )
     end
 
+    # The ordinary half of the arrival reports: a line that would not
+    # read is bad input rather than news, so it warns — the level and
+    # the no-card-content line the store's BDAY reports already use.
+    # BrokenAssumption lines are excluded, so a line reports once; why
+    # this lives at the PUT is the comment above, and holds for both.
+    #: (VCard card) -> void
+    def report_unreadable_lines(card)
+      unreadable = card.lines.count { it.unreadable? }
+      return if unreadable.zero?
+
+      Sentry.capture_message(
+        "a submitted card carried #{unreadable} line(s) that would not read — stored verbatim, missing from the index",
+        level: :warning,
+      )
+    end
+
     # A 412 whose body names the CardDAV precondition that failed, in
     # the DAV:error form RFC 4918 section 16 defines; the element names
     # are RFC 6352 section 6.3.2.1's, qualified by the card: namespace
diff --git a/test/pro_tacts/test_web.rb b/test/pro_tacts/test_web.rb
index 16b098a..7bc5624 100644
--- a/test/pro_tacts/test_web.rb
+++ b/test/pro_tacts/test_web.rb
@@ -407,11 +407,11 @@ class WebTest < Minitest::Test
     end
   end
 
-  # The one report left in the app: a card that breaks an assumption
-  # about what macOS Contacts sends is news that the assumption is
-  # wrong, and an arrival is the event worth a message — a read happens
-  # on every page load. A bare CR packs two content lines into one
-  # line's bytes, which is the assumption this card breaks.
+  # A card that breaks an assumption about what macOS Contacts sends
+  # is news that the assumption is wrong, and an arrival is the event
+  # worth a message — a read happens on every page load. A bare CR
+  # packs two content lines into one line's bytes, which is the
+  # assumption this card breaks.
   def test_put_of_a_card_breaking_a_parser_assumption_is_reported
     packed = card("new", "New").sub("FN:New\r\n", "FN:New\rNOTE:b\r\n")
 
@@ -425,6 +425,37 @@ class WebTest < Minitest::Test
     end
   end
 
+  # The ordinary half of the arrival reports: a line that will not
+  # read is bad input, not news, so it warns — once, at the arrival,
+  # and never on the reads that serve the card afterwards.
+  def test_put_of_a_card_with_an_unreadable_line_warns
+    unreadable = card("new", "New").sub("FN:New\r\n", "FN:New\r\nTEL;HOME:+1-555-1234\r\n")
+
+    with_contacts({}) do
+      put_request "new", unreadable, "CONTENT_TYPE" => VCARD
+      messages = sentry_messages
+
+      assert_equal 201, last_response.status
+      assert_equal 1, messages.length
+      assert_match "carried 1 line", messages.fetch(0)
+    end
+  end
+
+  # A card can carry both, and a line reports once: the unreadable
+  # line warns on its own, the broken assumption errors on its own.
+  def test_put_of_a_card_with_both_reports_each_once
+    both = card("new", "New").sub("FN:New\r\n", "FN:New\rX\r\nTEL;HOME:+1-555-1234\r\n")
+
+    with_contacts({}) do
+      put_request "new", both, "CONTENT_TYPE" => VCARD
+      messages = sentry_messages
+
+      assert_equal 2, messages.length
+      assert_match "carried 1 line", messages.fetch(0)
+      assert_match "broke 1 parser assumption", messages.fetch(1)
+    end
+  end
+
   # Nothing else reports: an ordinary card arrives in silence.
   def test_put_of_an_ordinary_card_is_quiet
     with_contacts({}) do
diff --git a/test/pro_tacts/vcard/test_parser.rb b/test/pro_tacts/vcard/test_parser.rb
index 03bcc8f..5658828 100644
--- a/test/pro_tacts/vcard/test_parser.rb
+++ b/test/pro_tacts/vcard/test_parser.rb
@@ -208,6 +208,13 @@ class VCardParserTest < Minitest::Test
     assert lines("FN:A\rNOTE:n\r\n").fetch(0).broke_assumption?
   end
 
+  # unreadable? is the other half of that split: the ordinary bad line
+  # reads as unreadable, the blank line and the break do not.
+  def test_only_a_plain_parse_failure_reads_as_unreadable
+    assert_equal [false, false, true], lines("FN:A\r\n\r\nBDAY;=;:\r\n").map { it.unreadable? }
+    assert_equal false, lines("FN:A\rNOTE:n\r\n").fetch(0).unreadable?
+  end
+
   def test_a_line_that_will_not_read_carries_its_error_and_keeps_its_bytes
     line = ProTacts::VCard::Parser.lines("BDAY;=;:\r\n").fetch(0)