Skip to content

feat(search): add multilingual lexical fields - #180

Open
vkehfdl1 wants to merge 5 commits into
openclaw:mainfrom
vkehfdl1:feat/multilingual-lexical-search
Open

feat(search): add multilingual lexical fields#180
vkehfdl1 wants to merge 5 commits into
openclaw:mainfrom
vkehfdl1:feat/multilingual-lexical-search

Conversation

@vkehfdl1

@vkehfdl1 vkehfdl1 commented Aug 18, 2026

Copy link
Copy Markdown

Summary

  • add opt-in [search.lexical] fields for Korean, Japanese, Chinese, and Arabic
  • keep independent FTS5 statistics per analyzer and merge rankings with RRF
  • Python is gone from the lexical path
  • Korean uses the published github.com/codingpot/kiwigo helper and native Kiwi 0.23.2
  • Japanese uses the optional discrawl-ja helper (github.com/ikawaha/kagome/v2 Search mode)
  • Chinese uses the optional discrawl-zh helper (github.com/go-ego/gse CutSearch)
  • Arabic is an in-process Go light stemmer; no helper and no CGO
  • the default Discrawl release stays pure Go / CGO-free and does not embed Kagome or GSE dictionaries
  • helpers load lazily on first indexing or search use
  • the existing unicode61 path is unchanged when no lexical languages are configured

Configuration

[search.lexical]
languages = ["ko", "ja", "zh", "ar"]
kiwi_command = "~/.local/share/discrawl/bin/discrawl-kiwi"
kiwi_model = "~/.local/share/discrawl/models/kiwi/base"
ja_command = "~/.local/share/discrawl/bin/discrawl-ja"
zh_command = "~/.local/share/discrawl/bin/discrawl-zh"

discrawl lexical install now reports required helpers. It does not download packages or invoke pip.

Live verification

Four-language live E2E and quality benchmark passed with no Python process:

Language unicode61 recall@5 multilingual recall@5
Korean / kiwigo + Kiwi 0.23.2 0/5 5/5
Japanese / Kagome Search 0/5 5/5
Chinese / GSE CutSearch 0/5 5/5
Arabic / in-process 0/5 5/5
Total 0/20 20/20

Helper protocol smoke tests:

  • discrawl-ja tokenized 東京都庁に行きます to 東京 都庁 都 庁 に 行き 行く ます
  • discrawl-zh tokenized 自然语言处理很有趣 to include 语言 / 自然 / 处理

Verification

  • make check lint, gosec, govulncheck, 85.0% coverage, smoke, and six-target snapshot passed
  • go test -race ./... passed on rerun after an unrelated internal/share git-hardlink flake
  • helper unit tests passed for Kagome and GSE
  • source scan found no kiwipiepy, sudachipy, Jieba, or Snowball Python lexical workers

License boundary

Kiwi/kiwigo remain LGPL-2.1 and are not linked into the default MIT binary. Kagome is MIT; GSE is Apache-2.0. Both live only in optional helper modules. Notices are in THIRD_PARTY_NOTICES.md.

@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@vkehfdl1

Copy link
Copy Markdown
Author

@steipete Could you review this opt-in multilingual lexical search implementation? It keeps the default path unchanged, uses independent FTS5 fields with RRF, and includes live Kiwi/Sudachi/Jieba/Arabic E2E plus a reproducible targeted benchmark.

GitHub did not allow the fork author to set a formal reviewer through RequestReviewsByLogin, so I am requesting review here.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 18, 2026
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 25, 2026, 3:16 AM ET / 07:16 UTC.

ClawSweeper review

What this changes

This PR adds opt-in Korean, Japanese, Chinese, and Arabic lexical search indexes with language-specific tokenizers and reciprocal-rank fusion.

Merge readiness

⚠️ Needs maintainer review before merge - 4 items remain

Keep open: the prior index-rebuild blocker remains on the unchanged head, and the new persistent search/config surface also needs maintainer product direction.

