[bjester]

Summary

Since #392, deserialization streams process records one partition prefix at a time, in the order of the sync filter. A record whose FK target is in a later partition fails to deserialize. Scope listed the read-write partitions first. In Kolibri's single-user scope, this put a user's logs before the user they reference.

Scope.read_filter now lists the read partitions, then the read-write partitions. Scope.write_filter lists the write partitions, then the read-write partitions. Each template keeps its own order. Implementers that pass Scope filters directly to a sync get a usable order, and Kolibri does not have to reorder the read filter.

Kolibri's integration tests passed with these changes. Some of those tests failed after #392 without an override to address this ordering issue.

TODO

• Have tests been written for the new code?
• Has documentation been written/updated?
• New dependencies (if any) added to requirements file (none)

Reviewer guidance

The new order is a convention, not a guarantee. Read-write partitions usually hold a device's own records, which reference shared records in the other partitions. A read-only or write-only record that references a read-write record still fails. Morango cannot detect this case, because it knows the dependencies between models but not between partitions.

#398 rejected two alternatives:

• Sort the partition prefixes. Streaming deserialization part 3: sink #392 tried this, and it broke Kolibri's test_single_user_course_syncing. There, the classroom partitions sort before the user partitions that they reference.
• Query each model across all prefixes at once, as the legacy code did. This makes the order irrelevant, but it combines many prefixes into one query expression.

These uses of a filter do not depend on its order:

• Filter.__eq__ and Filter.is_subset_of compare the partitions without regard to order. The server uses is_subset_of to validate a requested filter.
• lock_partitions collects the partitions into a set before it locks them.

cleanupsyncs --sync-filter matched the stored TransferSession.filter with startswith. Kolibri passes the full filter string of the current sync (str(context.filter)). After the change of order, this string no longer matched the transfer sessions of a single-user scope that were stored before the upgrade. The command now also matches a filter with the same partitions in any order. A prefix, such as a dataset ID, still matches as before.

One case still depends on the order. On a push, the server deserializes with the filter that the client requested. A 0.9 server that receives a push from an older client deserializes in the old order. Kolibri's single-user write filter has no FK between its partitions, so this does not affect Kolibri.

In the deserialization tests, the partition names put the read-write partition first in sort order. So the Scope test also fails if the partitions are sorted. A second test keeps the old order as a counter-example, in which the log fails to deserialize.

Issues addressed

Closes #398. Follows #392.

Documentation

• docs/architecture/index.rst, "Filters and scopes": how to order the partitions in scope templates.
• CHANGELOG.md, 0.9.0.

AI usage

I used Claude Code to write an implementation plan from #398. A separate Claude Code subagent then reviewed the plan against the code. The review found that the planned test did not reject a sorted order, and that cleanupsyncs depends on the filter order. Claude Code implemented the plan in three commits. The plan only documented the cleanupsyncs issue. While writing this description, I asked Claude Code to fix it, which it did in a fourth commit. I reviewed the plan and the code. I changed the duplicate-partition test to assert uniqueness instead of position. I also chose a permanent counter-example test instead of a manual revert step.

[rtibblesbot]

🟡 Waiting for changes

Outcome: reviewed 9c14262

Last updated: 2026-11-03 02:38 UTC

[bjesterbot]

