feat: get_knowledge language filter#267
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
[QA] Round 1 on PR #267 — QA Failed
Verdict: one substantive observation on semantic mismatch. Code is otherwise clean, test is good, 827/827 pass, CI green, CHANGELOG accurate.
What is working well
- SQL clause
language = %s::regconfigis parameterized and safe ✓ - Store Protocol updated with
languageparameter ✓ - Test
test_get_knowledge_filtered_by_languagecreates entries with explicit languages and verifies filter returns only the matching one ✓ - Docstring is clear about the purpose: "show me all entries in language X" ✓
- CHANGELOG under
### Added, accurate ✓
Substantive observation — unknown ISO codes silently map to simple, returning unexpected results
The filter path is:
lang_regconfig = iso_to_regconfig(language) if language else Noneiso_to_regconfig("xx") returns "simple" (unknown codes fall back to simple per the mapping design). So get_knowledge(language="xx") filters for language = 'simple'::regconfig — which returns all entries with the default language, not zero entries.
From the user's perspective: "show me entries in language xx" → gets entries in simple (the majority of entries). This is a semantic mismatch.
The issue is that iso_to_regconfig() was designed for WRITE paths where "unknown → simple" is the correct fallback (you want the entry stored with a valid regconfig). For READ/FILTER paths, the semantics are different — an unknown code should match nothing, not match "simple."
Suggested fix options:
-
(a) Check for the fallback case explicitly:
raw = iso_to_regconfig(language) lang_regconfig = raw if (raw != "simple" or language.strip().lower() == "simple") else None
This filters for
simpleonly when the user explicitly passeslanguage="simple". Unknown codes becomeNone(no filter applied) — or alternatively, return an empty result set. -
(b) Return an error for unknown codes:
_error_response("invalid_parameter", f"Unknown language code: {language!r}", ...). Strict but unambiguous. -
(c) Accept the current behavior and document it: "Unknown ISO codes are treated as 'simple' for filtering purposes." Honest but potentially surprising.
The choice between (a) and (b) depends on whether you want "unknown code = no filter" or "unknown code = error." Either is better than the current "unknown code = filter for simple."
Verification
- 827/827 tests pass (+1 new)
- CI: all green
- ruff/mypy: clean
- CHANGELOG:
### Added, accurate, refs #262 + #238
Applying QA Failed as the final act.
|
[QA] Round 1 — QA Failed. Substantive: unknown ISO codes (e.g. |
|
[Dev] QA round 1 fix: unknown ISO codes now return an error instead of silently matching simple entries. Added language='simple' as explicit filter option. New test for error case. |
25c866e to
79ee624
Compare
Add optional `language` parameter (ISO 639-1) to get_knowledge tool, store protocol, and PostgresStore. Maps to WHERE e.language = %s::regconfig filter. Enables queries like "show me all French entries". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unknown ISO codes (e.g., 'xx') now return an error instead of silently matching all 'simple' entries. Supports language='simple' explicitly for filtering entries with no detected language. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… round 1) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
79ee624 to
d3aa98a
Compare
cmeans
left a comment
There was a problem hiding this comment.
[QA] Round 2 — Signoff
Round-1 finding resolved. Dev chose option (b) — unknown ISO codes return an error with a helpful message listing valid formats. The implementation is clean:
normalized == SIMPLE→ filter for simple entries (explicitlanguage="simple"works as a valid filter) ✓normalized not in ISO_639_1_TO_REGCONFIG→_error_responsewithinvalid_parametercode ✓- Valid ISO code →
iso_to_regconfig()→ regconfig filter ✓
No more semantic mismatch — "language=xx" now gives the user a clear error instead of silently returning all simple-language entries.
838/838 pass (+11 from round 1). ruff clean. 2 PR checkboxes checked off. Zero new observations.
Ready for QA Signoff.
|
[QA] Round 2 — Ready for QA Signoff. Unknown ISO codes now return error (option b). Explicit language="simple" filter works. 838/838 pass, 2 checkboxes checked, ruff clean. Applying label as final act. |
Summary
get_knowledgegains optionallanguageparameter (ISO 639-1 code)WHERE e.language = %s::regconfigfilter in PostgresStorelanguageparameterCloses #262. Refs #238.
QA
Prerequisites
pip install -e ".[dev]"AWARENESS_PORT=8421)Manual tests (via MCP tools)
Expected: only the French entry returned
Expected: both entries returned
🤖 Generated with Claude Code