Priority: P2
Reviewed head: 7f3f7387e95dbf1ae3662cd85c85a2bd18ffb41c
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The claimed runtime proof is strong, but the unresolved analyzer-identity defect prevents merge readiness.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (linked_artifact): The PR body and linked benchmark provide after-fix helper and CLI search evidence; future evidence should continue to redact private paths and credentials.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (linked_artifact): The PR body and linked benchmark provide after-fix helper and CLI search evidence; future evidence should continue to redact private paths and credentials.
Evidence reviewed 4 items Unresolved index identity defect: The rebuild gate accepts only the constant version "2"; it does not reflect the configured Kiwi model or helper commands that determine stored query terms.
Prior finding remains unchanged: The reviewed head is unchanged from the prior review head, so the previously reported analyzer-fingerprint blocker remains unfixed.
Current main does not include this branch: The PR head is not an ancestor of current main, so the requested feature is neither implemented on main nor in v0.13.3.
Findings 1 actionable finding [P2] Fingerprint tokenizer settings before reusing lexical indexes
Security None None.

How this fits together

Discrawl stores Discord messages in SQLite and searches those local archives from its CLI. The optional lexical path tokenizes messages and queries into language-specific FTS tables, then merges those results with the default search index.

flowchart LR
A[Discord message archive] --> B[Language tokenizer]
B --> C[Language-specific FTS indexes]
A --> D[Default FTS index]
E[Search query] --> F[Query tokenizer]
F --> C
F --> D
C --> G[Rank fusion]
D --> G
G --> H[CLI search results]
Loading

Decision needed

Question Recommendation
Should Discrawl own optional language-specific helper binaries and persistent lexical-index configuration in core after the rebuild-safety repair? Sponsor the core feature: Accept the optional lexical fields after the analyzer-identity repair and focused upgrade proof.

Why: This is a new configuration and distribution surface spanning native and Go helper dependencies; source review cannot determine the intended long-term product boundary.

Before merge

  • Fingerprint tokenizer settings before reusing lexical indexes (P2) - ensureLexicalFTS accepts any stored value of "2", although kiwi_model, kiwi_command, ja_command, and zh_command determine indexed terms. Changing one retains old rows but sends queries through the new analyzer, so configured searches can silently miss messages. Persist an effective per-language identity and rebuild on mismatch, with a settings-change lifecycle regression.
  • Resolve merge risk (P1) - Changing a configured helper path or Kiwi model can silently retain rows tokenized by the old analyzer while queries use the new one.
  • Resolve merge risk (P1) - The feature adds persistent indexes, configuration, optional helper binaries, and CI work; acceptance of that ongoing core surface requires maintainer direction.

Findings

  • [P2] Fingerprint tokenizer settings before reusing lexical indexes — internal/store/lexical.go:81-82
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation and test delta non-test +1,796, tests +1,199; 45 files affected The feature spans storage, CLI, configuration, helper modules, CI, documentation, and substantial regression coverage.

Merge-risk options

Maintainer options:

  1. Fingerprint analyzer settings (recommended)
    Store a per-language identity derived from the active tokenizer and relevant helper/model configuration, then rebuild that table when it changes.
  2. Pause pending scope direction
    Pause the PR if maintainers do not want optional helper binaries and persistent multilingual indexes in the core product.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Persist effective analyzer identities, rebuild changed-language indexes on mismatch, and add a configuration-change lifecycle regression test.

Technical review

Best possible solution:

Persist an effective per-language analyzer identity with each index, rebuild on mismatch, and land the optional helper design only if its core maintenance boundary is explicitly sponsored.

Do we have a high-confidence way to reproduce the issue?

Yes: create a lexical index with one helper or model setting, reopen with a different setting, then search a term whose tokenization differs. The constant stored version prevents the required rebuild.

Is this the best way to solve the issue?

No: disabled-to-enabled invalidation is covered, but changing an enabled analyzer's command or model is not safely handled; identity-based invalidation is the narrower durable repair.

Full review comments:

  • [P2] Fingerprint tokenizer settings before reusing lexical indexes — internal/store/lexical.go:81-82
    ensureLexicalFTS accepts any stored value of "2", although kiwi_model, kiwi_command, ja_command, and zh_command determine indexed terms. Changing one retains old rows but sends queries through the new analyzer, so configured searches can silently miss messages. Persist an effective per-language identity and rebuild on mismatch, with a settings-change lifecycle regression.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 5ab5d11351b9.