This change reorders Scope.read_filter/Scope.write_filter to list read-write partitions last, so streaming deserialization (which processes one partition prefix at a time in filter order since #392) deserializes shared records before the records that reference them. I verified the order-sensitive consumers: the stream source yields prefixes in filter order (the intended, now-documented contract), while Filter.__eq__, Filter.is_subset_of, the root-cert scope check in Certificate.check_certificate, the viewsets scope validation, and lock_partitions (which locks a set) are all order-insensitive, so certificate IDs, signatures, scope validation, and partition locking are unaffected. The one order-sensitive persisted value, TransferSession.filter (str(context.filter)), is handled by making cleanupsyncs --sync-filter also match stored filters with the same partitions in any order, via order-insensitive Filter.__eq__, while keeping the legacy prefix match.

Findings

• ✅ Positive: The ordering convention is documented in the Scope docstring, in MorangoSource.prefix_conditions, and in docs/architecture/index.rst, including the explicit caveat that it is a convention, not a guarantee (a read-only record referencing a read-write record still fails, undetectable by Morango).
• ✅ Positive: The deserialization counter-example test uses partition names (u:a-rw:U sorts before u:b-ro:U) so it also rejects an alphabetically sorted order, and it pins the old order's failure mode (log stays dirty with ValidationError, no row written). See inline comments.
• 💡 Tactical Suggestion (Optional): In cleanupsyncs.handle, the --push/--pull queryset filters run after the Python-side sync-filter scan; moving them before it shrinks the scanned rows when the flags are combined with --sync-filter. Inline comment on cleanupsyncs.py.
• Note: the startswith branch in _filter_by_sync_filter intentionally preserves legacy prefix semantics (e.g. 1:2 still matches 1:2:ro); this also matches 1:20:..., which is pre-existing behavior, unchanged by this PR.

Verification

• CI on head commit b170d46: all checks pass (unit tests across Python 3.6-3.14 including postgres and Windows runs, lint, wheel build, Read the Docs).
• Local: pytest tests/testapp/tests/sync/stream/ tests/testapp/tests/models/test_certificates.py — 222 passed; pytest tests/testapp/tests/test_management_commands.py — 16 passed, including the 3 new cleanupsyncs --sync-filter tests and the 4 new ScopeTestCase tests.
• Scope: 8 files in 4 commits, each scoped to one concern (core reorder, deserialization tests, docs/changelog, cleanupsyncs matching); no out-of-scope changes. The in-repo acceptance criteria of #398 are covered.

Generated by Mistral Vibe using model mistral-large-4

[bjesterbot]

✅ Positive Highlight: This is the core of the fix. Filter.__add__ deduplicates while keeping the first occurrence's position, so the read/write template partitions keep their own order and the read-write partitions land last. I verified the order-insensitive consumers stay correct: Filter.__eq__ and is_subset_of compare partitions without regard to order (used by Certificate.check_certificate, the viewsets scope checks, and Scope.__eq__), and lock_partitions locks a set of partitions. Certificate IDs and signatures are unchanged because certificates store the templates, not the composed filter.

[bjesterbot]

✅ Positive Highlight: ScopeTestCase pins the exact partition order of both filters, plus the duplicate-partition and empty-template edge cases. Asserting exact tuples (rather than set equality) is what makes the new order a tested convention rather than an implementation detail.

[bjesterbot]

✅ Positive Highlight: The counter-example is well designed — u:a-rw:U sorts before u:b-ro:U, so this test also fails if the partitions were sorted alphabetically. It pins the old order's failure mode: the log streams before its user, so it stays dirty with a ValidationError and no SummaryLog row is written.

[rtibblesbot]

PR #410: the Scope reorder, docs and tests match #398. The deserialize tests show the old order failing, which is the right proof. Findings are inline.

• suggestion: cleanupsyncs --sync-filter can exceed old SQLite's 999-variable limit. It is also outside #398's scope.
• suggestion: the duplicate-partition test passes with read-write partitions first.
• nitpick: the Scope docstring repeats docs/architecture/index.rst.

CI is pending. The PR head moved after the review passes ran. The newest commits were not reviewed.

Next step: Needs a full code review by a human engineer.

• Decide whether the cleanupsyncs --sync-filter matching change belongs in this PR, and whether its query shape is acceptable on old SQLite.
• Confirm that read-write-last partition order is safe for mixed-version syncs, where a server deserializes in an older client's filter order.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

suggestion: --sync-filter now raises OperationalError: too many SQL variables when ~1000 stale sessions match, on SQLite < 3.32.

• The old filter__startswith matched in SQL.
• This loads every stale session's filter into Python, then re-queries with one bound parameter per match.
• Morango supports Python 3.6+, so SQLite's 999-variable limit is in play.
• Scope filters should list read-write partitions last #398 asked only to check filter comparisons; this matching change is new behaviour.

Move it to a separate PR. If it stays, narrow in SQL, e.g. Q(filter__startswith=sync_filter) | Q(*[Q(filter__contains=p) for p in partitions]). Then confirm only the contains hits in Python with Filter(...) == partitions.

[rtibblesbot]

suggestion: This test still passes if read-write partitions come first. Uniqueness plus set equality doesn't check order. Filter.__add__ keeps the first occurrence, so the order is deterministic. Assert it exactly:

self.assertEqual(tuple(scope.read_filter), ("a:shared", "a:user-ro:U", "a:user-rw:U"))

[rtibblesbot]

nitpick: This docstring repeats the new paragraph in docs/architecture/index.rst. The inline comment below repeats it a third time. Replace both with one line pointing to the architecture doc.

[rtibblesbot]

PR #410: the reordering adds no new issues, but all 3 prior findings are still open. None has a reply; see their existing threads.

CI passing.

• suggestion (cleanupsyncs.py:72): --sync-filter still fails on SQLite with too many SQL variables once about 1000 sessions match. It binds one id__in parameter per match. Filtering after --push/--pull scans fewer sessions but keeps this limit. Could the match be chunked, or done without id__in?
• suggestion (test_certificates.py:315): the duplicate-partition test still passes if read-write partitions come first. It only checks uniqueness and set equality. Could it assert the order too?
• nitpick (certificates.py:486): the Scope docstring repeats the architecture doc and the inline comment. Could it drop the repeated part?
• nitpick: one new comment, inline.

Prior-finding status

UNADDRESSED — morango/management/commands/cleanupsyncs.py:72 — --sync-filter exceeds SQLite variable limit past ~1000 matches
UNADDRESSED — tests/testapp/tests/models/test_certificates.py:315 — duplicate-partition test passes with read-write partitions first
UNADDRESSED — morango/models/certificates.py:486 — Scope docstring repeats architecture doc and inline comment

Next step: Needs a full code review by a human engineer.

• Confirm that putting read-write partitions last in Scope filters is safe for existing deployments, since it changes deserialization order and the stored filter strings that sync sessions compare.
• Decide whether cleanupsyncs --sync-filter needs to avoid SQLite's ~1000-variable limit before this merges.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

nitpick: This comment restates what the code already shows. The values_list call in the helper shows the filter is matched in Python. The helper docstring's second sentence also restates what its partition-order test covers. Could we drop both?