feat(PLU-511): ACL digest (permissions_version) for OneDrive/SharePoint

[danielle-unstructured-io]

What

Adds a per-file ACL digest (permissions_version) so an ACL-only change can trigger reprocessing under incremental (reprocess_all = false), following the mechanism recommended in the spike. This is the connector-layer slice of PLU-511; it pairs with the invoker changes in platform-plugins and the compare/write-back in platform-etl-orchestration (#1250).

• utils/acl.py — compute_permissions_version(permissions_data, denied_permissions_data=None): canonicalizes (recursive sort) then sha256. Returns None only when there is no ACL signal at all; an empty list is a real state (all access revoked) and yields a stable digest.
• data_types/file_data.py — new permissions_version field on FileDataSourceMetadata.
• processes/connectors/onedrive.py — the indexer computes the digest over the permissions it already batch-fetches (SharePoint inherits this base class). Adds call-count telemetry proving the digest issues 0 additional Graph calls.
• test/unit/utils/test_acl.py — determinism / order-independence / empty-vs-None behavior.

Validated

Proven end to end on an SND against a real SharePoint site: an ACL-only change (content unchanged) reprocesses exactly the changed file (spike scenarios S2/S8). See PLU-511 for evidence.

Notes

• Draft. Branch is behind main and needs a rebase before merge.
• Known limitation surfaced during validation: SharePoint siteGroup ACL changes are not captured (pre-existing PLU-370 _extract_identity_ids_from_raw exclusion), so a site-group-only change won't move the digest. Documented in the spike; follow-up TBD.

PLU-511

🤖 Generated with Claude Code

[cubic-dev-ai]

All reported issues were addressed across 6 files

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.

Re-trigger cubic</sub>

[awalker4]

A few things from a read-through, mostly around the new None-vs-empty contract plus some cleanup I'd want before this merges.

extract_permissions([]) returns [{}]

Relaxing the guard to is not None makes the empty-list branch reachable for the first time, and extract_permissions short-circuits a falsy input to [{}] rather than to the normalized all-empty triple. Two effects worth deciding on deliberately rather than inheriting:

• permissions_data is now [{}] on a fully-revoked record where it used to be unset, and that value flows downstream to destinations that write ACL fields.
• "No one has access" ends up with two different digests: [{}] when it arrives as an empty fetch, and [{"read": {"users": [], "groups": []}}, ...] when it arrives as permission entries whose roles don't map to any operation. Drifting between those two representations would look like an ACL change and cause a spurious reprocess, even though effective access is identical in both.

Returning the normalized all-empty triple from the not raw_permissions branch would collapse the two. Either way, test_permissions_captured_when_empty_list_passed currently only asserts is not None, so the actual value isn't pinned by anything — worth asserting the shape.

_parse_batch_response can raise on a null body

sub.get("body", {}).get("value", []) — the {} default only applies when the key is absent, so a sub-response of {"id": "0", "status": 200, "body": null} raises AttributeError and takes down the whole index run rather than degrading. Separately, a 200 whose body is present but has no value key falls through to [], which under the new contract is a fabricated revocation — the exact case the fetch-vs-revocation change set out to prevent. Something like (sub.get("body") or {}).get("value"), with a missing value treated as unavailable, covers both.

Version bump

This adds a public field to FileDataSourceMetadata and a new public unstructured_ingest/utils/acl module. A minor bump reads more accurately than a patch for a new consumable surface.

Changelog

Three entries, where two of them describe fixes made to the first during review. From outside the PR that reads as churn rather than as anything a consumer acts on — a single entry describing the digest and its None-vs-empty semantics covers what changed.

The telemetry helper

_AclDigestTelemetry's own docstring notes the digest's marginal API cost is "zero by construction, not a separately measured value," which I think is exactly right — but if so, the counter isn't measuring anything a reader couldn't determine from the code. Meanwhile it emits a six-line prose log line on every index run and is a private name imported across module boundaries. I'd drop it and record the number wherever the cost analysis is being written up.

Internal references in the code

There's a fair amount of internal ticket and scenario shorthand sitting in comments, docstrings, and test names here, including inside a log message that ships to production. This repo is public, so none of it means anything to a reader who doesn't have the internal context open. Worth a pass to either drop them or restate the reasoning inline so the code stands on its own.

Small one

_canonicalize's dict branch is redundant. json.dumps(..., sort_keys=True) already sorts keys both in the inner sort key and in the final encode, so the list-sorting branch is the only part doing real work.

[cubic-dev-ai]

2 issues found across 4 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="unstructured_ingest/processes/connectors/onedrive.py">

<violation number="1" location="unstructured_ingest/processes/connectors/onedrive.py:528">
P2: A malformed 200 sub-response can still crash indexing because `_parse_batch_response` accepts any `body/value` shape and passes it as `raw_permissions`. Consider validating `body` is a dict and `value` is a list of permission dicts, otherwise degrading to `None` so this path skips digesting instead of failing the run.</violation>
</file>

<file name="test/unit/connectors/test_onedrive.py">

<violation number="1" location="test/unit/connectors/test_onedrive.py:491">
P2: A malformed successful Graph sub-response with a non-object `body` can still abort the index run, despite these tests asserting malformed bodies should degrade to `None`. Extending this coverage with cases such as `body=[]` and `body="invalid"` would expose the remaining parser gap and support guarding the body with a mapping check.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: A malformed 200 sub-response can still crash indexing because _parse_batch_response accepts any body/value shape and passes it as raw_permissions. Consider validating body is a dict and value is a list of permission dicts, otherwise degrading to None so this path skips digesting instead of failing the run.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At unstructured_ingest/processes/connectors/onedrive.py, line 528:

<comment>A malformed 200 sub-response can still crash indexing because `_parse_batch_response` accepts any `body/value` shape and passes it as `raw_permissions`. Consider validating `body` is a dict and `value` is a list of permission dicts, otherwise degrading to `None` so this path skips digesting instead of failing the run.</comment>

<file context>
@@ -518,7 +521,11 @@ def _parse_batch_response(
+                # (revoked) state and stays []. A null/absent body or a missing
+                # value key is malformed/unavailable and degrades to None (skip
+                # the digest) rather than a fabricated revocation.
+                by_id[di.id] = (sub.get("body") or {}).get("value")
             elif status in (401, 403):
                 logger.error(f"forbidden fetching permissions for {di.name} (status {status})")
</file context>

[cubic-dev-ai]

P2: A malformed successful Graph sub-response with a non-object body can still abort the index run, despite these tests asserting malformed bodies should degrade to None. Extending this coverage with cases such as body=[] and body="invalid" would expose the remaining parser gap and support guarding the body with a mapping check.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At test/unit/connectors/test_onedrive.py, line 491:

<comment>A malformed successful Graph sub-response with a non-object `body` can still abort the index run, despite these tests asserting malformed bodies should degrade to `None`. Extending this coverage with cases such as `body=[]` and `body="invalid"` would expose the remaining parser gap and support guarding the body with a mapping check.</comment>

<file context>
@@ -471,6 +483,26 @@ def test_per_item_200_empty_value_is_empty_list(self):
+        # None (unavailable), not [] (which would be a fabricated revocation).
+        indexer = _make_indexer()
+        items = [_make_drive_item("f.docx")]
+        body = _batch_response(responses=[{"id": "0", "status": 200, "body": None}])
+        with patch("requests.post", return_value=body):
+            result = indexer._fetch_permissions_raw(items, access_token="tok")
</file context>

[danielle-unstructured-io]

Thanks Austin, this was a great pass. Rundown of what landed:

_parse_batch_response null/malformed body — fixed in 50c698a1. (sub.get("body") or {}).get("value") now degrades a null/absent body or a missing value key to None (unavailable, skip the digest) while a present-but-empty value stays [] (genuine revocation). Added tests for the null-body and missing-value cases.

extract_permissions([]) returning [{}] — fixed in 50c698a1. The empty branch now falls through to the normalized all-empty read/update/delete triple, so a revoked record and a no-op-role record produce the same digest (no spurious reprocess from representation drift). Both the extract_permissions test and the drive-item test now pin the exact shape instead of just is not None.

Version bump — agreed, bumped to a minor (1.8.0) for the new public field + utils/acl module.

Changelog — collapsed to a single entry describing the digest and its None-vs-empty semantics.

Telemetry helper — dropped entirely in 1c1d383d. You're right that the cost is zero by construction, so the counter wasn't measuring anything derivable from the code, and it was the one place internal shorthand shipped in a production log line. Cost is now documented once in the analysis (microbenchmark + write-up) rather than emitted at runtime.

_canonicalize dict branch — I looked at this closely and I'd push back: it's load-bearing, not redundant. json.dumps(sort_keys=True) canonicalizes dict key order but never reorders list elements, so the dict branch is what lets the recursion descend into the nested users/groups lists to sort them. Dropping it makes compute_permissions_version(_perms(["a","b"])) != compute_permissions_version(_perms(["b","a"])) — i.e. test_reordered_users_same_digest fails (confirmed by removing the branch and running it). Determinism regardless of principal ordering is the whole point of the canonicalization, so I've kept it. Happy to add a comment making that intent explicit if it read as redundant.

One remaining item from your public-repo note: the shipped log line is gone with the telemetry, but there are still PLU-511/scenario refs in source comments and test names. Most restate the reasoning inline already; I can do a sweep to drop the bare tags if you'd prefer.

[cubic-dev-ai]

0 issues found across 3 files (changes from recent commits).

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.

Re-trigger cubic</sub>

[danielle-unstructured-io]

Did the internal-reference sweep in b107d44c — dropped the ticket/scenario shorthand from comments, docstrings, and test docstrings so the code reads on its own in the public repo (restated the reasoning inline where a ref was carrying it, and kept the code pointer to _extract_identity_ids_from_raw). Left the CHANGELOG's ticket ref, since every entry in this repo carries one.

That should cover everything from your pass — thanks again.

[cubic-dev-ai]

0 issues found across 4 files (changes from recent commits).

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 2 unresolved issues from previous reviews.

Re-trigger cubic</sub>