feat(ecomops): knowledge base — shop policies for draft replies#22
Conversation
Closes a gap in what #19 shipped: the workflow accepted a `knowledge_lookup` but nothing implemented it, and the system prompt forbids answering from anything outside the "Dữ liệu" block. So shipping_fee and return_exchange — two of the seven intents — could never actually be answered; the bot could only promise to check. That's the safe failure mode, but it isn't a useful one. - knowledge.py — two interchangeable async lookups: * MagicKnowledgeLookup reads MagiC's Knowledge Hub (GET /api/v1/knowledge), so policies live alongside every other MagiC workload. * StaticKnowledgeLookup reads a YAML file, for shops running the pack against an in-memory core with no Hub. Entries carry two tags (`ecomops` + topic), and lookups require both. The Hub has no server-side tag filter, so without that a shipping question would get answered with the return policy whenever the keyword search overlapped — or worse, with unrelated org knowledge that merely shared a word. A Hub outage degrades to "no policy data" rather than dropping the message; the guardrails still catch anything the model invents to fill the gap. - sample_knowledge.yaml + seed_knowledge.py — Vietnamese starter KB (shipping fees, 7-day return policy, order statuses, product/complaint handling) and a seeder. Both the file and the seeder log loudly that this is sample data to be replaced with the shop's real policy, since wrong content here becomes a wrong answer to a customer. - CI: the install step now picks up every requirements.txt under connectors/ and packs/ instead of naming one file. Adding pyyaml for this feature would otherwise have gone missing on CI (it passed locally only because my venv happened to have it), and a future connector's deps would hit the same trap. Intentionally NOT included: the Nhanh.vn connector, which is the other Tuần 3-4 item. BUSINESS_MODEL.md and OPEN_CORE_STRATEGY.md both list the production Nhanh.vn connector as Commercial ("điểm khác biệt lớn"), and this is the public repo — that needs a deliberate decision, not a drive-by commit. Tests: 21 new (topic mapping, tag filtering, top_k, no-topic short-circuit, Hub-down degradation, malformed payloads, static YAML, seeding round-trip, plus a check that every topic in the sample YAML is reachable from some intent so the file can't drift into dead data). 191 total, all passing.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
My own mistake from the previous commit. Switching the install step to a `run: |` block moved the pip invocation onto a continuation line, but the NOSONAR marker stayed up on the `run: |` key — so it suppressed nothing and SonarCloud re-applied the two supply-chain hotspots (S4830/S5445), dropping the Security Rating to C. Back to a single-line `run:`, matching the neighbouring install/lint steps where this pattern is already proven: the marker is a YAML comment on the same line as the command, stripped before the shell ever sees it. Behaviour is unchanged — Python Tests passed on the previous commit, confirming the find/xargs install and the new pyyaml dependency both work.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
SonarCloud flagged a high-severity path-injection vulnerability in the code I added in this PR — and it was right, so this fixes it rather than suppressing it. `seed_knowledge.py` passes sys.argv[1] straight into `knowledge_entries_for_seeding()`, which did `Path(path).read_text()` with no validation, and everything it reads is POSTed into the Knowledge Hub. That turns "seed the knowledge base" into "copy any file this process can read into a searchable store" — an arbitrary-file read whose output lands somewhere it can be queried back out. `StaticKnowledgeLookup.from_yaml` had the same hole. Both now go through `_resolve_yaml_path()`, which requires an existing regular file with a .yaml/.yml suffix. It calls `resolve()` first, deliberately: that collapses `..` and follows symlinks, so a `policy.yaml` symlinked at a private key is judged by its real name and fails the suffix check instead of sailing through it. The seeder CLI now exits 2 with a clear message instead of propagating a traceback. Verified by hand that /etc/hostname, a suffix-less file and a .yaml symlink to a key file are all refused, and that a genuine policy YAML still loads. Note on the previous commit: it moved a NOSONAR marker in ci.yml on the theory that the block-scalar placement was what dropped the Security Rating. That placement was genuinely wrong and is worth keeping fixed, but it was not the cause — this was. The rule key only became visible once the finding surfaced as a code-scanning review comment on knowledge.py:134. Tests: 7 new (non-YAML suffix, missing file, directory, symlink disguise, traversal, valid file, and the same validation on StaticKnowledgeLookup). 198 total, all passing.
|
3 participants
What does this PR do?
Closes a gap in what #19 shipped: the workflow accepted a
knowledge_lookupbut nothing implemented it, and the system prompt forbids answering from anything outside the "Dữ liệu" block. Soshipping_feeandreturn_exchange— two of the seven intents — could never actually be answered; the bot could only promise to check. That's the safe failure mode, but it isn't a useful one.Roadmap item: Tuần 3–4, "Xây Knowledge Base mẫu cho EcomOps (chính sách đổi trả, phí ship, FAQ)".
knowledge.py— two interchangeable async lookupsMagicKnowledgeLookupGET /api/v1/knowledge)StaticKnowledgeLookupEntries carry two tags (
ecomops+ topic) and lookups require both. The Hub has no server-side tag filter, so without that a shipping question would get answered with the return policy whenever the keyword search overlapped — or worse, with unrelated org knowledge that merely shared a word. Both cases are covered by tests.A Hub outage degrades to "no policy data" rather than dropping the message; the guardrails still catch anything the model invents to fill the gap.
Sample KB + seeder
sample_knowledge.yaml(shipping fees, 7-day return policy, order statuses, product/complaint handling) andpython -m ecomops.seed_knowledge. Both the file and the seeder state loudly that this is sample data to be replaced with the shop's real policy — wrong content here becomes a wrong answer to a customer.CI
The install step now picks up every
requirements.txtunderconnectors/andpacks/instead of naming one file. Addingpyyamlfor this feature would otherwise have gone missing on CI — it passed locally only because my venv happened to have it — and a future connector's deps would hit the same trap.Not included, on purpose
The Nhanh.vn connector, the other Tuần 3–4 item.
BUSINESS_MODEL.mdandOPEN_CORE_STRATEGY.mdboth list the production Nhanh.vn connector as Commercial ("điểm khác biệt lớn"), and this is the public repo. That's a deliberate decision to make, not a drive-by commit — happy to build it inmagic-businessinstead, or here if the split has changed.Related issue
Tracks Month 1, Tuần 3–4 (Knowledge Base mẫu) of the technical roadmap in magic-business/technical-planning.
Checklist
go build ./...clean (Go core untouched)top_k, no-topic short-circuit (greetings spend no request), Hub-down degradation, malformed payload shapes, static YAML, seeding round-trip, plus a guard that every topic in the sample YAML is reachable from some intent so the file can't drift into dead dataexample_worker.pywired to the HubGenerated by Claude Code