Labels

Label justifications:

  • P2: The optional search path can return incomplete results after a configuration change, while the default search path remains unaffected.
  • merge-risk: 🚨 compatibility: Existing lexical archives can retain terms generated by an old analyzer after a helper or model setting changes.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (linked_artifact): The PR body and linked benchmark provide after-fix helper and CLI search evidence; future evidence should continue to redact private paths and credentials.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and linked benchmark provide after-fix helper and CLI search evidence; future evidence should continue to redact private paths and credentials.

Evidence

Acceptance criteria:

  • [P1] go test ./internal/store.

What I checked:

  • Unresolved index identity defect: The rebuild gate accepts only the constant version "2"; it does not reflect the configured Kiwi model or helper commands that determine stored query terms. (internal/store/lexical.go:81, 7f3f7387e95d)
  • Prior finding remains unchanged: The reviewed head is unchanged from the prior review head, so the previously reported analyzer-fingerprint blocker remains unfixed. (internal/store/lexical.go:81, 7f3f7387e95d)
  • Current main does not include this branch: The PR head is not an ancestor of current main, so the requested feature is neither implemented on main nor in v0.13.3. (5ab5d11351b9)
  • Current search/config ownership: The released baseline search entry point and SearchConfig declaration are attributed to the current baseline commit. (internal/store/query.go:101, 089c1f5b6ae4)

