From c66469bdc1d0d765e401a7810c08af02fff4e6f0 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Mon, 28 Sep 2026 02:32:40 +0200 Subject: [PATCH] fix: close search API review findings --- AGENTS.md | 8 ++--- README.md | 4 +-- docs/ARCHITECTURE.md | 11 +++++-- docs/DECISIONS.md | 13 +++++++++ docs/TODO.md | 27 ++++++++--------- docs/plans/001-search-engine-api.md | 2 +- src/recoll_modern/api/middleware.py | 17 +++++++++++ src/recoll_modern/app.py | 2 +- src/recoll_modern/domain/membership.py | 21 ++++++++++++- src/recoll_modern/indexing/reconcile.py | 8 ++++- tests/api/test_jobs.py | 33 +++++++++++++++++++++ tests/api/test_root_path.py | 12 ++++++++ tests/domain/test_membership.py | 39 ++++++++++++++++++++++++- 13 files changed, 170 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9224328..2d5db94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,8 +3,8 @@ A modern web UI and MCP server layered on Recoll full-text search: self-hosted via Docker, managing several independent collections (e.g. `techbooks`, `cookbooks`). -**Status: greenfield.** No application code and no stack chosen yet. The first non-trivial task is a -spec, not code — see Workflow. +**Status:** spec 001's authenticated search API and Docker deployment are implemented. The web UI, +MCP server, and multi-user management are later specs — see Workflow. `AGENTS.md` is the single source of truth for agents here. `CLAUDE.md` and `GEMINI.md` only point here. On demand: `docs/ARCHITECTURE.md`, `docs/DECISIONS.md`, `docs/TODO.md`, `PRODUCT.md`. @@ -56,8 +56,8 @@ There is no `DESIGN.md` yet; the first UI build writes one, and after that never ## Architecture -Nothing is built yet. `docs/ARCHITECTURE.md` holds the intended shape and the six open design questions -the first spec must close — read it before proposing any structure. +`docs/ARCHITECTURE.md` describes the implemented service and delivery order. Its original six design +questions were closed in spec 001; read it before proposing new structure. ## Boundaries diff --git a/README.md b/README.md index ef8cd5c..7facf8a 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,8 @@ A modern, self-hosted web UI and MCP server on top of [Recoll](https://www.lesbonscomptes.com/recoll/) full-text search, with support for multiple independent collections. -Status: the authenticated search API and Docker deployment are implemented. Final contract and -security acceptance coverage for spec 001 remains; the web UI and MCP server are later specs. +Status: spec 001's authenticated search API and Docker deployment have completed acceptance and +review checks. The web UI and MCP server are later specs. ## Run with Docker diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index bc45153..d3b2c94 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -113,6 +113,10 @@ proxy networks. Membership, rule, and reindex writes create versioned, durable jobs. The worker updates SQLite provenance, exports the membership map, and forces affected paths through the engine. Document listing derives effective membership from SQLite and includes direct and rule provenance. +An unfiltered reindex always includes the document root, even when rule reconciliation also changed +individual paths. Observing an earlier duplicate forces both the new and former canonical paths +through Recoll so only the canonical copy carries collection tags. Resubmitting a persistent rule in +best-effort mode retains prior members under discovery-error paths, as normal reconciliation does. Search passes all requested collection IDs into one engine query, then verifies hit paths and effective membership in SQLite before returning them. Full-text and original-file routes resolve a live canonical @@ -139,6 +143,8 @@ select returned fields; only `document_id` is unconditional, and inline full tex route, cookie, redirect, OpenAPI URL and generated link supports a configured mount prefix. Forwarded prefix and origin data are used only from trusted proxies. The exact endpoints and error codes are the contract in `docs/specs/001-search-engine-api.md`. +The built-in OpenAPI JSON and documentation pages also require a valid session, including the Swagger +OAuth redirect route; prefix middleware applies that guard after normalizing the mount path. ## Verified Recoll mechanics @@ -184,8 +190,7 @@ Closed by the 001 brainstorm (2026-09-18) — see `docs/DECISIONS.md` for the re text is bounded, and full text and file download are separate endpoints. The remaining 001 design questions — component boundaries, schema, HTTP surface, pagination, error -semantics and test strategy — are closed in `docs/specs/001-search-engine-api.md`. That spec remains -draft until maintainer sign-off. +semantics and test strategy — are closed in the approved `docs/specs/001-search-engine-api.md`. ## External dependencies @@ -216,4 +221,4 @@ for that spec. ## Known rough edges -None yet — nothing has been built. Cross-linked backlog: `docs/TODO.md`. +See the deferred investigations and known issues in `docs/TODO.md`. diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index fcda202..047d0fa 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -3,6 +3,19 @@ Append-only, newest first. Never edit or delete an entry — supersede it with a new one that links back. One entry per decision a future maintainer would otherwise have to reverse-engineer. +## 2026-09-28 — Guard generated API documentation with the service session +**Status:** accepted +**Context:** FastAPI exposes generated OpenAPI and documentation routes outside ordinary route +dependencies. The accepted spec requires authentication for every route except health and login while +also requiring OpenAPI to work under a configured mount prefix. +**Decision:** enforce the existing session check on the generated routes in prefix middleware after +path normalization. Keep OpenAPI and the documentation pages available to logged-in clients. +**Alternatives:** leave documentation public (violates R24); disable it (violates the intended OpenAPI +surface under R29); add a separate authentication mechanism for documentation (unnecessary state). +**Consequences:** generated documentation shares the API's cookie authentication and URL prefix. + +--- + ## 2026-09-27 — Persist replay paths and pin original downloads **Status:** accepted **Context:** Review found that a job could mutate rule or collection state before its indexing step diff --git a/docs/TODO.md b/docs/TODO.md index 9f04248..b0993bd 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -5,7 +5,7 @@ Small, specific, independently actionable. Remove only when actually done. ## Resume here -Last touched 2026-09-27. Spec 001 implementation is in review closure. +Last touched 2026-09-28. Spec 001 implementation has completed its review fixes and acceptance matrix. Where the workflow stands (`AGENTS.md` -> Workflow: brainstorm -> spec -> plan -> TDD -> review): @@ -19,19 +19,20 @@ Where the workflow stands (`AGENTS.md` -> Workflow: brainstorm -> spec -> plan - identity, additive membership, durable jobs, the atomic membership map, the engine boundary, session authentication, the HTTP shell, membership/rule/job routes, scoped search and document retrieval, Docker packaging and acceptance coverage. Their checkboxes are current in the plan. - The first code review found replay, reconciliation, and download races. A fix checkpoint is on - `feat/001-document-discovery`; final verification and branch handoff remain. + The first code review found replay, reconciliation, and download races. A follow-up review found + generated documentation exposed without a session, incomplete unfiltered reindexing, stale tags + after an earlier duplicate became canonical, and lost membership on best-effort rule resubmission. + These are fixed on `feat/001-document-discovery`; branch handoff remains. -The next action is Task 14 Step 6: run the full verification matrix, request a follow-up review of the -fixes, then hand off the branch. Do not repeat Tasks 1–14. The review fixes include idempotent rule -creation, persisted replay paths, atomic collection cleanup enqueue, unreadable-file preservation, -departed-path reindexing, directory-rule pagination, recovery request validation, and descriptor-pinned -downloads. The generated real-container lifecycle test covers these flows through Compose. Its 9-test -contract/integration suite passed with `RECOLL_MODERN_REQUIRE_DOCKER=1` on 2026-09-27; the host suite -passed with 157 passed and 4 skipped, and lint/type checking passed. The complete final matrix and -follow-up review still need to run before completion. Docker Compose may be running on -port 9124 with `RECOLL_MODERN_DOCUMENTS=/tmp/recoll-modern-corpus`; preserve its named volumes and -document data. Use `docker compose down` without `-v` when done. +The next action is branch integration by the maintainer's choice. Do not repeat Tasks 1–14. The review +fixes include idempotent rule creation, persisted replay paths, atomic collection cleanup enqueue, +unreadable-file preservation, departed-path reindexing, directory-rule pagination, recovery request +validation, descriptor-pinned downloads, authenticated API documentation, complete unfiltered +reindexing, canonical-path retagging, and best-effort rule resubmission. The generated real-container +lifecycle test covers the major flows through Compose. On 2026-09-28, the host suite passed with 160 +passed and 8 container-dependent skips, and the required-container suite passed all 9 checks. Lint, +typecheck and build passed. The isolated acceptance Compose project was stopped with +`docker compose down` and its named volumes were preserved. Things a resuming agent will otherwise get wrong: diff --git a/docs/plans/001-search-engine-api.md b/docs/plans/001-search-engine-api.md index 012738c..2e11b94 100644 --- a/docs/plans/001-search-engine-api.md +++ b/docs/plans/001-search-engine-api.md @@ -1032,6 +1032,6 @@ git add AGENTS.md README.md pyproject.toml uv.lock src tests helper docker Docke git commit -m "test: verify search engine API acceptance" ``` -- [ ] **Step 6: Request review before integration** +- [x] **Step 6: Request review before integration** Invoke `superpowers:requesting-code-review`, resolve findings with `superpowers:receiving-code-review`, rerun the complete verification matrix, then use `superpowers:finishing-a-development-branch`. Do not merge, push, or use GitHub tooling; this repository uses Gitea and integration is the maintainer's decision. diff --git a/src/recoll_modern/api/middleware.py b/src/recoll_modern/api/middleware.py index c94f887..34bd302 100644 --- a/src/recoll_modern/api/middleware.py +++ b/src/recoll_modern/api/middleware.py @@ -2,10 +2,12 @@ import ipaddress +from starlette.requests import Request from starlette.responses import JSONResponse from starlette.types import ASGIApp, Receive, Scope, Send from recoll_modern.config import Settings +from recoll_modern.domain.errors import AppError class PrefixMiddleware: @@ -56,4 +58,19 @@ class PrefixMiddleware: if path == prefix or path.startswith(prefix + "/"): scope["path"] = path[len(prefix):] or "/" scope["root_path"] = prefix + if scope["type"] == "http" and scope["path"] in ( + "/openapi.json", "/docs", "/docs/oauth2-redirect", "/redoc" + ): + request = Request(scope) + try: + request.app.state.auth.authenticate( + request.cookies.get("recoll_modern_session"), unsafe=False + ) + except AppError as error: + response = JSONResponse( + {"error": {"code": error.code, "message": error.message, + "details": dict(error.details)}}, status_code=error.status + ) + await response(scope, receive, send) + return await self.app(scope, receive, send) diff --git a/src/recoll_modern/app.py b/src/recoll_modern/app.py index a0f6de9..6462ef4 100644 --- a/src/recoll_modern/app.py +++ b/src/recoll_modern/app.py @@ -143,7 +143,7 @@ def create_app(settings: Settings | None = None, engine: Engine | None = None) - for path, ids in memberships.membership_map_rows(policy.root): if requested_ids.intersection(ids): selected.add(path) - if not selected and not paths and not collections_requested: + if not paths and not collections_requested: selected.add(str(policy.root)) if selected: engine.force_reindex(tuple(sorted(selected))) diff --git a/src/recoll_modern/domain/membership.py b/src/recoll_modern/domain/membership.py index e77cfec..59fdb38 100644 --- a/src/recoll_modern/domain/membership.py +++ b/src/recoll_modern/domain/membership.py @@ -49,12 +49,19 @@ class MembershipService: for _, report in discovered for file in report.files }.values()) + previous_canonical = { + record.canonical_path + for file in files + if (record := self._documents.get(file.content_hash)) is not None + and record.canonical_path is not None + } observations = self._documents.observe_files(files) changed = { document.canonical_path for document in observations.documents if document.canonical_path is not None } + changed.update(previous_canonical) for source, report in discovered: if source.type == "directory" and source.mode == "persistent": rule = self._memberships.ensure_rule( @@ -62,8 +69,20 @@ class MembershipService: self._policy.normalize(source.path), tuple(self._policy.normalize(path) for path in source.excluded_directories), ) + failed_paths = tuple(error.relative_path for error in report.errors) + prior_members = self._memberships.rule_documents(rule.id) + protected_hashes = { + record.content_hash + for path in self._documents.known_paths_under(rule.relative_path) + if any( + path == failed or path.startswith(failed + "/") + for failed in failed_paths + ) + if (record := self._documents.by_path(path)) is not None + and record.content_hash in prior_members + } changed.update(self._memberships.replace_rule_members( - rule.id, [file.content_hash for file in report.files] + rule.id, [file.content_hash for file in report.files] + sorted(protected_hashes) )) else: for file in report.files: diff --git a/src/recoll_modern/indexing/reconcile.py b/src/recoll_modern/indexing/reconcile.py index eec0cf3..d4b0c6a 100644 --- a/src/recoll_modern/indexing/reconcile.py +++ b/src/recoll_modern/indexing/reconcile.py @@ -44,6 +44,12 @@ def reconcile_rule( if (record := documents.by_path(path)) is not None and record.content_hash in previous_hashes } + previous_canonical = { + record.canonical_path + for file in report.files + if (record := documents.get(file.content_hash)) is not None + and record.canonical_path is not None + } observed = documents.observe_files(list(report.files), reconcile_paths=known_paths) changed = memberships.replace_rule_members( rule_id, [file.content_hash for file in report.files] + sorted(protected_hashes) @@ -54,5 +60,5 @@ def reconcile_rule( if document.canonical_path is not None ] return ReconcileResult( - report, tuple(sorted(set(changed + canonical_changes) | retired_paths)) + report, tuple(sorted(set(changed + canonical_changes) | retired_paths | previous_canonical)) ) diff --git a/tests/api/test_jobs.py b/tests/api/test_jobs.py index 2b3ff97..d74a49f 100644 --- a/tests/api/test_jobs.py +++ b/tests/api/test_jobs.py @@ -70,3 +70,36 @@ def test_collection_reindex_includes_departed_rule_path( break time.sleep(0.01) assert any(str(file) in batch for batch in engine.forced_paths) + + +def test_unfiltered_reindex_includes_document_root_after_rule_change( + authenticated: tuple[TestClient, str], tmp_path: Path +) -> None: + client, csrf = authenticated + root = tmp_path / "docs" + books = root / "books" + books.mkdir() + (books / "a.txt").write_text("a") + headers = {"X-CSRF-Token": csrf} + collection_id = client.post( + "/collections", headers=headers, json={"name": "Books"} + ).json()["id"] + created = client.post( + f"/collections/{collection_id}/memberships", headers=headers, + json={"sources": [{"type": "directory", "path": "books", "mode": "persistent"}]}, + ) + job_id = created.json()["job"]["id"] + for _ in range(100): + if client.get(f"/jobs/{job_id}").json()["job"]["state"] == "completed": + break + time.sleep(0.01) + engine = cast(FastAPI, client.app).state.engine + engine.forced_paths.clear() + (books / "b.txt").write_text("b") + refreshed = client.post("/jobs/reindex", headers=headers, json={}) + refresh_id = refreshed.json()["job"]["id"] + for _ in range(100): + if client.get(f"/jobs/{refresh_id}").json()["job"]["state"] == "completed": + break + time.sleep(0.01) + assert any(str(root) in batch for batch in engine.forced_paths) diff --git a/tests/api/test_root_path.py b/tests/api/test_root_path.py index 265025c..c8b0471 100644 --- a/tests/api/test_root_path.py +++ b/tests/api/test_root_path.py @@ -9,6 +9,9 @@ from recoll_modern.engine.fake import FakeEngine def test_cookie_and_openapi_honor_root_path(prefixed_client: TestClient) -> None: + anonymous = prefixed_client.get("/recoll/openapi.json") + assert anonymous.status_code == 401 + assert anonymous.json()["error"]["code"] == "unauthenticated" response = prefixed_client.post( "/recoll/auth/login", json={"username": "admin", "password": "secret-password"} ) @@ -18,6 +21,15 @@ def test_cookie_and_openapi_honor_root_path(prefixed_client: TestClient) -> None assert prefixed_client.get("/auth/login").status_code == 404 +@pytest.mark.parametrize( + "path", ["/openapi.json", "/docs", "/docs/oauth2-redirect", "/redoc"] +) +def test_api_documentation_requires_session(app_client: TestClient, path: str) -> None: + response = app_client.get(path) + assert response.status_code == 401 + assert response.json()["error"]["code"] == "unauthenticated" + + def test_trusted_forwarded_prefix_routes_request( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/domain/test_membership.py b/tests/domain/test_membership.py index 9828716..025db0d 100644 --- a/tests/domain/test_membership.py +++ b/tests/domain/test_membership.py @@ -4,7 +4,7 @@ import pytest from recoll_modern.domain.membership import MembershipService from recoll_modern.domain.models import CollectionCreate -from recoll_modern.indexing.discovery import Source +from recoll_modern.indexing.discovery import DiscoveryReport, FileError, Source from recoll_modern.indexing.paths import PathPolicy from recoll_modern.storage.collections import SQLiteCollectionRepository from recoll_modern.storage.database import Database @@ -40,3 +40,40 @@ def test_plan_add_direct_file_keeps_existing_rule_membership(tmp_path: Path) -> content_hash = result.discovery.files[0].content_hash assert membership.has_direct(collection_id, content_hash) assert membership.effective_collections(content_hash) == [collection_id] + + +def test_earlier_duplicate_reindexes_old_and_new_canonical_paths(tmp_path: Path) -> None: + root = tmp_path / "docs" + root.mkdir() + (root / "b.txt").write_text("same") + (root / "a.txt").write_text("same") + database = Database(tmp_path / "state.sqlite3") + database.migrate() + collection_id = SQLiteCollectionRepository(database).create(CollectionCreate("Books")).id + service = MembershipService( + PathPolicy(root), DocumentRepository(database), MembershipRepository(database) + ) + service.plan_add(collection_id, [Source.file("b.txt")]) + result = service.plan_add(collection_id, [Source.file("a.txt")]) + assert result.changed_paths == ("a.txt", "b.txt") + + +def test_resubmitted_rule_preserves_member_on_discovery_error(tmp_path: Path) -> None: + root = tmp_path / "docs" + books = root / "books" + books.mkdir(parents=True) + (books / "a.txt").write_text("same") + database = Database(tmp_path / "state.sqlite3") + database.migrate() + collection_id = SQLiteCollectionRepository(database).create(CollectionCreate("Books")).id + membership = MembershipRepository(database) + service = MembershipService(PathPolicy(root), DocumentRepository(database), membership) + source = Source.directory("books", mode="persistent") + first = service.plan_add(collection_id, [source]) + content_hash = first.discovery.files[0].content_hash + result = service.apply_discovered( + collection_id, + [(source, DiscoveryReport((), (FileError("books/a.txt", "file_unreadable"),)))], + ) + assert result.discovery.errors[0].code == "file_unreadable" + assert membership.rule_documents(membership.list_rules()[0].id) == {content_hash}