From 1988d350242b47aa52aa07904c495e7e2c0eba82 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Thu, 20 Aug 2026 22:11:17 +0200 Subject: fix: audit findings — parser strictness, name ambiguity, and errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by auditing the shipped program rather than the diff. The parser accepted OCaml integer-literal syntax, so "Luke 1_1:5" read as chapter ELEVEN and "+5" as 5 -- a typo silently becoming a different chapter, reachable through any user overlay. Numbers are now plain digits and positive, and a descending range is rejected: 1:20-10 is always a transcription error. No shipped citation changed. FOUR PAIRS OF DIFFERENT BOOKS SHARED A FULL TITLE. 1 and 2 Corinthians both rendered "Epistola ad Corinthios", as did Thessalonians, Timothy and Peter -- 108 citations in 2027 alone that a reader cannot resolve to a book. This is the Kings defect fixed earlier and not generalised. The titles now carry their volume numeral, marked CONSTRUCTED, and a test asserts no two books share a name -- while allowing the case where two ids ARE the same book under different numbering, which a tradition relates. Spec section 8.5 is now delivered rather than merely recorded. Shipped styles did not re-parse their own output: 32 of 52 Latin abbreviations and 49 of 52 full titles failed, so a citation copied from colitur's own output into an overlay was passed through untouched and printed in the wrong language, silently. Every shipped name is registered as a spelling and split_book learned multi-word titles by longest-token match. Now 0 of 52 fail beyond the same-book aliases. Overlay errors were written for a compiler author: they named an OCaml source file the reader does not have and buried the useful token. The existing five-path rewriter is replaced by a generic one, applied to every load path rather than one, so "rank: is not one of the allowed values (at Class9)" replaces the raw Of_sexp_error dump. Also: the new-overlay scaffold documented citations and layer without showing them, and its comment implied the wrong nesting -- the single easiest thing to get wrong; error messages echoed whole file lines, copying an unrelated file's contents into stderr when a flag pointed at one; and config --show validated partway down its table, exiting 2 after writing five rows to stdout. --- test/cli.t | 10 ++-- test/test_citation.ml | 19 ++++++++ test/test_lang_coverage.ml | 118 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 5 deletions(-) (limited to 'test') diff --git a/test/cli.t b/test/cli.t index b44a256..fe18800 100644 --- a/test/cli.t +++ b/test/cli.t @@ -1374,12 +1374,12 @@ fallback: `Colitur_citation.Render.with_book` takes a closed variant, not an arbitrary string, so anything other than "full"/"abbr" could never mean anything downstream. +STDOUT STAYS EMPTY. The check used to happen partway down the table, so this +exited 2 having already written five rows -- a caller redirecting stdout to a +file got a truncated but plausible-looking report beside a non-zero status. +Validation now runs before anything is printed. + $ XDG_CONFIG_HOME=xdg-sigla colitur config --show --sigla-book bogus - config file: xdg-sigla/colitur/config.ini (not found) - lang la (default) - template (none) (default) - format (none) (default) - sigla_style la (default) colitur: unknown --sigla-book "bogus" (want "full" or "abbr") [2] diff --git a/test/test_citation.ml b/test/test_citation.ml index 133553b..dae4610 100644 --- a/test/test_citation.ml +++ b/test/test_citation.ml @@ -283,6 +283,11 @@ let parses input expected () = | Error e -> Alcotest.failf "%s did not parse: %s" input e | Ok t -> Alcotest.(check s) input expected (show t) +let rejects input () = + match P.parse input with + | Ok t -> Alcotest.failf "%s should not parse, got %s" input (show t) + | Error _ -> () + let test_rejects_unknown_book () = Alcotest.(check bool) "error" true (Result.is_error (P.parse "Nonesuch 1:1")) @@ -321,6 +326,20 @@ let parse_suite = ("ordinal 3 parses", `Quick, parses "3 Kings 17:8-16" "kings_3|17:8-16"); ("ordinal 3 dotted", `Quick, parses "3 Kgs. 19:3-8" "kings_3|19:3-8"); ("ordinal 4 parses", `Quick, parses "4 Kings 5:1-15" "kings_4|5:1-15"); + (* A citation number must be plain digits and positive. OCaml's + [int_of_string] also accepts its own literal syntax, so "1_1" parsed + as chapter ELEVEN and "+5" as 5 -- a typo silently becoming a + DIFFERENT chapter, which no amount of downstream care can catch. + Reachable through a user overlay, which supplies arbitrary strings. *) + ("rejects underscore in chapter", `Quick, rejects "Luke 1_1:5"); + ("rejects underscore in verse", `Quick, rejects "Luke 1:5_0"); + ("rejects plus in chapter", `Quick, rejects "Luke +1:5"); + ("rejects plus in verse", `Quick, rejects "Luke 1:+5"); + ("rejects chapter zero", `Quick, rejects "Luke 0:5"); + ("rejects verse zero", `Quick, rejects "Luke 1:0"); + (* A descending range is always a transcription error. *) + ("rejects descending range", `Quick, rejects "Luke 1:20-10"); + ("allows a single-verse range", `Quick, parses "Luke 1:5-5" "luke|1:5-5"); ("unknown book", `Quick, test_rejects_unknown_book); ("garbage", `Quick, test_rejects_garbage) ] diff --git a/test/test_lang_coverage.ml b/test/test_lang_coverage.ml index 0271a59..d41f7e7 100644 --- a/test/test_lang_coverage.ml +++ b/test/test_lang_coverage.ml @@ -11,6 +11,14 @@ let la () = | Ok t -> t | Error e -> Alcotest.failf "lang/la.ini: %s" e +(* en.ini resolved THROUGH la.ini, the way a real run resolves it: en + declares `fallback = la`, so a key it omits must reach the Latin table + rather than degrade to the raw key. *) +let en () = + match L.of_string (read "../lang/en.ini") with + | Ok t -> L.with_fallback t (la ()) + | Error e -> Alcotest.failf "lang/en.ini: %s" e + (* Every slug the engine can emit -- temporal ("^ef-") AND sanctoral (a fixed saint's day) alike -- must have a Latin name. THIS IS THE TEST THAT WOULD HAVE CAUGHT THE ORIGINAL DEFECT -- a booklet printed "ef-septuagesima- @@ -145,6 +153,112 @@ let test_no_book_name_is_an_internal_id () = Alcotest.(check (list string)) "no book name is its own internal id" [] leaked +(* Two DIFFERENT books must not share a name. Found by audit: `1 Cor` and + `2 Cor` both rendered as "Epistola ad Corinthios" under --sigla-book full, + and so did Thessalonians, Timothy and Peter -- 108 citations in 2027 alone + that a reader cannot resolve to a book. The same defect had already been + fixed for the two Books of Kings and simply not generalised. + + Sharing a name is CORRECT, though, when the two ids are the same physical + book under different numbering -- `osee`/`hosea`, `jonas`/`jonah`, + `apocalypse`/`revelation`. A tradition maps one onto the other, so the + rule is exact: a shared name is a defect UNLESS some tradition relates the + two ids. *) +let test_no_two_books_share_a_name () = + let la = la () in + let traditions = + let text = + let ic = open_in_bin "../lang/traditions.ini" in + let s = really_input_string ic (in_channel_length ic) in + close_in ic; s + in + match Colitur_kernel.Overlay_ini.parse_sections text with + | Ok ss -> + List.concat_map (fun (sc : Colitur_kernel.Overlay_ini.section) -> + sc.Colitur_kernel.Overlay_ini.fields) ss + | Error e -> Alcotest.failf "traditions.ini: %s" e + in + let related a b = + List.exists (fun (x, y) -> (x = a && y = b) || (x = b && y = a)) traditions + in + let offenders = ref [] in + List.iter + (fun form -> + let seen = Hashtbl.create 64 in + List.iter + (fun id -> + let n = Colitur_citation.Book.to_string id in + let key = n ^ "." ^ form in + let v = L.bible la key in + if v <> key then + match Hashtbl.find_opt seen v with + | Some other when not (related n other) -> + offenders := Printf.sprintf "%s: %s and %s" v other n :: !offenders + | _ -> Hashtbl.replace seen v n) + Colitur_citation.Book.all) + [ "full"; "abbr" ]; + Alcotest.(check (list string)) "no two different books share a name" [] + (List.sort compare !offenders) + +(* SPEC SECTION 8.5: each shipped style must parse its own rendered output. + It did not. Measured at the time: 32 of 52 Latin abbreviations and 49 of 52 + Latin full titles failed to re-parse, so a user who copied a citation out + of `colitur readings` into an overlay handed the parser a string it could + not read; [Sigla.format] passed it through untouched and, say, an English + full-name booklet printed a Latin abbreviation with no warning. Closed by + registering every shipped name as a spelling and by teaching [split_book] + multi-word titles. + + A name may resolve to a DIFFERENT id than the one it was rendered from, + but only when the two are the same physical book under different + numbering: "Sir" is registered to [ecclesiasticus] and the modern id + [sirach] maps onto it. The parser has no tradition context, so it returns + the Vulgate id, and that is right rather than tolerated. *) +let test_shipped_styles_round_trip () = + let traditions = + let text = + let ic = open_in_bin "../lang/traditions.ini" in + let s = really_input_string ic (in_channel_length ic) in + close_in ic; s + in + match Colitur_kernel.Overlay_ini.parse_sections text with + | Ok ss -> + List.concat_map + (fun (sc : Colitur_kernel.Overlay_ini.section) -> + sc.Colitur_kernel.Overlay_ini.fields) + ss + | Error e -> Alcotest.failf "traditions.ini: %s" e + in + let related a b = + a = b || List.exists (fun (x, y) -> (x = a && y = b) || (x = b && y = a)) traditions + in + let check_file label t = + List.concat_map + (fun form -> + List.filter_map + (fun id -> + let n = Colitur_citation.Book.to_string id in + let name = L.bible t (n ^ "." ^ form) in + if name = n ^ "." ^ form then None + else + match Colitur_citation.Parse.parse (name ^ " 5:12-14") with + | Ok r + when related + (Colitur_citation.Book.to_string r.Colitur_citation.Parse.book) + n -> + None + | Ok r -> + Some + (Printf.sprintf "%s %s/%s -> %s" label name form + (Colitur_citation.Book.to_string r.Colitur_citation.Parse.book)) + | Error e -> Some (Printf.sprintf "%s %s/%s: %s" label name form e)) + Colitur_citation.Book.all) + [ "full"; "abbr" ] + in + let bad = check_file "la" (la ()) @ check_file "en" (en ()) in + Alcotest.(check (list string)) "every shipped book name re-parses" [] + (List.sort compare bad) + (* lang/en.ini is DELIBERATELY partial (see its own header note): it declares [meta] fallback = la, so a slug it does not carry itself should still resolve through the chain to la.ini's name rather than degrade to the bare @@ -189,4 +303,8 @@ let suite = Alcotest.test_case "every book named, both forms" `Quick test_every_book_named; Alcotest.test_case "no book name is an internal id" `Quick test_no_book_name_is_an_internal_id; + Alcotest.test_case "no two books share a name" `Quick + test_no_two_books_share_a_name; + Alcotest.test_case "shipped styles round-trip" `Quick + test_shipped_styles_round_trip; Alcotest.test_case "en.ini falls back to Latin" `Quick test_en_falls_back_to_latin ] ) -- cgit v1.3 From 671c4264707ec8c81845059746632b00f2481d88 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Thu, 20 Aug 2026 22:33:07 +0200 Subject: fix(render): a month answers to both spellings of its own name A week object carries month_name/month_num/month_abbr, because at that level a bare `name` would be ambiguous. An author who learned those names inside {{#weeks}} reaches for them one level up inside {{#months}} too -- where they resolved to nothing, and an unknown key renders as the empty string by design, so the result was a silently blank month heading rather than any error. Found by writing a template from scratch rather than copying a shipped one; every shipped template sidesteps it, which is why nothing caught it. The aliases are additive, so no existing template or golden changes. colitur-templates(5) now documents both spellings and says why they exist. --- lib/render/view.ml | 14 +++++++++++++- man/colitur-templates.5 | 34 ++++++++++++++++++++++++++++++---- test/test_view.ml | 19 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) (limited to 'test') diff --git a/lib/render/view.ml b/lib/render/view.ml index 0691ffb..67dfa70 100644 --- a/lib/render/view.ml +++ b/lib/render/view.ml @@ -267,8 +267,20 @@ let of_days ~lang ~sigla ~vocab ~rite ~year days = let month_name = Lang.month lang m in let month_abbr = Lang.month_abbr lang m in T.Obj - [ ("num", str month_num); + [ (* A month carries its own name under BOTH spellings. The week + objects nested below expose [month_name]/[month_num]/ + [month_abbr], and a template author who learned those names + there naturally reaches for them one level up -- where, before + this, they resolved to nothing and rendered as the empty + string, because an unknown key is silently empty by design. + A month is the only scope where the short forms are + unambiguous, so they stay as the primary names and the + qualified forms are aliases. *) + ("num", str month_num); ("name", str month_name); + ("month_num", str month_num); + ("month_name", str month_name); + ("month_abbr", str month_abbr); ("days", T.List day_values); ("weeks", T.List (weeks_of_month ~first_dow ~month_num ~month_name ~month_abbr day_values)) ]) in diff --git a/man/colitur-templates.5 b/man/colitur-templates.5 index e04560b..e668b55 100644 --- a/man/colitur-templates.5 +++ b/man/colitur-templates.5 @@ -566,13 +566,21 @@ template iterates over directly, without going through .BR months . .SS month .TP -.B num -The month number, 1 through 12, as a string. +.BR num ", " month_num +The month number, 1 through 12, as a string. The two spellings are the same +value \(em see the note at the end of this subsection. .TP -.B name +.BR name ", " month_name The month's own resolved display name in the active language (e.g. .RB \(lq Ianuarius \(rq ), -a plain string. +a plain string. Both spellings are the same value. +.TP +.B month_abbr +The month's abbreviated name in the active language (e.g. +.RB \(lq Ian \(rq ), +from the language file's own +.B [month_abbr] +section. .TP .B days This month's own @@ -587,6 +595,24 @@ last rows padded with blank cells (see .B day \(-> in_month below) so every row has seven entries regardless of which weekday the month starts or ends on. +.PP +.B "Why two spellings." +A +.B week +object (below) carries +.BR month_name ", " month_num " and " month_abbr, +because at that level a bare +.B name +would be ambiguous. An author who learned those names inside +.B {{#weeks}} +naturally reaches for them one level up, inside +.BR {{#months}} . +Before colitur 0.9 they resolved to nothing there, and because an unknown +key renders as the EMPTY STRING by design (see +.B SCOPE AND LOOKUP +above), the result was a silently blank month heading rather than any +error. A month now answers to both. Prefer the short forms in new +templates: a month is the only scope where they are unambiguous. .SS week .TP .B num diff --git a/test/test_view.ml b/test/test_view.ml index b2d302e..a542419 100644 --- a/test/test_view.ml +++ b/test/test_view.ml @@ -292,6 +292,25 @@ let test_vocabularies_localise () = Alcotest.(check string) "rite_name" "Missale Romanum, editio typica 1962" (as_str (get [ "rite_name" ] v)) +(* A month answers to BOTH spellings of its own name. The week objects nested + inside a month expose [month_name]/[month_num]/[month_abbr], so an author + who learned those names there reaches for them one level up too -- and an + unknown key renders as the EMPTY STRING by design, so the mistake produced + a silently blank heading rather than any error. Found by writing a template + from scratch; the shipped ones all sidestep it. *) +let test_month_answers_to_both_spellings () = + let v = view_named 2027 in + let jan = List.hd (as_list (get [ "months" ] v)) in + let same a b = + Alcotest.(check string) (a ^ " = " ^ b) (as_str (get [ a ] jan)) + (as_str (get [ b ] jan)) + in + same "name" "month_name"; + same "num" "month_num"; + Alcotest.(check string) "month_name" "Ianuarius" (as_str (get [ "month_name" ] jan)); + Alcotest.(check bool) "month_abbr is present and non-empty" true + (as_str (get [ "month_abbr" ] jan) <> "") + let suite = ( "View", [ Alcotest.test_case "year shape" `Quick test_year_shape; -- cgit v1.3