From 7cb5b0fc2b5caaea4e729cae74457fce9078f47b Mon Sep 17 00:00:00 2001 From: dmitry Date: Thu, 14 May 2026 16:14:55 +0300 Subject: [PATCH] add lossless-permissive nodefilter proposal for #122 Co-authored-by: Cursor --- .../LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md diff --git a/propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md b/propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md new file mode 100644 index 0000000..eca1c7d --- /dev/null +++ b/propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md @@ -0,0 +1,89 @@ +# Lossless-permissive `NodeFilter` contract (tactical fix for silent-drop bug class) + +## Status + +Proposal — not yet implemented. Tracks upstream discussion: [HumanBean17/java-codebase-rag#122](https://github.com/HumanBean17/java-codebase-rag/issues/122) (tactical subset of the strategic frame question in #117). + +## Problem Statement + +MCP tools that accept a `filter` object use the shared `NodeFilter` Pydantic model in `mcp_v2.py`. Today that surface has two honesty gaps: + +1. **Unknown keys are ignored.** Pydantic’s default extra handling means typos or misplaced fields (for example nesting a `kind` selector inside `filter` instead of at the tool boundary) are **silently dropped**. The tool runs with a weaker filter than the agent believed it sent. + +2. **Cross-kind fields are ignored at match time.** `_node_matches_filter` only reads fields that apply to the row’s kind (`symbol` / `route` / `client`). Fields that belong to another kind are never consulted — so an agent can pass symbol-only constraints (for example `fqn_prefix`) while calling `find(kind="client")` and still receive **every** client, with no signal that part of the filter was inert. + +Both behaviors are **lossy**: input the agent treated as meaningful is discarded without an error. That matches the “silent-drop bug class” called out in #117; this proposal fixes that class **without** choosing the long-term strict vs permissive vs hybrid frame (#117). + +**Bright line (from #122):** lossless input normalization (same intent, different encoding — for example JSON string vs dict) remains acceptable; **lossy** silent drops are not. + +## Proposed Solution + +1. **`NodeFilter` uses explicit extra forbiddance.** Set `model_config = ConfigDict(extra="forbid")` on `NodeFilter` so unknown top-level keys raise `pydantic.ValidationError` at parse time. That catches the “wrong key / nested kind” class cheaply. + +2. **Per-kind applicability validation** after `NodeFilter` is constructed and **before** graph push-down / neighbor expansion: the set of filter fields that are populated (non-`None`, and for lists non-empty if that is the chosen rule) must be a subset of the fields defined as applicable for the **effective** kind: + - `find_v2`: the `kind` argument. + - `search_v2`: post-filter rows are always symbol-shaped hits from Lance; applicability is the **symbol** field set (today’s `_node_matches_filter("symbol", …)` branch). + - `neighbors_v2`: the **neighbor node’s** kind after resolution (`symbol` / `route` / `client`) — the filter applies to `other` nodes loaded in the loop, so validation should mirror that (see Open Questions). + + When a populated field is not applicable, return the same structured failure style as other tool errors (`success=False`, `message=…`) listing the offending field(s) and the **applicable field names for that kind** so the message acts as a teaching surface. Composition note: if [#120](https://github.com/HumanBean17/java-codebase-rag/issues/120) (hints field) lands, these messages are natural candidates for machine-readable `hints`. + +3. **Keep `_coerce_filter` behavior.** JSON-decoding a string into a dict before validation is a **lossless** multi-form input path (same object, different wire encoding) and stays as-is per #122. + +4. **Do not change `search.query` semantics.** Fuzzy query ranking is unrelated; only the `filter` contract is tightened. + +**Repo-specific correction vs #122 text:** `describe_v2` in this codebase does **not** accept `filter`; only `find_v2`, `search_v2`, and `neighbors_v2` do. No change is required on `describe_v2` for filter honesty unless a future proposal adds filtering there. + +## Scope + +- `mcp_v2.py`: `NodeFilter` model config; a small applicability helper (or inline checks) keyed by kind; wiring at `find_v2`, `search_v2`, and `neighbors_v2` entry points after coerce + validate. +- Tests: unknown filter keys; cross-kind populated fields; empty filter still succeeds; JSON-string filter still accepted. +- README / tool instruction strings if the public contract text should mention loud-fail filter semantics (minimal delta). + +## Schema / Ontology / Re-index impact + +- **Ontology bump:** not required (no graph or indexer semantics change). +- **Re-index required:** no. +- **Config / tool surface changes:** behavior change only on invalid filter shapes — agents that relied on ignored extra keys or inert cross-kind fields will start seeing errors instead of silent success. + +## Tests / Validation + +- Unknown top-level key in `filter` → validation failure (exact surface: `ValidationError` vs wrapped `success=False` — see Open Questions). +- Populated symbol-only field with `find(kind="client")` → `success=False` with message listing applicable client fields. +- Populated client-only field with `find(kind="symbol")` → `success=False`. +- `fqn_prefix` + `find(kind="symbol")` → still honored (regression). +- Empty `{}` filter (and `None` where allowed) → unchanged success path. +- Filter passed as JSON string → still decodes then validates (lossless multi-form). + +Run: `.venv/bin/ruff check .` and `.venv/bin/python -m pytest tests -v` (plus any new focused tests under `tests/` naming the scenarios above). + +## Open Questions ([TBD]) + +1. **`ValidationError` vs uniform `success=False` for unknown keys.** `find_v2` / `search_v2` already wrap generic exceptions into `success=False`; `neighbors_v2` currently re-raises `ValidationError` for at least some validation paths. Should unknown-key failures be **wrapped** into `success=False` for all three tools for a uniform MCP experience, or is raising `ValidationError` acceptable for “contract violations”? + - **Recommended:** catch filter `ValidationError` at the same boundary as applicability errors and return `success=False` with a parsed, agent-readable message (consistent across tools). + +2. **`neighbors_v2` applicability kind.** Neighbors can be mixed when multiple origin ids are passed; each edge’s `other` has its own kind. Is the contract “filter must be applicable to **every** neighbor kind that could appear,” “union of fields that applies to any neighbor,” or “validate per neighbor row” (strictest, matches runtime)? + - **Recommended:** validate **per neighbor** inside the loop is unnecessary if we define the rule as: populated fields must be subset of **union** of applicable fields across `symbol`, `route`, `client`, **or** require the filter to only use the common subset (`microservice`, `module`). The issue text implies per-request kind like `find`; neighbors are trickier — default to **reject if any populated field is not applicable to the neighbor kind currently being tested** (same as proposed post-validate before filtering each `other`), which may mean no single upfront check — implementors can still factor a helper `_filter_applies_to_kind(kind, nf) -> str | None` returning an error message. + +3. **Empty lists vs `None`.** Should `exclude_roles: []` count as “populated” for applicability? + - **Recommended:** treat empty list as absent (same as `None`) for applicability and for Cypher push-down, matching intuitive “no-op constraint” behavior. + +## Out of scope + +- Choosing or implementing the full #117 frame (strict / permissive / hybrid vocabulary). +- New tools (`resolve`, etc.), field renames, or cross-kind field aliasing. +- Changing `search.query` fuzzy matching or ranking. +- Changing `EdgeType` literals, `find.kind` literals, or graph schema. +- `describe_v2` (no filter today). + +## Sequencing / Follow-ups + +- **Independent of** graph builder / Kuzu schema work; can land in **one small PR** after any urgent Kuzu query fixes upstream maintainers prioritize (issue #119 in the same tracker — independent). +- **Composes with** hints-field work (#120): teaching-style messages become structured hints later. +- **Strategic** #117 remains open to lock the long-term contract after observing real agent traffic under loud-fail behavior. + +## References (this repo) + +- `NodeFilter` — `mcp_v2.py` (approx. lines 59–76). +- `_coerce_filter` — `mcp_v2.py` (approx. lines 79–98). +- `_node_matches_filter` — `mcp_v2.py` (approx. lines 351–398). +- `search_v2` / `find_v2` / `neighbors_v2` — `mcp_v2.py` (approx. lines 401+).