fix(confluence): surface unverified restriction checks instead of failing open silently

docs/spec/S26-managed-elsewhere.md

@@ -38,8 +38,39 @@ layers don't run.
 
 If none match, the page is normally pushable.
 
+**Guarantees vs. advisory: layer 5 can fail, layers 1-4 cannot**
+
+Layers 1-4 are local comparisons against `ManagedConfig`, already loaded
+into the process before the cascade runs. Barring a config-loading bug,
+they always evaluate, so a match is a guarantee: once a space, subtree,
+account ID, or body pattern is configured, the matching page is
+unconditionally blocked from push, every time.
+
+Layer 5 is different — it is a network call
+(`GET /content/{id}/restriction`), and network calls fail: a transient
+Confluence error, an auth problem, a response shape the client doesn't
+recognize. This check is therefore advisory, not a guarantee: **it fails
+open**. If the call raises, `mdd` cannot tell whether the page is
+restricted, and rather than block every push during a Confluence outage
+or credential hiccup, it treats the page as pushable. The alternative —
+failing closed — would mean a degraded Confluence API blocks all
+publishing everywhere, which is a worse outcome than an occasional
+unverified restriction check.
+
+Failing open silently would be worse still, because restrictions are
+precisely the mechanism protecting pages somebody actively does not want
+overwritten. So when layer 5's API call fails, `mdd` logs a warning
+naming the page and the underlying exception, and — on `mdd confluence
+sync` — the run summary counts how many pages were pushed with the check
+unverified (see **Run summary additions** below). The check still fails
+open; the gap is now visible after the fact instead of indistinguishable
+from a clean pass.
+
 **Strictly fail-closed: no override mechanism**
 
+This is about what happens once a layer *has* matched — it is
+unrelated to layer 5's fail-open behaviour above, which is about
+what happens when the restriction check itself cannot run at all.
 A detected managed page **cannot be pushed via `mdd`**. There is
 no `--force-managed` flag, no `mdd.managed_override: true`
 frontmatter escape, no per-page bypass. The intended workflow is:
@@ -123,11 +154,20 @@ Skipped (managed elsewhere):
   - 4 pages from technical-documentation
   - 2 pages read-only restricted
 
+Restriction check unverified:
+  - 3 pages pushed without confirming update permission (Confluence restriction check failed)
+
 Mirror -> Confluence:
   (no actions; all candidates were managed-elsewhere)
 ```
 
-When the section is empty (no managed skips), it's omitted.
+When a section is empty (no managed skips, no unverified restriction
+checks), it's omitted. The "Restriction check unverified" count is
+distinct from "Skipped (managed elsewhere)": a skip means layer 5 ran
+and found a restriction; an unverified count means layer 5's API call
+failed and the page was pushed anyway, per the fail-open behaviour
+above. The same count is also logged as a warning by `mdd confluence
+sync` at the time it happens, not only in the eventual summary.
 
 **`mdd confluence whoami` helper**
 - `GET /wiki/api/v2/users/current` returns the authenticated
@@ -162,6 +202,20 @@ the same `classify_page` function is called from every push site
 --apply`). Each push site either errors out (single-page
 operations) or skips and records a summary entry (bulk sync).
 
+**Layer 5 fails open, visibly.** Layers 1-4 are guarantees because
+they never call out of the process; layer 5 calls Confluence and
+can fail, and failing closed there would mean a degraded
+Confluence API blocks all publishing, everywhere, which is worse
+than the gap it is meant to close. So layer 5 fails open — but the
+earlier design left that silent: an API error and a confirmed
+"unrestricted" result produced the identical `is_managed=False`.
+Since restrictions are the mechanism protecting pages someone
+actively does not want overwritten, that silence was the design's
+weakest point. `classify_page` now distinguishes the two outcomes
+and both the warning log and the sync run summary make the gap
+visible after the fact, without changing the underlying fail-open
+choice.
+
 **Pull-side stamping** runs on every exported page so readers see
 the "managed by X" header in the mirror itself, not only when
 they attempt to push. This makes the failure mode discoverable

