From a03bc0af66b603741d78e827ef150a649eee0fd0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 5 Jun 2026 01:18:58 +0200 Subject: [PATCH] fix(review): guard placeholder in scanner reconcile + test dual-write path Round-1 review follow-ups (PR #857): - scanner.py: skip the rename-reconcile when the existing metadata point is a placeholder. reconcile_document_path only touches real chunks, so a not-yet- indexed file would just incur a 0-point set_payload; the real index writes the current path anyway. - test_sharing_state.py: add a dedup-hit case where the file was renamed AND the user is new to the ACL, asserting both set_payload writes fire (file_path/title and acl_principals). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/scanner.py | 9 +++++++-- tests/unit/vector/test_sharing_state.py | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 90c232dd..755fbf8a 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -587,13 +587,18 @@ async def scan_user_documents( ) ) file_queued += 1 - elif existing_metadata is not None: + elif existing_metadata is not None and not existing_metadata.get( + "is_placeholder", False + ): # Unchanged content (not re-queued) but the file may have # been renamed/moved: a rename keeps the fileid while # changing the path, and the dedup miss here means the # etag changed without a modified_at bump. Refresh the # stale path/title metadata without re-embedding. No-op - # when the path is unchanged. + # when the path is unchanged. Skip placeholders: reconcile + # only touches real chunks, so a not-yet-indexed file would + # just incur a 0-point set_payload (the real index writes + # the current path anyway). try: await reconcile_document_path( file_id, diff --git a/tests/unit/vector/test_sharing_state.py b/tests/unit/vector/test_sharing_state.py index 22774b26..8bb448dc 100644 --- a/tests/unit/vector/test_sharing_state.py +++ b/tests/unit/vector/test_sharing_state.py @@ -210,6 +210,31 @@ class TestClaimExistingIndex: assert payload["file_path"] == "/a/new.pdf" assert payload["title"] == "new.pdf" + async def test_dedup_hit_reconciles_and_grants_new_principal(self, client) -> None: + # Renamed file (stale path) AND a user not yet in the ACL: both writes + # fire — one set_payload for file_path/title, one for acl_principals. + client.scroll.return_value = ( + [ + _point( + { + payload_keys.EMBEDDING_IDENTITY: _MODEL, + ss.ACL_PRINCIPALS_KEY: ["user:alice"], + "file_path": "/a/old.pdf", + } + ) + ], + None, + ) + claimed = await ss.claim_existing_index( + "42", "file", "abc", "bob", current_path="/a/new.pdf" + ) + assert claimed is True + assert client.set_payload.await_count == 2 + payloads = [c.kwargs["payload"] for c in client.set_payload.await_args_list] + # One write refreshes the path/title, the other unions the new principal. + assert {"file_path": "/a/new.pdf", "title": "new.pdf"} in payloads + assert {ss.ACL_PRINCIPALS_KEY: ["user:alice", "user:bob"]} in payloads + async def test_dedup_hit_without_current_path_skips_reconcile(self, client) -> None: # No current_path (non-file callers) -> never touches file_path/title. client.scroll.return_value = (