fix: close search API review findings

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
 

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
 

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`.

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

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:
 

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.

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)

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)))

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:

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))
     )

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)

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:

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}