Likely related people:

  • steipete: Current-main blame attributes the pre-feature SearchMessages and SearchConfig baseline to Peter Steinberger; the PR discussion also requested this reviewer. (role: current search and configuration baseline author; confidence: medium; commits: 089c1f5b6ae4; files: internal/store/query.go, internal/config/config.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Persist per-language analyzer identities and add a regression proving that changing a helper or model rebuilds its index.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (11 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-19T07:48:05.243Z sha 58e41fb :: found issues before merge. :: [P2] Verify the Kiwi archive before privileged installation
  • reviewed 2026-08-19T16:40:55.371Z sha 7f3f738 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-20T07:49:55.263Z sha 7f3f738 :: found issues before merge. :: [P2] Rebuild a lexical table when its analyzer configuration changes
  • reviewed 2026-08-21T22:09:37.943Z sha 7f3f738 :: found issues before merge. :: [P2] Fingerprint analyzer settings before reusing lexical indexes
  • reviewed 2026-08-22T21:57:28.572Z sha 7f3f738 :: needs changes before merge. :: [P2] Rebuild lexical indexes when analyzer settings change
  • reviewed 2026-08-23T18:59:31.376Z sha 7f3f738 :: found issues before merge. :: [P2] Fingerprint analyzer settings before reusing lexical indexes
  • reviewed 2026-08-23T21:02:25.952Z sha 7f3f738 :: found issues before merge. :: [P2] Fingerprint analyzer settings before reusing lexical indexes
  • reviewed 2026-08-24T17:13:15.181Z sha 7f3f738 :: needs changes before merge. :: [P2] Fingerprint tokenizer settings before reusing lexical indexes

@vkehfdl1

Copy link
Copy Markdown
Author

@clawsweeper[bot] I addressed the review in commit e581712 and would appreciate a re-review.

Resolved items:

  1. Disabled -> re-enabled lifecycle: writer opens now invalidate version markers for configured-out language tables. Re-enabling forces a full rebuild. Added an enabled -> disabled write -> re-enabled regression test.
  2. Windows venv path: absolute C:\...\python.exe paths are supported directly; only known launcher names or absolute paths are accepted. Windows amd64 cross-build passes.
  3. Lazy loading: archive open creates lazy tokenizer handles only. Python starts on the first indexing/search tokenization call, and startup failures are cached. Metadata-only opens work without installed modules.
  4. Selected installation: added explicit discrawl lexical install. It requires a venv, adds pip --require-virtualenv, installs exact pinned packages only for configured languages, and never runs implicitly during open/sync/search.
  5. Security boundary: workers inherit an allowlisted environment only; Discord tokens, API keys, and pip credentials are removed. No shell execution is used, and arbitrary relative interpreter commands are rejected.
  6. Inspectable proof: live transcript and reproduction instructions are committed at https://github.com/openclaw/discrawl/blob/e581712/docs/benchmarks/multilingual-lexical.md#live-verification-transcript

Additional live check: a clean languages = ["ko"] venv opened the archive before Kiwi existed, discrawl lexical install installed Kiwi only, and CLI search for 저녁 returned the Korean fixture.

Full make check passes: lint, gosec, govulncheck, 85.0% coverage, race tests, and Darwin/Linux/Windows snapshot builds.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 18, 2026
@vkehfdl1
vkehfdl1 marked this pull request as draft August 19, 2026 04:27
@vkehfdl1

Copy link
Copy Markdown
Author

@clawsweeper[bot] Korean tokenization has been reworked in commit 89ea8a1; please re-review this revision.

The previous kiwipiepy/Python implementation is removed. Korean now uses the published github.com/codingpot/kiwigo Go binding with native Kiwi 0.23.2 through the optional persistent discrawl-kiwi helper. This keeps the default six-target Discrawl release pure Go while removing Python entirely from the Korean path.

Review-relevant changes:

  1. kiwipiepy was removed from the worker and installer package map.
  2. The remaining Python installer for ja/zh/ar now sets the same allowlisted environment as workers, so parent DISCORD_BOT_TOKEN, API keys, and pip credential variables are not inherited. A subprocess test proves this.
  3. kiwi_command accepts only an absolute path or the known discrawl-kiwi launcher; arbitrary relative commands are rejected.
  4. The Kiwi helper and Python workers remain lazy and are not started by archive open.
  5. The helper dynamically links Kiwi rather than embedding it into the default Discrawl binary. Native prerequisites and LGPL/source obligations are documented in THIRD_PARTY_NOTICES.md and tools/discrawl-kiwi/README.md.
  6. CI now builds/tests the published kiwigo binding against official Kiwi 0.23.2 native assets on Ubuntu and macOS.

Live evidence:

  • helper: {"ready":true,"version":"0.23.2"} then {"tokens":"오늘 저녁 먹 음 기록"}
  • Discrawl CLI search for 저녁 returned 오늘 저녁먹음 기록
  • four-language benchmark remained 0/20 -> 20/20, SQLite ratio 1.37x
  • full make check passed including security scans, 85.0% coverage, race, and six-target release snapshots.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Aug 19, 2026
Replace remaining ja/zh/ar Python workers with Go analyzers. Japanese and
Chinese now run as optional Kagome and GSE helpers so the default binary
stays small and CGO-free. Arabic light stemming is in-process.
@vkehfdl1

Copy link
Copy Markdown
Author

@clawsweeper[bot] Python is now fully removed from the lexical path in 58e41fb. Please re-review.

What changed:

  • Japanese uses optional tools/discrawl-ja (pure-Go Kagome Search + embedded IPADIC)
  • Chinese uses optional tools/discrawl-zh (pure-Go GSE CutSearch)
  • Arabic is in-process Go light stemming; no helper
  • discrawl lexical install no longer calls pip or a virtualenv
  • the default Discrawl binary remains CGO-free and does not embed Kagome/GSE dictionaries

Live proof, no Python:

  • helper discrawl-ja: 東京都庁に行きます東京 都庁 に 行き 行く ます
  • helper discrawl-zh: 自然语言处理很有趣自然 语言 自然语言 处理 很 有趣
  • four-language benchmark remained 0/20 → 20/20
  • source scan found no kiwipiepy, sudachipy, Jieba, or Snowball workers

Gates: lint/gosec/govulncheck, 85.0% coverage, six-target snapshot, and go test -race ./... (one unrelated share git-hardlink flake reran clean).

@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Aug 19, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 19, 2026
@clawsweeper clawsweeper Bot removed merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 19, 2026
@vkehfdl1
vkehfdl1 marked this pull request as ready for review August 20, 2026 07:46
@vkehfdl1
vkehfdl1 requested a review from a team as a code owner August 20, 2026 07:46
@clawsweeper

clawsweeper Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 20, 2026
@vkehfdl1

Copy link
Copy Markdown
Author

@clawsweeper Can you assign a reviewer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant