fix: preserve Unicode marks in FTS queries#37
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 8:39 AM ET / 12:39 UTC. Summary Reproducibility: yes. Source inspection of current main shows combining marks are not retained by Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the narrow store helper fix after removing the release-owned changelog conflict and adding contributor-supplied real behavior proof. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of current main shows combining marks are not retained by Is this the best way to solve the issue? Yes for the functional code path. Adding Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e74b0c4130c9. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ff8e9ed20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| for _, r := range value { | ||
| if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '_' { | ||
| if unicode.IsLetter(r) || unicode.IsDigit(r) || unicode.IsMark(r) || r == '_' { |
There was a problem hiding this comment.
When a mark follows a character that this helper discards, this predicate starts a new token from the mark. For inputs like hello ❤️ or a leading combining mark before a word, FTS5TokenQuery now emits an extra quoted mark term under implicit AND (for example, "hello" "️"), but SQLite's default FTS5 tokenizer does not index standalone marks, so otherwise matching documents are filtered out instead of ignoring the non-searchable character. Only preserve marks when they are attached to an existing token/base character.
Useful? React with 👍 / 👎.
3ff8e9e to
5ed6342
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1 participant
Summary
Root cause
PR #35 kept only Unicode letters, digits, and underscores. Decomposed graphemes therefore split at combining marks, so the generated query no longer matched SQLite's token for the original text.
Validation
GOWORK=off go test -count=1 ./store -run '^TestFTS5Helpers$' -vGOWORK=off go test -count=1 ./...GOWORK=off go test -count=1 -race ./...GOWORK=off go mod tidyplus clean module diffGOWORK=off go vet ./...staticcheck ./store/...gosecon./store/...The SQLite proof covers punctuation, quote/operator-shaped input, precomposed and decomposed Unicode, multiple scripts, empty/all-punctuation input, and unchanged legacy helper behavior. It uses only a temporary database.