diff --git a/plans/CURSOR-PROMPTS-LOSSLESS-PERMISSIVE-NODEFILTER.md b/plans/CURSOR-PROMPTS-LOSSLESS-PERMISSIVE-NODEFILTER.md new file mode 100644 index 0000000..7bc690c --- /dev/null +++ b/plans/CURSOR-PROMPTS-LOSSLESS-PERMISSIVE-NODEFILTER.md @@ -0,0 +1,139 @@ +# Cursor task prompts — Lossless-permissive `NodeFilter` (PR-N1) + +Status: **active**. Implements +[`plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md`](./PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md) +and +[`propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md`](../propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md). + +One prompt for the single PR. Copy the **Prompt** block into Cursor agent mode, attach the listed `@-files`, and execute. If the prompt disagrees with the plan, **the plan wins**. + +**Universal rules:** + +- Use repo `.venv/bin/python` and `.venv/bin/ruff` only (see `AGENTS.md`). +- Do not push from the agent; you open the PR. +- If a change would touch files outside the PR’s allowlist, stop and ask. + +--- + +## PR-N1 — Loud-fail `NodeFilter` (forbid extras + kind applicability) + +**Branch:** `feat/lossless-nodefilter` off `master`. + +**Base:** `master`. + +**Plan section:** `plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md` § **PR-N1 - Loud-fail `NodeFilter`**. + +**Estimated diff size:** ~4 files, ~200–350 LOC (mostly `mcp_v2.py` + tests). + +**Attach (`@-files`):** + +- `@plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md` +- `@propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md` +- `@mcp_v2.py` +- `@server.py` +- `@README.md` (MCP tool reference §4 — `NodeFilter` notes only, minimal edit) +- `@tests/test_mcp_v2.py` +- `@tests/conftest.py` (session `kuzu_graph` fixture — read-only unless you must extend fixtures; prefer not) + +**Prompt:** + +```` +You are implementing PR-N1 from `plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md`. + +Read the plan’s **PR-N1** section and the **Resolved design decisions** table in full before coding. The plan is the source of truth. + +## Scope + +- **`mcp_v2.py`** + - Set `NodeFilter.model_config = ConfigDict(extra="forbid")` (Pydantic v2); import `ConfigDict` as needed. + - Implement helpers per plan: populated-field detection (treat `exclude_roles: []` as absent), `_nodefilter_inapplicable_fields`, `_nodefilter_applicability_error` with **stable** field ordering in messages for tests. + - Applicable field sets must match the plan table (aligned with `_node_matches_filter` / `_symbol_where_from_filter`; `source_layer` is client-only). + - **`find_v2`:** after `NodeFilter` is built, if applicability error → `FindOutput(success=False, message=err)` before any graph query. Catch filter `ValidationError` from unknown keys and return `FindOutput(success=False, …)` (same style as today’s broad `except Exception` path — do not leak raw trace to success paths). + - **`search_v2`:** effective kind is always **symbol** for applicability. Fail before iterating hits when filter is invalid. + - **`neighbors_v2`:** wrap **only** filter coerce + `model_validate` (+ any upfront filter-only logic) so `ValidationError` from bad filter keys returns `NeighborsOutput(success=False, message=…)`. **Do not** wrap `@validate_call` failures for missing `direction`, bad `edge_types`, wrong literals — those must still `raise ValidationError`. In the neighbor loop, on first `other_kind` where populated fields include any not applicable to that kind, return `NeighborsOutput(success=False, message=…)` and stop (no silent skip). +- **`server.py`:** Update `_INSTRUCTIONS` and the `Field(description=…)` for `search` / `find` / `neighbors` `filter` parameters — remove “irrelevant keys ignored per kind”; state unknown keys and cross-kind populated fields fail with `success=False` and `message`. +- **`README.md`:** Minimal update under §4 **`NodeFilter` notes:** consistent with `server.py` (one short bullet on strict schema + kind applicability; keep JSON string fallback note). + +## Out of scope (do NOT touch) + +- `describe_v2`, graph builder, `java_ontology.py`, Kuzu schema, Lance indexer, `search.query` / ranking, new MCP tools, ontology bump, `JAVA_CODEBASE_RAG_RUN_HEAVY` paths. +- Any fixture Java under `tests/bank-chat-system/` (fixture data only). +- Structured `hints` on tool outputs (#120). +- Drive-by refactors outside the files in **Scope**. + +## Deliverables + +1. `NodeFilter` rejects unknown top-level keys; no silent drop. +2. `find_v2`, `search_v2`, `neighbors_v2` return `success=False` with teaching `message` when populated fields are not applicable to the effective / neighbor kind (per plan). +3. `neighbors_v2` filter `ValidationError` → `success=False`; `validate_call` argument errors still raise `ValidationError`. +4. `exclude_roles: []` does not count as populated for applicability. +5. `_coerce_filter` JSON-string path unchanged; regressions for JSON string filters stay green. +6. Replace **`test_find_silent_ignore_irrelevant_filter_keys`** with a test that expects **`success=False`** for cross-kind populated fields (name per plan, e.g. `test_find_cross_kind_filter_fields_return_failure`). +7. Add the remaining tests named in the plan § “Tests for PR-N1” (unknown keys find/search/neighbors; cross-kind find/search/neighbors; `test_neighbors_validate_call_still_raises`). +8. `README.md` + `server.py` public strings match the new contract. + +## Tests to run (iteration loop) + +Run only these files during local iteration; **full** `pytest tests` (with `JAVA_CODEBASE_RAG_RUN_HEAVY` unset or `0`) is the merge gate once CI runs on the PR and on `master`. + +- `tests/test_mcp_v2.py` — exercises `find_v2` / `search_v2` / `neighbors_v2`, filter coercion, and graph-backed neighbor paths for applicability. + +## Tests + +Run: + +```bash +cd /path/to/java-enterprise-codebase-rag +.venv/bin/ruff check . +.venv/bin/python -m pytest tests/test_mcp_v2.py -v +.venv/bin/python -m pytest tests -v +``` + +Expected: ruff clean; `tests/test_mcp_v2.py` all pass; full `tests` green with only documented skips (no new skips). New or renamed tests should cover at minimum the nine scenarios listed in the plan § “Tests for PR-N1” items 1–9 plus listed regressions. + +## Sentinel checks + +Run from repo root on `git diff master..HEAD` (or inspect working tree before commit): + +```bash +git diff master --name-only +``` + +Expected: only `mcp_v2.py`, `server.py`, `README.md`, `tests/test_mcp_v2.py` (and this prompts file **only if** you were asked to update tracking — otherwise do **not** commit prompts). + +```bash +rg "irrelevant keys ignored" . +``` + +Expected: **zero** matches after your `server.py` edit. + +```bash +rg "test_find_silent_ignore" tests/test_mcp_v2.py || true +``` + +Expected: **zero** — old test name removed or renamed per plan. + +```bash +rg "extra.?=.?[\"']forbid[\"']|extra=\"forbid\"|ConfigDict\\(extra=.forbid" mcp_v2.py +``` + +Expected: at least one match on `NodeFilter`. + +## Manual evidence + +```bash +.venv/bin/ruff check . +.venv/bin/python -m pytest tests/test_mcp_v2.py -v +.venv/bin/python -m pytest tests -v +``` + +Optional smoke (direct Python): call `find_v2("symbol", {"typo_key": 1}, graph=...)` and assert `success is False` (exact message text is flexible but must be agent-readable). + +## Definition of Done + +- [ ] Plan PR-N1 file-by-file checklist satisfied (`mcp_v2.py`, `server.py`, `README.md`, `tests/test_mcp_v2.py`). +- [ ] Sentinel greps above pass. +- [ ] PR title: **`enforce lossless NodeFilter (forbid extras + kind applicability)`** (or equivalent imperative, lowercase-first-word style per repo convention). +- [ ] Branch: **`feat/lossless-nodefilter`** (or `cursor/lossless-nodefilter` if you prefer agent prefix — one PR, one branch). +- [ ] PR body references `plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md` and `propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md`; states **no ontology bump / no re-index**; lists test command and that full `tests` passed. +```` diff --git a/plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md b/plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md new file mode 100644 index 0000000..dcdb77b --- /dev/null +++ b/plans/PLAN-LOSSLESS-PERMISSIVE-NODEFILTER.md @@ -0,0 +1,140 @@ +# Plan: Lossless-permissive `NodeFilter` contract + +Status: **active (planning)**. This plan implements +[`propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md`](../propose/LOSSLESS-PERMISSIVE-NODEFILTER-PROPOSE.md). + +Depends on: **none** (orthogonal to graph/Kuzu work such as upstream #119). + +## Goal + +- **Unknown `filter` keys** fail at parse time (no silent drop from typos or nested `kind` mistakes). +- **Cross-kind populated fields** fail with `success=False` and a **teaching** `message` that names offending fields and the **applicable field names** for the effective node kind (per proposal). +- **Lossless wire shapes** stay supported: `_coerce_filter` JSON-string → dict path unchanged; empty `{}` / `None` (where allowed) unchanged; `exclude_roles: []` treated as absent (same as `None`) for applicability and push-down. +- **`search.query` semantics** unchanged; only `filter` contract tightens. +- **`neighbors_v2`** returns `success=False` for invalid **filter** contract (including `ValidationError` from `NodeFilter`), matching `find_v2` / `search_v2` — without swallowing `@validate_call` argument errors (direction / `edge_types` / casing) that should continue to raise `ValidationError`. + +## Principles (do not relitigate in review) + +- **Bright line (#122):** lossless normalization (e.g. JSON string vs dict for the same object) is OK; **lossy** silent drops are not. +- **No ontology bump, no re-index** — MCP validation only (`README` may get a one-line contract note if tool docs change). +- **No backward-compat shims** for agents that relied on ignored extras or inert cross-kind fields; those calls become explicit errors. +- **Applicability is derived from `_node_matches_filter` + `_symbol_where_from_filter`** — one source of truth for which `NodeFilter` fields affect which kind; do not duplicate divergent allowlists in `server.py` beyond user-facing copy. + +## PR breakdown - overview + +| PR | Scope | Ontology bump | Files touched (approx) | Test buckets | Independent of | +| --- | --- | --- | --- | --- | --- | +| PR-N1 | `NodeFilter` `extra="forbid"`; applicability helper + wiring in `find_v2`, `search_v2`, `neighbors_v2`; uniform `success=False` for filter `ValidationError` on neighbors; README + MCP `Field` descriptions + `_INSTRUCTIONS` if needed | No | `mcp_v2.py`, `server.py`, `README.md` (minimal), `tests/test_mcp_v2.py` | `tests/test_mcp_v2.py` | — | + +Landing order: **PR-N1 only**. + +## Resolved design decisions + +| Topic | Decision | +| --- | --- | +| Unknown keys | `ConfigDict(extra="forbid")` on `NodeFilter` (Pydantic v2). | +| `ValidationError` vs `success=False` | **Wrap filter-related `ValidationError`** into the same structured tool failure as applicability errors for **all three** tools. **Do not** change `@validate_call` behavior for bad `direction` / `edge_types` / literal casing on `neighbors_v2` (keep raising `ValidationError` for those). Implementation: narrow `try`/`except` around `_coerce_filter` + `NodeFilter.model_validate` (and applicability check) so argument validation still propagates. | +| `neighbors_v2` mixed neighbor kinds | When evaluating each neighbor `other_kind`, if any **populated** field on `NodeFilter` is **not applicable** to `other_kind`, return **`NeighborsOutput(success=False, message=…)` immediately** (first offending neighbor wins). Do not silently skip edges. **Vacuous success:** if there are zero neighbor rows before any applicability check, empty results with a symbol-only filter are acceptable (no neighbor row triggered the check); document as a known residual if reviewers care — proposal prioritizes loud failure when a row is actually evaluated. | +| “Populated” for applicability | `None` absent; for `list[str]` fields, **empty list = absent** (matches proposal and existing `_symbol_where_from_filter` / `_node_matches_filter` list handling). | +| Applicable field sets | **Symbol:** `microservice`, `module`, `role`, `exclude_roles`, `annotation`, `capability`, `fqn_prefix`, `symbol_kind`, `symbol_kinds`. **Route:** `microservice`, `module`, `http_method`, `path_prefix`, `framework`. **Client:** `microservice`, `module`, `source_layer`, `client_kind`, `target_service`, `target_path_prefix`, `client_method`. (Align with current `_node_matches_filter` / `_symbol_where_from_filter`; `source_layer` is client-only in match logic today.) | +| `search_v2` effective kind | Always **symbol**-shaped post-filter rows (per proposal). | + +--- + +# PR-N1 - Loud-fail `NodeFilter` (forbid extras + kind applicability) + +## File-by-file changes + +### 1. `mcp_v2.py` + +- Add `model_config = ConfigDict(extra="forbid")` to `NodeFilter`. +- Add a small helper, e.g. `_populated_nodefilter_fields(nf: NodeFilter) -> set[str]` (field names with non-absent values per principles above). +- Add `_nodefilter_inapplicable_fields(kind: Literal["symbol","route","client"], nf: NodeFilter) -> list[str]` returning sorted field names that are populated but not allowed for `kind` (empty list = filter is applicable to that kind for all populated keys). +- Add `_nodefilter_applicability_error(kind: Literal["symbol","route","client"], nf: NodeFilter) -> str | None` building the teaching `message`: list offending fields + list applicable names for that `kind` (stable ordering for tests). +- **`find_v2`:** after `nf` is built, if `err := _nodefilter_applicability_error(kind, nf)` then `return FindOutput(success=False, message=err)` before graph queries. +- **`search_v2`:** after `nf` is built, if `nf` and `err := _nodefilter_applicability_error("symbol", nf)` then return `SearchOutput(success=False, message=err)` before iterating hits. +- **`neighbors_v2`:** refactor so filter coerce + `model_validate` + **optional** upfront checks live in an inner scope that catches `ValidationError` and returns `NeighborsOutput(success=False, message=…)` with a parsed agent-readable string; in the per-neighbor loop, after `other_kind` / `other_rec` are known, if `errs := _nodefilter_inapplicable_fields(other_kind, nf)` is non-empty when `nf` is not `None`, return `NeighborsOutput(success=False, message=…)` and stop. Preserve existing `except ValidationError: raise` only for errors **outside** the filter pipeline (or split try blocks so filter errors never hit the re-raise path). + +### 2. `server.py` + +- Update `_INSTRUCTIONS` and `Field(description=…)` for `find` / `search` / `neighbors` **filter** parameters: replace “irrelevant keys ignored per kind” with language that unknown keys error and cross-kind populated fields error with `success=False` + `message` (keep JSON-object vs JSON-string guidance). + +### 3. `README.md` + +- Minimal MCP tool reference delta: same contract as `server.py` (one short paragraph or bullet under `find` / `search` / `neighbors` filter docs if those sections duplicate the strings — avoid large doc rewrites). + +### 4. `tests/test_mcp_v2.py` + +- **Replace** `test_find_silent_ignore_irrelevant_filter_keys` (it asserts the old bug): new test expects `success=False` and `message` mentioning inapplicable field(s) for `kind="symbol"` when using route-only `path_prefix` (or rename to `test_find_cross_kind_filter_fields_return_failure` and same assertions). +- Add `test_find_unknown_filter_key_returns_failure` — bogus top-level key → `success=False` (wrapped message) or assert `ValidationError` is not raised to callers of `find_v2` (prefer `success=False` on the output model). +- Add `test_find_symbol_only_field_with_kind_client_returns_failure` — e.g. `fqn_prefix` + `find_v2("client", …)`. +- Add `test_find_client_only_field_with_kind_symbol_returns_failure` — e.g. `client_kind` + `find_v2("symbol", …)`. +- Add `test_search_unknown_filter_key_returns_failure` (monkeypatch `run_search` to cheap rows). +- Add `test_search_cross_kind_filter_returns_failure` — e.g. `path_prefix` or `client_kind` on `search_v2` (symbol applicability). +- Keep / assert regressions: `test_find_symbol_empty_filter_handles_non_declaration_symbol_kinds`, `test_find_symbol_by_role` or explicit `test_find_symbol_fqn_prefix_still_honored` if not redundant; `test_search_filter_accepts_json_string`, `test_neighbors_filter_accepts_json_string`. +- Add `test_neighbors_filter_unknown_key_returns_failure` and `test_neighbors_filter_cross_kind_on_neighbor_returns_failure` — use fixture graph helpers (`_method_id_with_calls`, etc.) to force a neighbor whose kind makes a populated field inapplicable (e.g. `path_prefix` with a symbol neighbor, or `fqn_prefix` with a client neighbor). Assert **`success=False`** and **no** `ValidationError` for filter unknown keys. +- Add `test_neighbors_validate_call_still_raises` — quick smoke that invalid `edge_types` / missing `direction` still `pytest.raises(ValidationError)` (unchanged). + +## Tests for PR-N1 + +1. `test_find_cross_kind_filter_fields_return_failure` (replaces former silent-ignore test; exact name negotiable but must assert loud failure). +2. `test_find_unknown_filter_key_returns_failure` +3. `test_find_symbol_only_field_with_kind_client_returns_failure` +4. `test_find_client_only_field_with_kind_symbol_returns_failure` +5. `test_search_unknown_filter_key_returns_failure` +6. `test_search_cross_kind_filter_returns_failure` +7. `test_neighbors_filter_unknown_key_returns_failure` +8. `test_neighbors_filter_cross_kind_on_neighbor_returns_failure` +9. `test_neighbors_validate_call_still_raises` +10. Regression carries: `test_find_symbol_empty_filter_handles_non_declaration_symbol_kinds`, `test_search_filter_accepts_json_string`, `test_neighbors_filter_accepts_json_string`, existing role / `fqn_prefix` coverage as appropriate. + +## Definition of done (PR-N1) + +- [ ] `NodeFilter` rejects unknown keys; `find_v2` / `search_v2` / `neighbors_v2` never silently ignore them. +- [ ] Cross-kind populated fields produce `success=False` with teaching messages for `find` + `search`; `neighbors` fails on first neighbor row where applicability breaks. +- [ ] `neighbors_v2` filter `ValidationError` is wrapped to `success=False`; `@validate_call` errors still raise. +- [ ] `exclude_roles: []` does not trigger applicability errors. +- [ ] `.venv/bin/ruff check .` clean. +- [ ] `.venv/bin/python -m pytest tests/test_mcp_v2.py -v` (or full `tests -v` per `AGENTS.md`) passes. + +## Implementation step list + +| # | Step | File(s) | Done when | +| --- | --- | --- | --- | +| 1 | Add `ConfigDict(extra="forbid")` to `NodeFilter` | `mcp_v2.py` | Unknown key raises `ValidationError` at `model_validate` | +| 2 | Implement populated-field + applicability helpers | `mcp_v2.py` | Unit-testable pure functions; docstring notes alignment with `_node_matches_filter` | +| 3 | Wire `find_v2` / `search_v2` applicability + wrap filter `ValidationError` | `mcp_v2.py` | New find/search tests pass | +| 4 | Refactor `neighbors_v2` try/except boundaries; per-neighbor applicability; wrap filter `ValidationError` | `mcp_v2.py` | New neighbors tests pass; validate_call tests still raise | +| 5 | Update MCP descriptions + `_INSTRUCTIONS` | `server.py` | Wording matches new contract | +| 6 | README minimal filter contract note | `README.md` | Matches server copy | +| 7 | Replace/add tests per table above | `tests/test_mcp_v2.py` | All green | + +--- + +## Cross-PR risks and mitigations + +| # | Risk | Severity | Mitigation | +| --- | --- | --- | --- | +| 1 | `neighbors_v2` accidentally wraps non-filter `ValidationError` | Medium | Narrow `except ValidationError` to the filter parse block only; keep outer `raise` for `validate_call`. | +| 2 | Applicability set drifts from `_node_matches_filter` after a future edit | Medium | Code comment + single helper used by tools; consider a one-line assertion in tests that symbol applicability includes `fqn_prefix`. | +| 3 | Agents relied on silent ignore | Low (intentional break) | Message lists applicable fields; README + tool `description` teach the contract. | + +## Out of scope + +- Full #117 strict / permissive / hybrid vocabulary frame. +- New tools, `NodeFilter` field renames, cross-kind aliasing. +- `search.query` ranking / fuzzy behavior. +- `EdgeType` / `find.kind` literals / Kuzu schema. +- `describe_v2` (no `filter` today). +- Structured `hints` field on outputs (#120) — messages should remain plain string until that work lands. + +## Whole-plan done definition + +1. Proposal behavior (items 1–3 + `_coerce_filter` preserved) is implemented in `mcp_v2.py` and covered by tests. +2. Public MCP strings (`server.py`, optional `README.md`) match the new contract. +3. Ruff + pytest (at least `tests/test_mcp_v2.py`, preferably full `tests`) pass. + +## Tracking + +- `PR-N1`: _pending_ +- Per-PR Cursor prompts: [`plans/CURSOR-PROMPTS-LOSSLESS-PERMISSIVE-NODEFILTER.md`](./CURSOR-PROMPTS-LOSSLESS-PERMISSIVE-NODEFILTER.md)