diff --git a/docs/plans/002-point-crud-and-keyword-search.md b/docs/plans/002-point-crud-and-keyword-search.md new file mode 100644 index 0000000..79fd813 --- /dev/null +++ b/docs/plans/002-point-crud-and-keyword-search.md @@ -0,0 +1,285 @@ +# 002. Point CRUD and keyword-search implementation plan + +## Purpose + +This plan covers the milestone named at the end of +[plan 001](001-ingestion-vertical-slice.md): direct, fine-grained management of +individual Qdrant points through `/v1/points`, plus filter/keyword search over +them. Ingestion (plan 001) writes points in bulk; this slice lets a human or +admin frontend read, edit, reorder, and soft-delete them one at a time, under +the same tenant isolation. + +This is an implementation plan, not an Architecture Decision Record. The ADRs +explain why the collection schema, endpoints, and isolation rules are what they +are; this document defines order, scope, and verification criteria. + +## Prerequisite + +Plan 001 must be complete through **Phase 5** before Phase 3 of this plan +starts. Specifically this plan depends on: the `chunks` collection and its +payload indexes actually existing, API-key authentication and `AuthContext` +tenant derivation, the application-lifetime Qdrant client from the FastAPI +lifespan, and the request-lifetime `AsyncSession` wiring. Phases 1–2 below +(schemas and the read paths) can be built against the collection alone and do +not need the full ingestion path. + +## Architecture baseline + +| System | Responsibility | +|---|---| +| FastAPI | HTTP boundary, auth, tenant derivation, request/response schemas, scope checks. | +| Points application service | Point CRUD semantics: soft delete, ordering, neighbor relinking, optimistic concurrency. | +| Qdrant | The only store for point content, vectors, and payload. | +| Postgres | Audit of mutating operations. It does **not** hold a mirror of point state. | + +The controlling ADRs are: + +- [ADR-0001](../adr/0001-ingestion-pipeline-and-collection-schema.md): the + `chunks` collection, payload schema, payload indexes, deterministic point IDs, + `order_id` as a fractional float, and the + `previous_chunk_id`/`next_chunk_id` adjacency pointers. +- [ADR-0002](../adr/0002-chunk-crud-and-search-api.md): the CRUD operation set, + soft-delete-by-default, reorder semantics, keyword search vs. semantic search, + and `version`-based optimistic concurrency. +- [ADR-0008](../adr/0008-rest-api-and-fastapi-boundary.md): the `/v1/points` + REST surface, `GET /v1/files/{file_id}/points`, + `DELETE /v1/files/{file_id}`, and the `points:read`/`points:write` scopes. +- [ADR-0012](../adr/0012-application-resource-lifetime-and-dependency-ownership.md): + the Qdrant client is application-lifetime and injected, never constructed per + request. +- [ADR-0015](../adr/0015-modular-monolith-package-architecture.md): routes call + `application/points/`, which calls a port implemented in + `infrastructure/qdrant/`. Routers never build Qdrant filters or call the SDK. +- [ADR-0016](../adr/0016-testing-strategy-and-quality-gates.md): unit tests + against a fake point port; Testcontainers Qdrant for the adapter. + +ADR-0001 through 0004 are Accepted; 0008, 0012, 0015, and 0016 are still +Proposed. Treat the proposed ones as the implementation baseline only once the +project owner accepts them, and update the ADR rather than diverging silently. + +## Scope + +### In scope + +- `POST /v1/points`, `GET /v1/points/{point_id}`, `PUT /v1/points/{point_id}`, + `PATCH /v1/points/{point_id}/payload`, `DELETE /v1/points/{point_id}`. +- `GET /v1/points?file_id=...` (scroll, `order_by: order_id`, paginated), + `GET /v1/points/count`, `GET /v1/points/search?q=...`. +- `PATCH /v1/points/{point_id}/order` with correct neighbor relinking. +- `POST /v1/points/batch` for bulk multi-operation edits. +- `GET /v1/files/{file_id}/points` and `DELETE /v1/files/{file_id}` + (bulk soft delete of a file's points), from ADR-0008. +- Soft delete as the default for every delete path, with neighbor relinking. +- Optimistic concurrency on every mutating path via the `version` payload field. +- Audit rows in Postgres for mutating operations. +- Automated tests for tenant isolation, pointer integrity, concurrency + conflicts, and pagination. + +### Explicitly out of scope + +- Hybrid dense+sparse retrieval, RRF fusion, and late-interaction rerank + (ADR-0003/0005) — that is the next plan, 003. `GET /v1/points/search` here is + **keyword/filter matching only**; do not let it grow a semantic mode. +- Re-embedding a point on content edit (see the open decision below). +- Hard deletion / compliance purge endpoints. +- The LangGraph conversational API (ADR-0006/0007). +- A frontend. + +## Required invariants + +1. `tenant_id` comes from the authenticated context and is injected into every + filter server-side — on reads *and* writes, on every code path. A + `tenant_id`, or any payload key that would override it, appearing in a + request body or query string is rejected, never honored. +2. Cross-tenant access returns `404`, not `403` — the same rule plan 001 applies + to files. A caller must not be able to probe for the existence of another + tenant's point IDs. +3. Soft delete is the default: `is_active=false` + `deleted_at` via + `set_payload`. Points are never removed from Qdrant by any endpoint in this + slice. +4. Reads exclude inactive points unless the caller explicitly opts in. +5. Any operation that changes a point's position or removes it from the sequence + — insert, reorder, delete — updates the affected neighbors' + `previous_chunk_id`/`next_chunk_id` in the **same** `points/batch` request. + A partial relink is a defect: ADR-0003's context-window expansion walks these + pointers. +6. Point IDs stay derived from `file_id` + the immutable `chunk_index`. + Reordering changes `order_id` only, never the point ID. +7. Every mutating operation is guarded by the `version` payload field via + Qdrant's `update_filter`, and increments it. A stale write returns `409`, + it does not silently clobber. +8. `order_id` is a fractional float. Inserting or moving a point assigns a value + between its two new neighbors; it never renumbers siblings. +9. Routers contain no Qdrant SDK calls and no filter construction. The Qdrant + client is injected from the lifespan (ADR-0012). + +## Decisions needed before the affected phase + +### Re-embedding on content edit (blocks Phase 4) + +`PUT /v1/points/{point_id}` can change `content`. The stored vectors then no +longer match the text. Three options, in order of preference: + +1. **Re-embed inline** on content change, reusing plan 001's embedding ports and + bounds. Consistent, but puts embedder latency and `502`/`504` failure modes + on an admin edit path. +2. **Require caller-supplied vectors** when content changes, and reject the edit + otherwise. Simple and honest, but pushes model knowledge to the client. +3. **Mark the point stale** (a payload flag) and re-embed later. Needs + background work, which ADR-0017 currently rules out. + +Default to (1) for parity with ingestion, with the same batch/semaphore bounds +and the same status codes. Record whichever is chosen in ADR-0002 before +implementing Phase 4 — this is a real behavioral contract, not an +implementation detail. + +### Fractional-key exhaustion + +ADR-0001 notes float keys eventually need renormalization. Decide now whether +this slice ships a renormalize path (an internal operation rewriting a file's +`order_id` values to `1000, 2000, 3000, ...`) or explicitly defers it with a +logged warning when the gap between neighbors falls under a threshold. Deferring +is acceptable; silently producing unrepresentable gaps is not. + +### Batch semantics + +`POST /v1/points/batch` must define, in the API schema and the tests: whether +operations are all-or-nothing, what happens when operation 3 of 5 fails a +version check, and the maximum operation count per request. Decide before +Phase 5; do not let the answer be "whatever Qdrant happened to do." + +## Build order + +### Phase 1: Point contracts and the port + +1. Define the payload model in `src/application/points/` mirroring ADR-0001's + field list exactly, with the reserved/server-owned fields (`tenant_id`, + `version`, `chunk_index`, `deleted_at`) separated from caller-writable ones. +2. Define the point port: get, upsert, set payload, scroll, count, keyword + search, batch. Tenant filter is a required parameter on every method — not an + optional argument a caller can forget. +3. Implement the Qdrant adapter behind the port in + `src/infrastructure/qdrant/`, using the injected application-lifetime client. +4. Add a fake port implementation for unit tests, with ordering and version + behavior faithful enough to test the service against. +5. Define the Pydantic request/response schemas in `src/api/schemas/`. Vectors + are returned only when explicitly requested (ADR-0008). + +**Exit criteria:** the service layer can be unit-tested end to end against the +fake; a Testcontainers Qdrant test confirms the adapter's filter construction +and `order_by` scroll match the fake's semantics. + +### Phase 2: Read paths + +1. `GET /v1/points/{point_id}` — tenant-filtered retrieve; `404` when the point + belongs to another tenant or does not exist. +2. `GET /v1/points?file_id=...` — scroll with `order_by: order_id`, stable + pagination, `is_active: true` implied. +3. `GET /v1/files/{file_id}/points` — same listing, addressed by file. +4. `GET /v1/points/count` — tenant/domain-filtered count. +5. `GET /v1/points/search?q=...` — full-text payload match on `content` plus + structured filters. Name the response and docstring so it cannot be mistaken + for semantic retrieval. +6. Enforce `points:read` scope on all of the above. +7. Tests: tenant isolation returns `404`; inactive points are excluded by + default and included on explicit opt-in; pagination does not skip or repeat + under a concurrent insert. + +**Exit criteria:** a tenant can list and search only its own active points, in +display order, with correct paging. + +### Phase 3: Soft delete and neighbor relinking + +1. Implement the relinking primitive in the service: given a point leaving the + sequence, compute the neighbor payload updates and emit them with the + deactivation in one `points/batch` call. +2. `DELETE /v1/points/{point_id}` — soft delete plus relink. +3. `DELETE /v1/files/{file_id}` — bulk soft delete of a file's active points. + The whole file leaves the sequence, so the boundary pointers must end + consistent (typically all null within that file). +4. Deleting an already-inactive point is a no-op success, not a `404` and not a + second relink. +5. Tests: after deleting a middle point, its old neighbors point at each other; + after deleting the first point, the new first has a null + `previous_chunk_id`; a full traversal of a file's pointer chain after a + series of deletes visits every active point exactly once and never enters an + inactive one. + +**Exit criteria:** no delete path can leave a stale or dangling pointer, and +none removes a point from Qdrant. + +### Phase 4: Create, replace, and payload update + +1. `POST /v1/points` — create one point. Server assigns `tenant_id`, `version`, + and the derived point ID; the caller supplies content, position, and + metadata. Insert relinks neighbors like a reorder does. +2. `PUT /v1/points/{point_id}` — upsert with `update_only`, guarded by + `version` via `update_filter`; `409` on a stale version. Apply the + re-embedding decision above. +3. `PATCH /v1/points/{point_id}/payload` — payload-only update. Reject attempts + to write server-owned fields. +4. Enforce `points:write` scope; write an audit row per mutation + (tenant, actor, point, operation, resulting version). +5. Tests: concurrent writers at the same version — one succeeds, one gets `409` + and does not mutate; a `tenant_id` or `version` in the request body is + rejected; a create lands in the right sequence position. + +**Exit criteria:** manual edits and ingestion re-runs cannot silently clobber +each other; server-owned fields are unwritable through any endpoint. + +### Phase 5: Reorder and batch + +1. `PATCH /v1/points/{point_id}/order` — assign a new fractional `order_id` + between the new neighbors and relink up to four points in one + `points/batch`, per ADR-0002. +2. Handle the boundary moves (to first, to last) and the no-op move + (already in that position). +3. Implement the fractional-gap decision from above. +4. `POST /v1/points/batch` with the semantics decided above, including the + per-request operation cap. +5. Tests: a randomized sequence of inserts, moves, and deletes leaves the + `order_id` ordering and the pointer chain agreeing with each other after + every step — this property test is the main defense for this phase. + +**Exit criteria:** display order derived from `order_id` and traversal order +derived from the pointer chain are identical for every file, after any sequence +of mutations. + +### Phase 6: Integration tests, operations, and documentation + +1. Testcontainers Qdrant integration tests for each endpoint's real filter and + ordering behavior, isolated per test by unique collection or tenant keys. +2. An end-to-end test crossing plan 001 and this slice: ingest a CSV, list its + points, reorder one, soft-delete another, re-upload the same file, and assert + the manual edits interact with re-ingestion exactly as ADR-0001/0002 specify. + If that interaction is not yet decided, this test is what forces the decision. +3. Structured logging at the mutation boundary with stable event names + (`points.updated`, `points.reordered`, `points.soft_deleted`) carrying + `request_id`, `tenant_id`, `file_id`, and the resulting version. +4. Extend the operator runbook: how to inspect a file's point sequence, how to + spot a broken pointer chain, and how to recover one. +5. Update the README and ADR-0002 with anything this implementation settled. + +**Exit criteria:** every endpoint has an integration test against real Qdrant, +and an operator can diagnose an ordering problem from the logs and the runbook. + +## Definition of done + +This slice is done when, in local Compose and under automated test: + +```text +ingest a CSV (plan 001) + -> GET /v1/points?file_id=... lists its active points in order_id order + -> PATCH .../order moves one point; order_id and the + previous/next chain still agree + -> PUT /v1/points/{id} edits content under a version guard; + a stale write gets 409 + -> DELETE /v1/points/{id} soft-deletes and relinks neighbors + -> GET /v1/points/search?q=... keyword-matches content within the tenant + -> every one of the above returns 404, not 403, for another tenant +``` + +The next plan (003) is agent hybrid retrieval — three prefetches, RRF fusion, +late-interaction rerank, and context-window expansion over the pointer chain +this slice is responsible for keeping correct (ADR-0003, ADR-0005). Do not pull +any of it into this plan.