src/mdd/commands/confluence.py

@@ -317,6 +317,12 @@ def _print_sync_summary(summary: SyncSummary) -> None:
             "Conflicts (skipped): %d — resolve manually with 'mdd confluence update-page'",
             len(summary.conflicts),
         )
+    if summary.restriction_check_unverified:
+        log.warning(
+            "Restriction check unverified: %d — pushed without confirming update "
+            "permission (Confluence restriction API call failed; see warnings above)",
+            summary.restriction_check_unverified,
+        )
     if summary.failures:
         log.error("Failures: %d", len(summary.failures))
         for f in summary.failures:

src/mdd/confluence/managed/classify.py

@@ -6,13 +6,17 @@
 from enum import StrEnum
 from typing import TYPE_CHECKING, Any
 
+from mdd.utils.logging import get_logger
+
 from ._api_coerce import dict_field, iter_dicts
 
 if TYPE_CHECKING:
     from collections.abc import Callable
 
     from .config import ManagedConfig, PublisherEntry
 
+log = get_logger(__name__)
+
 
 class ManagedReason(StrEnum):
     READ_ONLY = "READ_ONLY"
@@ -31,6 +35,13 @@ class ManagedClassification:
     publisher_name: str | None = None  # None for READ_ONLY
     source_url: str | None = None
     message: str | None = None  # rendered with substitutions
+    # True when layer 5 (page restrictions) could not be evaluated — the API
+    # call raised — and the cascade fell through to "not managed" as a
+    # result. Distinguishes "confirmed pushable" from "pushable because the
+    # check that would say otherwise didn't run." Always False when
+    # ``is_managed`` is True: an earlier layer's match, or a confirmed
+    # restriction, already answers the question.
+    restriction_check_unverified: bool = False
 
 
 @dataclass
@@ -75,16 +86,18 @@ def _hit_read_only() -> ManagedClassification:
     )
 
 
-def _not_managed() -> ManagedClassification:
-    return ManagedClassification(is_managed=False)
+def _not_managed(*, restriction_check_unverified: bool = False) -> ManagedClassification:
+    return ManagedClassification(
+        is_managed=False, restriction_check_unverified=restriction_check_unverified
+    )
 
 
 def _user_can_update(
     page_id: str,
     current_account_id: str,
     client: Any,  # ConfluenceClient — avoid circular import
-) -> bool:
-    """Return True if the current user is allowed to update *page_id*.
+) -> bool | None:
+    """Return whether the current user is allowed to update *page_id*.
 
     Calls ``GET /wiki/rest/api/content/{id}/restriction``.  If the "update"
     restriction type has an empty ``restrictions`` block, the page is
