From 18baa501c9c49d98227c87ff05d7ed90a1565b0f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 10 Jun 2026 20:53:54 +0200 Subject: [PATCH] test: address round-2 claude-review on #883 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pact.yml: guard `can-i-deploy` job on `env.PACT_BROKER != ''` so a secret rotation/fork can't break every master merge (the CLI errors on empty URL) - pact.yml: pin install.sh to the v2.6.1 commit SHA (immune to tag force-push) - astrolabe_client.py: `_token_cache` Optional[dict] -> `dict | None` and drop the now-unused `Optional` import (CLAUDE.md union syntax) - add tests/unit/test_astrolabe_client.py: mocked unit coverage for get_background_sync_status field mapping (200 provisioned / 200 not-provisioned / 404) — the layer that would have caught the original silent app_password bug - consumer pact test: note the 404 branch is internal defensive handling (covered by the unit test), not a contract obligation Co-Authored-By: Claude Opus 4.8 --- .github/workflows/pact.yml | 9 +- nextcloud_mcp_server/auth/astrolabe_client.py | 3 +- .../test_astrolabe_credentials_consumer.py | 4 + tests/unit/test_astrolabe_client.py | 104 ++++++++++++++++++ 4 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_astrolabe_client.py diff --git a/.github/workflows/pact.yml b/.github/workflows/pact.yml index 3c88ba4e..e7625ce5 100644 --- a/.github/workflows/pact.yml +++ b/.github/workflows/pact.yml @@ -54,7 +54,7 @@ jobs: - name: Install Pact CLI if: ${{ env.PACT_BROKER != '' }} run: | - curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/v2.6.1/install.sh | bash + curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/f03e620e7552239b6ca59438c9beed9d1038c949/install.sh | bash # v2.6.1 echo "$PWD/pact/bin" >> "$GITHUB_PATH" - name: Publish pacts to broker @@ -130,7 +130,10 @@ jobs: name: can-i-deploy runs-on: ubuntu-latest needs: [consumer, provider] - if: ${{ github.ref == 'refs/heads/master' }} + # Gate on the broker secret too: the pact-broker CLI errors on an empty + # --broker-base-url, so without this guard a secret rotation/fork would + # break every master merge. + if: ${{ github.ref == 'refs/heads/master' && env.PACT_BROKER != '' }} steps: - name: Join tailnet uses: tailscale/github-action@6cae46e2d796f265265cfcf628b72a32b4d7cade # v3 @@ -141,7 +144,7 @@ jobs: - name: Install Pact CLI run: | - curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/v2.6.1/install.sh | bash + curl -fsSL https://raw.githubusercontent.com/pact-foundation/pact-ruby-standalone/f03e620e7552239b6ca59438c9beed9d1038c949/install.sh | bash # v2.6.1 echo "$PWD/pact/bin" >> "$GITHUB_PATH" - name: Can I deploy nextcloud-mcp-server? diff --git a/nextcloud_mcp_server/auth/astrolabe_client.py b/nextcloud_mcp_server/auth/astrolabe_client.py index 8c316da8..1e6da78b 100644 --- a/nextcloud_mcp_server/auth/astrolabe_client.py +++ b/nextcloud_mcp_server/auth/astrolabe_client.py @@ -7,7 +7,6 @@ and retrieve user app passwords for background sync operations. import logging import time -from typing import Optional from ..http import nextcloud_httpx_client @@ -38,7 +37,7 @@ class AstrolabeClient: self.nextcloud_host = nextcloud_host.rstrip("/") self.client_id = client_id self.client_secret = client_secret - self._token_cache: Optional[dict] = None # {access_token, expires_at} + self._token_cache: dict | None = None # {access_token, expires_at} async def get_access_token(self) -> str: """ diff --git a/tests/contract/test_astrolabe_credentials_consumer.py b/tests/contract/test_astrolabe_credentials_consumer.py index b0788758..5a5bb893 100644 --- a/tests/contract/test_astrolabe_credentials_consumer.py +++ b/tests/contract/test_astrolabe_credentials_consumer.py @@ -16,6 +16,10 @@ This pact pins the request shape and the two states the consumer branches on: The OAuth token fetch (:meth:`AstrolabeClient.get_access_token`) is stubbed so only the status call hits the Pact mock server. +The client's 404 branch is internal defensive handling (astrolabe always returns +200 for this endpoint), not a contract obligation, so it is covered by a unit +test (``tests/unit/test_astrolabe_client.py``) rather than a pact interaction. + See ADR-029 for the overall contract-testing architecture. """ diff --git a/tests/unit/test_astrolabe_client.py b/tests/unit/test_astrolabe_client.py new file mode 100644 index 00000000..27d53a97 --- /dev/null +++ b/tests/unit/test_astrolabe_client.py @@ -0,0 +1,104 @@ +"""Unit tests for ``AstrolabeClient.get_background_sync_status`` (ADR-029). + +The Pact consumer contract (``tests/contract/``) pins the wire shape; these +mocked tests pin the *field mapping* that the original silent bug got wrong — +it read a non-existent ``app_password`` field, so ``has_access`` was always +``False``. A regression here now fails fast at the unit layer. +""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient + +pytestmark = pytest.mark.unit + + +def _patch_outbound_client(mocker, response: MagicMock) -> MagicMock: + """Patch the httpx client used by AstrolabeClient; return the mock client.""" + mock_client = AsyncMock() + mock_client.get = AsyncMock(return_value=response) + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=False) + mocker.patch( + "nextcloud_mcp_server.auth.astrolabe_client.nextcloud_httpx_client", + MagicMock(return_value=mock_client), + ) + return mock_client + + +def _client(mocker) -> AstrolabeClient: + client = AstrolabeClient( + nextcloud_host="https://cloud.example.com", + client_id="mcp", + client_secret="secret", + ) + mocker.patch.object(client, "get_access_token", AsyncMock(return_value="tok")) + return client + + +async def test_provisioned_user_maps_status_fields(mocker): + """200 + has_background_access=True maps to has_access/credential_type/provisioned_at.""" + response = MagicMock() + response.status_code = 200 + response.json.return_value = { + "success": True, + "user_id": "alice", + "has_background_access": True, + "sync_type": "app_password", + "provisioned_at": 1717000000, + } + mock_client = _patch_outbound_client(mocker, response) + client = _client(mocker) + + status = await client.get_background_sync_status("alice") + + assert status == { + "has_access": True, + "credential_type": "app_password", + "provisioned_at": 1717000000, + } + response.raise_for_status.assert_called_once() + # The bearer token from get_access_token is forwarded on the request. + _, kwargs = mock_client.get.call_args + assert kwargs["headers"]["Authorization"] == "Bearer tok" + + +async def test_provisioned_false_reports_no_access(mocker): + """200 + has_background_access=False reports no access (the bug's regression guard).""" + response = MagicMock() + response.status_code = 200 + response.json.return_value = { + "success": True, + "user_id": "bob", + "has_background_access": False, + "sync_type": None, + "provisioned_at": None, + } + _patch_outbound_client(mocker, response) + client = _client(mocker) + + status = await client.get_background_sync_status("bob") + + assert status["has_access"] is False + assert status["credential_type"] is None + assert status["provisioned_at"] is None + + +async def test_missing_credentials_404_returns_no_access(mocker): + """404 short-circuits to no-access without touching raise_for_status/json.""" + response = MagicMock() + response.status_code = 404 + _patch_outbound_client(mocker, response) + client = _client(mocker) + + status = await client.get_background_sync_status("carol") + + assert status == { + "has_access": False, + "credential_type": None, + "provisioned_at": None, + } + response.raise_for_status.assert_not_called() + response.json.assert_not_called()