feat(textnorm): MeaningPreset + HygienePreset (meaning-preserving normalization) - #34
Conversation
…-separator aware)
MeaningPreset: fingerprints that must preserve meaning (s22+ != s22, 4.5 != 45, 1,000 != 1 000; no token merging/dedup; Latin-only diacritic stripping). PreserveMeaningPunct now also keeps combining marks (Mn/Mc/Me) so Devanagari matras / Arabic harakat survive the stage standalone. HygienePreset: payload-grade byte hygiene, case/punct/diacritics untouched.
|
👀 Starting code review... |
|
🔄 Cloning repository... |
|
🤖 Analyzing code with opencode... |
|
✅ Review complete! |
![git-code-reviewer-v1[bot]](https://avatars.githubusercontent.com/u/31842468?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-thought-out addition to the textnorm package. The two new presets (MeaningPreset, HygienePreset) and supporting stages (PreserveMeaningPunct, DecodeHTMLEntities, RemoveFormatChars, NormalizeUnicodeLatin) follow existing patterns, are thoroughly unit-tested, and include fuzz coverage. No blocking correctness, security, or performance issues were found.
Non-blocking findings
hygiene.goandmeaning.gocontain package-level doc comments (// Package textnorm …) in addition todoc.go. Go tooling shows whichever package comment it encounters first; having multiple package comments is non-standard and can cause the wrong synopsis to surface ingodoc/pkgsitedepending on file ordering. Convert them to plain file-level comments (drop thePackage textnormprefix) or fold the text intodoc.go.fuzz_test.gogained a fuzz target forMeaningPresetbut not forHygienePreset. AddingFuzzHygienePresetwould keep parity with the other presets and guard idempotency regressions.
| @@ -0,0 +1,34 @@ | |||
| // Package textnorm — hygiene.go: byte-hygiene stages for text that will be | |||
There was a problem hiding this comment.
This package comment duplicates (and could shadow) the one in doc.go. Convert to a plain file-level comment or merge into doc.go.
| // Package textnorm — hygiene.go: byte-hygiene stages for text that will be | |
| // hygiene.go: byte-hygiene stages for text that will be | |
| // sent to an external model or stored verbatim. These stages never touch | |
| // case, punctuation, or diacritics. | |
| package textnorm |
| @@ -0,0 +1,62 @@ | |||
| // Package textnorm — meaning.go implements meaning-preserving punctuation | |||
There was a problem hiding this comment.
This package comment duplicates (and could shadow) the one in doc.go. Convert to a plain file-level comment or merge into doc.go.
| // Package textnorm — meaning.go implements meaning-preserving punctuation | |
| // meaning.go implements meaning-preserving punctuation filtering: instead | |
| // of deleting non-alphanumeric runes (which merges tokens, e.g. "2.5"→"25"), | |
| // every rejected rune becomes a single space, and the small set of | |
| // punctuation that carries meaning in product/search text is kept: | |
| // | |
| // - '.' and ',' when BOTH neighbours are digits ("4.5", "1,000"). | |
| // Separators are NOT unified: "1,000" and "1.000" stay distinct because | |
| // the same string means different numbers in different locales. | |
| // - '+' when it terminates an alphanumeric token ("s22+", "c++"). | |
| // - '%' immediately after a digit ("100%"). | |
| package textnorm |
| t.Fatalf("uncollapsed whitespace in %q", out) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider adding an idempotency fuzz target for HygienePreset to match the coverage the other presets already have.
| } | |
| func FuzzHygienePreset(f *testing.F) { | |
| for _, seed := range []string{ | |
| "Café \u200bCrème!", | |
| "A & B", | |
| " spaced\n\nout\t ", | |
| } { | |
| f.Add(seed) | |
| } | |
| f.Fuzz(func(t *testing.T, in string) { | |
| pipe := HygienePreset() | |
| out1, err := pipe.Run(in) | |
| if err != nil { | |
| t.Fatalf("Run() error = %v", err) | |
| } | |
| out2, err := pipe.Run(out1) | |
| if err != nil { | |
| t.Fatalf("second Run() error = %v", err) | |
| } | |
| if out1 != out2 { | |
| t.Fatalf("HygienePreset not idempotent: %q != %q", out1, out2) | |
| } | |
| }) | |
| } |
Addresses PR #34 review: meaning.go/hygiene.go no longer carry competing package comments (detail moved to stage docs); HygienePreset gets a fuzz target checking UTF-8 validity, no Cf/Cc runes, collapsed whitespace — deliberately no idempotency invariant since entity decoding is not idempotent.
|
👀 Starting code review... |
|
🔄 Cloning repository... |
|
🤖 Analyzing code with opencode... |
|
✅ Review complete! |
There was a problem hiding this comment.
All prior review feedback has been addressed in commit 4bb16f0: the duplicate package comments in hygiene.go and meaning.go were removed, and a FuzzHygienePreset target was added with a clear explanation of why idempotency is intentionally skipped (HTML-entity decoding is legitimately non-idempotent).
A fresh pass over the full change set finds no blocking issues:
- Correctness:
PreserveMeaningPunctcorrectly keeps./,only when both neighbors are ASCII digits,+only when terminating an alphanumeric token, and%only after a digit, avoiding token-merging regressions.NormalizeUnicodeLatinstrips Mn marks only when the preceding base is Latin, preserving non-Latin combining marks.RemoveFormatCharscorrectly excludes\nand\tfrom removal. - Error handling: No swallowed errors or silent failures;
html.UnescapeStringis appropriately wrapped. - Security: No injection, auth, or trust-boundary concerns introduced.
- Tests: Thorough unit-test coverage for each new stage and preset, explicit collision-avoidance assertions in
TestMeaningPresetGoldens, and fuzz targets for both new presets. - API consistency:
MeaningPresetandHygienePresetfollow the same option-pattern and stage-chaining conventions as existing presets.
Mergeable.
1 participant
What
Four new pipeline stages and two presets for meaning-preserving text normalization:
2.5→2 5under old FilterRunes deletion became25). Keeps./,between digits (4.5,1,000— separators NOT unified), token-final+(s22+,c++),%after digit (100%). Combining marks (Mn/Mc/Me) are kept — Devanagari matras and Arabic harakat are vowels, not punctuation.café→cafe, butकिताबandمَكتَبsurvive intact (the existingNormalizeUnicoderemoves ALL Mn marks, destroying Indic/Arabic vowel signs).\n/\t).Galaxy S22+must not collide withGalaxy S22.Why
Downstream consumers (framework-golang textpipe → veliu.com embeddings) need a fingerprint preset that doesn't destroy meaning and a payload-grade hygiene preset. Existing presets are intentionally untouched (byte-identical behavior).
Testing
s22+≠s22,4.5≠45,1,000≠1 000, no dedup, Indic/Arabic preserved.FuzzMeaningPreset(20s run clean): output alphabet restricted to letters/numbers/marks/space/.,+%, no uncollapsed whitespace.go vetclean.Release: will tag v1.3.0 after merge.