@@ -93,13 +106,18 @@ def _user_can_update(
     If the list is non-empty, returns True only if *current_account_id* is in
     the user list or is a member of one of the groups.
 
-    On any API error, returns True (fail-open for the restriction check; other
-    cascade layers are stronger).
+    Returns None, rather than raising or guessing, when the API call itself
+    fails — a transient error, an auth problem, or a response shape this
+    client doesn't understand. Callers treat None as fail-open (other
+    cascade layers are local comparisons and don't have this failure mode),
+    but unlike a real answer it is logged so an outage doesn't masquerade as
+    a clean permission check.
     """
     try:
         data = client.get_page_restrictions(page_id)
-    except Exception:
-        return True  # can't tell — assume allowed
+    except Exception as exc:
+        log.warning("could not check page restrictions for page %s: %s", page_id, exc)
+        return None  # can't tell — caller fails open, but now knows it happened
 
     # Shape: {"update": {"restrictions": {"user": {"results": [...]}, "group": {...}}}}
     restrictions = dict_field(dict_field(data, "update"), "restrictions")
@@ -173,6 +191,13 @@ def classify_page(
     4. body_marker_patterns → BODY_MARKER
     5. page_restrictions → READ_ONLY  (only when *check_restrictions* is True)
 
+    Layers 1-4 are local comparisons against *config* and cannot fail short
+    of a config-loading bug, so a match is a guarantee. Layer 5 calls the
+    Confluence API and can fail (network, auth, unexpected response shape);
+    when it does, this returns "not managed" with
+    ``restriction_check_unverified=True`` rather than guessing — see
+    :func:`_user_can_update`.
+
     Args:
         page:               Minimal page data required for classification.
         config:             Merged ManagedConfig (from :func:`load_managed_config`).
@@ -190,7 +215,11 @@ def classify_page(
 
     if check_restrictions:
         account_id = _resolve_account_id(client, current_account_id)
-        if account_id and not _user_can_update(page.page_id, account_id, client):
-            return _hit_read_only()
+        if account_id:
+            can_update = _user_can_update(page.page_id, account_id, client)
+            if can_update is None:
+                return _not_managed(restriction_check_unverified=True)
+            if not can_update:
+                return _hit_read_only()
 
     return _not_managed()

src/mdd/confluence/sync/_types.py

@@ -69,6 +69,12 @@ class SyncSummary:
     office_cache_hits: int = 0
     # managed-elsewhere skips: {publisher_name: count}
     managed_skips: dict[str, int] = field(default_factory=dict)
+    # Pages pushed even though the page-restrictions check (managed-elsewhere
+    # cascade layer 5) couldn't complete — a Confluence API error, not a
+    # confirmed "unrestricted" result. The check fails open so the push went
+    # ahead; this counter is how an operator sees, after the fact, that N
+    # pages were pushed without that check having actually run.
+    restriction_check_unverified: int = 0
     # Pages dropped by the ``.mddignore`` matcher before download.
     # ``skipped_ignored_paths`` records POSIX-style rel-paths (directories
     # carry a trailing ``/`` so dry-run output distinguishes prunes from
@@ -127,6 +133,14 @@ def _managed_skip_lines(self) -> list[str]:
             for publisher, count in self.managed_skips.items()
         ]
 
+    def _restriction_unverified_lines(self) -> list[str]:
+        if not self.restriction_check_unverified:
+            return []
+        return [
+            f"  - {self.restriction_check_unverified} pages pushed without confirming "
+            "update permission (Confluence restriction check failed)"
+        ]
+
     def format_commit_message(self, space_key: str, *, message_override: str | None = None) -> str:
         lines: list[str] = [
             message_override or f"chore(mirror): sync from Confluence space {space_key}",
@@ -141,6 +155,7 @@ def format_commit_message(self, space_key: str, *, message_override: str | None
                 [f"1 conflict (local + remote both edited): {p}" for p in self.conflicts],
             ),
             ("Skipped (managed elsewhere):", self._managed_skip_lines()),
+            ("Restriction check unverified:", self._restriction_unverified_lines()),
             ("Cross-space moves detected:", [f"- {note}" for note in self.cross_space]),
         ]
         for header, body in sections:

src/mdd/confluence/sync/events.py

@@ -101,6 +101,8 @@ def record_managed_skip(page_id: str, page_data: dict[str, Any]) -> bool:
         body_storage = extract_storage_body(page_data)
         page_info = build_page_info_from_page_data(page_data, body_storage)
         cl = classify_page(page_info, get_managed_cfg(), client)
+        if cl.restriction_check_unverified:
+            summary.restriction_check_unverified += 1
         if not cl.is_managed:
             return False
         warn_managed(page_id, cl)

src/mdd/confluence/sync/office_publish.py

@@ -52,6 +52,8 @@ def _record_office_managed_skip(
     """If page is managed elsewhere, record and return True (sync should skip)."""
     page_info = build_page_info_from_page_data(page_data, body_xhtml)
     cl = classify_page(page_info, managed_config, client)
+    if cl.restriction_check_unverified:
+        summary.restriction_check_unverified += 1
     if not cl.is_managed:
         return False
     warn_managed(page_id, cl, context="office-publish")

tests/confluence/sync/test_events.py

@@ -0,0 +1,84 @@
+"""Tests for the managed-elsewhere skip recording built by make_managed_helpers."""
+
+from __future__ import annotations
+
+from typing import Any
+from unittest.mock import MagicMock
+
+from mdd.confluence.managed import ManagedConfig
+from mdd.confluence.sync._types import SyncOptions, SyncSummary
+from mdd.confluence.sync.events import make_managed_helpers
+
+_PAGE_DATA: dict[str, Any] = {
+    "id": "111",
+    "spaceKey": "ENG",
+    "version": {"authorId": "human-456"},
+    "body": {"storage": {"value": "<p>hello</p>"}},
+}
+
+
+def _opts(config: ManagedConfig) -> SyncOptions:
+    return SyncOptions(managed_config=config)
+
+
+class TestRecordManagedSkip:
+    def test_restriction_api_error_pushes_and_counts_unverified(self) -> None:
+        """A restriction-check API error doesn't skip the page, but the summary counts it."""
+        client = MagicMock()
+        client.get_current_user.return_value = {"accountId": "my-account-id"}
+        client.get_page_restrictions.side_effect = Exception("network error")
+
+        summary = SyncSummary()
+        config = ManagedConfig()
+        _get_cfg, record_managed_skip = make_managed_helpers(client, _opts(config), summary)
+
+        skipped = record_managed_skip("111", _PAGE_DATA)
+
+        assert skipped is False  # fails open: sync proceeds to push
+        assert summary.restriction_check_unverified == 1
+        assert summary.managed_skips == {}
+
+    def test_confirmed_unrestricted_page_not_counted(self) -> None:
+        """A genuinely unrestricted page pushes with no unverified count."""
+        client = MagicMock()
+        client.get_current_user.return_value = {"accountId": "my-account-id"}
+        client.get_page_restrictions.return_value = {
+            "update": {
+                "restrictions": {
+                    "user": {"results": []},
+                    "group": {"results": []},
+                }
+            }
+        }
+
+        summary = SyncSummary()
+        config = ManagedConfig()
+        _get_cfg, record_managed_skip = make_managed_helpers(client, _opts(config), summary)
+
+        skipped = record_managed_skip("111", _PAGE_DATA)
+
+        assert skipped is False
+        assert summary.restriction_check_unverified == 0
+
+    def test_confirmed_restricted_page_is_skipped_not_unverified(self) -> None:
+        """A genuinely restricted page is skipped and recorded as a managed skip, not unverified."""
+        client = MagicMock()
+        client.get_current_user.return_value = {"accountId": "my-account-id"}
+        client.get_page_restrictions.return_value = {
+            "update": {
+                "restrictions": {
+                    "user": {"results": [{"accountId": "someone-else"}]},
+                    "group": {"results": []},
+                }
+            }
+        }
+
+        summary = SyncSummary()
+        config = ManagedConfig()
+        _get_cfg, record_managed_skip = make_managed_helpers(client, _opts(config), summary)
+
+        skipped = record_managed_skip("111", _PAGE_DATA)
+
+        assert skipped is True
+        assert summary.restriction_check_unverified == 0
+        assert summary.managed_skips == {"_read_only": 1}

tests/confluence/sync/test_types.py

@@ -0,0 +1,23 @@
+"""Tests for SyncSummary's restriction-check-unverified reporting."""
+
+from __future__ import annotations
+
+from mdd.confluence.sync._types import SyncSummary
+
+
+class TestRestrictionCheckUnverified:
+    def test_omitted_when_zero(self) -> None:
+        summary = SyncSummary(content_pushed=1)
+        msg = summary.format_commit_message("ENG")
+        assert "Restriction check unverified" not in msg
+
+    def test_included_with_count_when_nonzero(self) -> None:
+        summary = SyncSummary(content_pushed=1, restriction_check_unverified=3)
+        msg = summary.format_commit_message("ENG")
+        assert "Restriction check unverified:" in msg
+        assert "3 pages pushed without confirming update permission" in msg
+
+    def test_does_not_count_as_a_change(self) -> None:
+        """An unverified check by itself isn't a mutation the summary reports as 'changed'."""
+        summary = SyncSummary(restriction_check_unverified=5)
+        assert summary.has_changes() is False