Skip to content

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

Merged
lsimons merged 2 commits into
mainfrom
fix/restriction-check-visible
Jul 31, 2026
Merged

fix(confluence): surface unverified restriction checks instead of failing open silently#61
lsimons merged 2 commits into
mainfrom
fix/restriction-check-visible

Conversation

@lsimons

@lsimons lsimons commented Jul 31, 2026

Copy link
Copy Markdown
Member

_user_can_update returned True on any exception. It is the last layer of
the managed-elsewhere cascade — the one that asks Confluence whether page
restrictions make a page read-only for you — so a transient API error, an auth
problem, or an unexpected response shape produced "you may update this page" and
the push proceeded.

Four of the five cascade layers are local comparisons against configuration and
are genuine guarantees. The fifth is a network call that degraded to permission.
Restrictions are precisely the mechanism protecting pages somebody actively
does not want overwritten
, so the check most likely to matter was the one that
could silently stop applying — and an unverified check was indistinguishable
from a clean pass.

Approach: still fails open, no longer silent

Failing closed would mean a Confluence outage blocks all publishing, which is
worse. So the behaviour is unchanged and the visibility is fixed.

_user_can_update now returns bool | None, where None means the API call
raised; it logs a warning at that point, with the exception in scope, rather
than guessing. ManagedClassification carries a new
restriction_check_unverified flag, and SyncSummary carries a count.

The flag lives on the classification rather than in an out-parameter because
every call site — the bulk sync path and the four single-page CLI paths
(mutate.py, update.py, export.py, publish_office.py) — already receives a
ManagedClassification back from classify_page. A collector would have needed
plumbing into paths that have no SyncSummary at all.

What an operator now sees

Warning log:

could not check page restrictions for page 111: network error

Sync summary:

Restriction check unverified: 3 — pushed without confirming update permission (Confluence restriction API call failed; see warnings above)

And a matching section in the generated commit message.

The except Exception is deliberately not narrowed: the point is that any
failure becomes visible, not that some do.

Spec

docs/spec/S26-managed-elsewhere.md read as though all five layers were equally
strict guarantees. It now states the guarantee/advisory split, the fail-open
rationale, and the new visibility mechanism, and the "no override mechanism"
section gained a clarifying sentence so it does not read as contradicting the
fail-open language (that section is about overriding a confirmed match — a
different axis).

Tests

test_api_error_is_unverified_not_allowed,
test_classify_fails_open_and_reports_unverified_on_api_error,
test_classify_logs_warning_on_restriction_api_error,
test_classify_unrestricted_page_not_flagged_unverified,
test_classify_restricted_page_still_refused_when_check_succeeds, plus summary
formatting and record_managed_skip coverage in
tests/confluence/sync/test_types.py and test_events.py.

Follow-up in a separate PR

docs/guide/04-safety.md should carry the same guarantee/advisory split. It is
owned by the in-flight blacklist-enforcement change (#50) to avoid two PRs
editing the same file, and will be updated there.

Fixes #53

Co-Authored-By: lsimons-bot bot@leosimons.com

lsimons and others added 2 commits July 31, 2026 09:29
…ling open silently

_user_can_update returned True on any API exception, so a transient
Confluence error, an auth problem, or an unexpected response shape
looked identical to a confirmed "page is unrestricted" result and the
push proceeded unnoticed. Keep the fail-open behaviour (an outage must
not block all publishing) but stop it being silent: log a warning with
the underlying exception, thread a restriction_check_unverified flag
through ManagedClassification, and count it on SyncSummary so a bulk
sync's run summary and commit message both show how many pages were
pushed without a completed restriction check.

Fixes #53

Co-Authored-By: lsimons-bot <bot@leosimons.com>
Assisted-by: Claude:claude-sonnet-5
…es vs advisory

S26 read as though all five detection layers were equally strict, but
layer 5 (page restrictions) is a network call that can fail while
layers 1-4 are local config comparisons that can't. State the
guarantee/advisory split explicitly, the fail-open rationale for layer
5 (an outage must not block all publishing), and the new visibility
mechanism (warning log + sync run summary count) that keeps a failed
check from looking identical to a confirmed pass.

Co-Authored-By: lsimons-bot <bot@leosimons.com>
Assisted-by: Claude:claude-sonnet-5
@lsimons
lsimons merged commit 5e763f8 into main Jul 31, 2026
7 checks passed
@lsimons
lsimons deleted the fix/restriction-check-visible branch July 31, 2026 11:09
@lsimons lsimons added this to MDD Jul 31, 2026
@github-project-automation github-project-automation Bot moved this to Done in MDD Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

managed-elsewhere restrictions check fails open: _user_can_update returns True on any API exception

1 participant