Service layer centralization — Phase 2B & 2C: CorpusService + caller migration (#1716)

[JSv4]

Summary

Phases 2B and 2C of the service-layer centralization roadmap (issue #1716), stacked on Phase 2A (PR #1737). Targets the 2A branch so this diff is exactly 2B + 2C; GitHub will auto-retarget to main once #1737 merges.

Phase 2A split the corpus_objs_service.py monolith into the segmented corpuses/services/ package behind a deprecated CorpusObjsService re-export shim. This PR adds the sixth service and removes the shim.

Phase 2B — CorpusService for Corpus-row CRUD

New CorpusService(BaseService) (corpuses/services/corpus_service.py) owning the write surface of the Corpus row itself, closing design-doc §3 Problem 3 (corpus contents had a service; the corpus row did not):

• delete_corpus — personal-corpus / user-lock / DELETE-permission gates
• set_visibility — PERMISSION-gated public/private switch + make_corpus_public_task cascade
• update_description — creator-only markdown-description versioning
• assert_embedder_change_allowed — issue Embedder Consistency Issue: PREVENT Need for Re-embedding Support #437 embedder guard
• grant_creator_permissions — CRUD + PUBLISH + PERMISSION

SetCorpusVisibility, UpdateCorpusDescription, and DeleteCorpusMutation become thin wrappers. CreateCorpusMutation / UpdateCorpusMutation keep their shared DRFMutation create/update machinery (deliberately — gutting it for one model would be a needless frontend-contract risk) but delegate their corpus-specific logic to CorpusService. No GraphQL contract change.

Phase 2C — caller migration + shim removal

Every caller of the CorpusObjsService facade is migrated onto the segmented services, and corpus_objs_service.py + the CorpusObjsService facade class are deleted (CLAUDE.md no-dead-code rule):

• ~23 production caller files repointed at the specific owning service(s)
• test_corpus_objs_service.py (~290-test regression suite) keeps every scenario/assertion unchanged — only call targets repointed
• test_corpus_services_package.py drops the obsolete shim/facade structural tests, adds CorpusService
• CLAUDE.md rule 7, the consolidated permissioning guide, docstrings, and schema.graphql descriptions updated

Pure relocation of import paths and call targets — no permission semantics, transaction boundary, or query shape changes.

Test plan

• pytest test_corpus_objs_service.py test_corpus_services_package.py test_corpus_service.py test_base_service.py test_document_service.py test_permanent_deletion.py test_zip_import_integration.py test_corpus_get_documents_deprecation.py test_corpus_folder_mutations.py permissioning/test_permission_optimizer.py permissioning/test_corpus_visibility.py — 438 passed
• pytest test_llms_typing_coverage.py test_move_document_tool.py test_document_stats.py test_import_utils.py test_mcp_extended.py test_og_metadata_queries.py test_extract_mutations.py test_document_relationship_mutations.py test_llm_tools.py test_caml_review_tools.py — 444 passed (1 unrelated env error — parallel-worker PDF-storage collision — confirmed passing in isolation)
• black / isort / flake8 / mypy clean on all changed files
• New test_corpus_service.py — 16 tests covering every CorpusService method

Plan: docs/refactor_plans/2026-05-22-service-layer-phase2bc-corpus-service-and-caller-migration.md

Closes the remainder of #1716 (Phases 2B + 2C; Phase 2A is PR #1737).

[claude]

Code Review — Phase 2B & 2C: CorpusService + Caller Migration

Overview

This PR completes the service-layer centralization for issue #1716, adding CorpusService (Phase 2B) to own corpus-row CRUD, migrating all ~23 callers off the deprecated CorpusObjsService facade (Phase 2C), and deleting the shim (CLAUDE.md no-dead-code rule). No GraphQL contract changes; purely structural. The diff is clean and purposeful.

---

Phase 2B — CorpusService

Strengths:

• Good separation: the new service correctly owns only the write surface of the Corpus row. Read operations stay on Corpus.objects.visible_to_user / get_for_user_or_none, which is the right call.
• ServiceResult[T] used consistently for delete_corpus, set_visibility, and update_description. Error strings are IDOR-safe (same message for not-found vs. not-authorized).
• The comment thread in delete_corpus accurately describes the lock-check semantics (lock holder OR superuser via the permission gate can proceed).
• grant_creator_permissions is a clean extraction — no logic change, just a named home.

Issues / questions:

1. Permission bootstrap in TestCorpusServiceSetVisibility and TestCorpusServiceDeleteCorpus

Both test classes create corpora via Corpus.objects.create() (bypassing CreateCorpusMutation) and then call service methods that run require_permission(corpus, user, PermissionTypes.{PERMISSION,DELETE}) against the creator. Without CorpusService.grant_creator_permissions being called, the creator has no guardian-level object permission. If Corpus.user_can or BaseService.require_permission has special-case logic for the object's creator field the tests pass for the right reason; if not, they pass vacuously and would miss a regression where the check is accidentally short-circuited.

Suggestion: Add a brief comment in the test setUp explaining why the creator passes the gate without an explicit grant (e.g., auto-signal, creator bypass in user_can, or superuser sentinel), or call CorpusService.grant_creator_permissions explicitly to make the tests self-documenting and independent of that implicit mechanism.

2. corpus_id type change in make_corpus_public_task

Old code passed corpus_id=corpus_pk where corpus_pk was extracted via from_global_id (a string). New code passes corpus_id=corpus.pk (integer). The PR notes the task accepts either. The test assertion was correctly updated. However, if there are other callers of make_corpus_public_task outside this mutation, they may still be passing a string — worth a quick grep to confirm consistency.

3. assert_embedder_change_allowed return type asymmetry

All other CorpusService methods return ServiceResult. assert_embedder_change_allowed returns a plain str (empty = ok, non-empty = error). The asymmetry is acceptable for a guard function — just worth a docstring note that it is deliberate, since a future reader may attempt to "normalize" it to ServiceResult.

4. Local import in set_visibility for circular-dep avoidance

The from opencontractserver.tasks.permissioning_tasks import make_corpus_public_task lives inside the method body. This is correct (and the @patch target in tests is correctly aimed at the source module, not the old config.graphql.corpus_mutations location). Consider a one-line comment explaining the local import is for circular-import avoidance, consistent with how other local-import patterns are documented in the codebase.

---

Phase 2C — Caller Migration + Shim Deletion

Strengths:

• Migration is deterministic (segmented services have disjoint method names) — no ambiguity about which service owns which method.
• All 290 behavioral assertions in test_corpus_objs_service.py are preserved; only the call target is repointed. Correct approach.
• test_corpus_services_package.py cleanly drops the now-dead shim/facade structural tests and adds CorpusService. No stale assertions.
• schema.graphql, CLAUDE.md, the consolidated permissioning guide, and docstrings are all consistently updated. No documentation debt left behind.
• corpus_objs_service.py deletion is complete. The CHANGELOG details the method-to-service mapping.

Nits:

5. PermissionTypes import in corpus_mutations.py — the top-level import remains after the refactor. Likely still used elsewhere in the file; flake8 would catch it if not.

6. test_corpus_visibility.py @patch target — updated correctly from config.graphql.corpus_mutations.make_corpus_public_task to opencontractserver.tasks.permissioning_tasks.make_corpus_public_task. Since set_visibility imports it locally at call time, patching the source module is the right approach. The integer vs. string corpus_id assertion change is also correct.

7. og_metadata_queries.py OG endpoint — the anonymous-user visibility gate is preserved verbatim. No semantic change.

---

Test Coverage

• 16 new test_corpus_service.py tests cover all five CorpusService methods with success and denial paths.
• The 290-test behavioral regression suite passes through direct service calls (no longer via the shim), which is the gold standard.
• The PR test plan reports 882 total passing tests across two runs — solid.

Optional enhancement: A test asserting from opencontractserver.corpuses import corpus_objs_service raises ModuleNotFoundError would lock in the shim-deletion guarantee and prevent accidental resurrection. Low priority.

---

Security

No permission semantics changed. IDOR-safe patterns are maintained throughout:

• get_for_user_or_none as the READ gate before any write operation.
• Same error message for not-found vs. unauthorized.
• require_permission with the correct permission type at each service boundary.

---

Summary

Well-executed structural refactor. The migration is mechanical and auditable, the shim is cleanly deleted, and the test suite is updated at the right level of granularity. The main item to address before merge is point 1 — the permission bootstrap question in the new CorpusService unit tests. Everything else is minor.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review — PR #1745: Service Layer Phase 2B & 2C

Overview

This PR completes the service-layer centralization roadmap (issue #1716) across two phases:

• Phase 2B: Adds CorpusService owning the corpus-row write surface (delete, visibility, description versioning, embedder guard, creator permissions)
• Phase 2C: Migrates all ~23 production callers from the deprecated CorpusObjsService facade to the segmented services, then deletes the facade entirely

The approach is sound: pure relocation of call sites, no semantic changes, backed by a 290-test regression suite and 16 new service-level tests. The architecture is significantly cleaner after this.

---

Positives

• Consistent service API: CorpusService methods follow the established ServiceResult.success / ServiceResult.failure convention from BaseService, making control flow in callers uniform and readable.
• IDOR protection preserved: Every mutation wrapper maintains unified "not found or no permission" messages; the service layer adds a PERMISSION gate in set_visibility that correctly sits inside the existing READ gate (established by get_for_user_or_none in the caller).
• Circular import handled correctly: make_corpus_public_task is deferred inside set_visibility rather than imported at module level. The test mock path is also updated accordingly (config.graphql.corpus_mutations.make_corpus_public_task → opencontractserver.tasks.permissioning_tasks.make_corpus_public_task), which is the right place to patch a lazily-imported name.
• No-dead-code rule enforced: corpus_objs_service.py is completely deleted, not left as a commented-out stub.
• Test superuser collision fix: The rename from admin → su_bypass_admin in test_corpus_objs_service.py with an explanatory comment is exactly the right fix for the pytest -n --dist loadscope worker-DB collision.
• Documentation sweep is complete: CLAUDE.md rule 7, the permissioning guide, model docstrings, schema.graphql descriptions, apps.py comment, and every mutation/resolver docstring are all updated. Nothing left pointing at the deleted class.

---

Issues and Suggestions

1. corpus_id type change to make_corpus_public_task — verify task accepts int

File: opencontractserver/corpuses/services/corpus_service.py:136

The old SetCorpusVisibility.mutate passed corpus_pk (the string returned by from_global_id) to the task:

make_corpus_public_task.si(corpus_id=corpus_pk).apply_async()

The new CorpusService.set_visibility passes corpus.pk (an integer):

make_corpus_public_task.si(corpus_id=corpus.pk).apply_async()

The test assertion is updated with the comment "the task accepts either". This should be verified — if the task passes corpus_id directly to an ORM lookup (Corpus.objects.get(pk=corpus_id)), Django handles both. But if it does string comparison or passes it to a relay to_global_id call, an int will silently behave differently. Worth confirming the task is truly type-agnostic, or asserting the integer type explicitly in the test rather than documenting the ambiguity in a comment.

2. assert_embedder_change_allowed — API inconsistency is documented but worth surfacing at class level

File: opencontractserver/corpuses/services/corpus_service.py:157-178

The method docstring explicitly says "Do not normalise it to ServiceResult" and explains why. The reasoning is solid (success value channel would be dead weight for a pure guard). No change required, but since callers cannot uniformly pattern-match on ServiceResult for all CorpusService methods, consider adding a one-sentence note to the class-level docstring so the inconsistency is visible at a glance when scanning the class.

3. getattr(user, "id", None) in update_description — fragile anonymous-user path

File: opencontractserver/corpuses/services/corpus_service.py:61

if corpus.creator_id != getattr(user, "id", None):

For AnonymousUser, getattr(user, "id", None) returns None. Since corpus.creator_id is NOT NULL, this comparison is always True for anonymous users, returning the IDOR-safe error — correct behavior, but the intent is obscured. The GraphQL wrapper gates corpus READ before reaching this point, so an anonymous user can never get here for a private corpus, but the service itself doesn't document that assumption. A brief comment on the getattr pattern would help the next reader distinguish intentional defensive fallback from an accidental attribute name typo (id vs pk).

4. test_make_public_dispatches_cascade_task — .apply_async() not asserted

File: opencontractserver/tests/test_corpus_service.py:156-161

with patch("opencontractserver.tasks.permissioning_tasks.make_corpus_public_task") as mock_task:
    result = CorpusService.set_visibility(self.creator, self.corpus, True)
self.assertTrue(result.ok)
mock_task.si.assert_called_once_with(corpus_id=self.corpus.pk)

The test verifies .si() was called but not that .apply_async() was invoked on the result. The test_corpus_visibility.py fixture sets mock_task.si.return_value.apply_async.return_value = None before calling, which is the safer pattern. Consider also asserting mock_task.si.return_value.apply_async.assert_called_once() — this would catch a future regression where .apply_async() is accidentally dropped from the service method.

---

Security

No new security concerns. The PERMISSION gate in CorpusService.set_visibility correctly follows the READ-first / escalation-gate pattern. The request= kwarg is properly threaded through to the Tier-2 permission cache in every method that performs a require_permission check.

Test Coverage

• 16 new CorpusService tests with meaningful scenario names and clear setup/teardown
• 290 behavioral regression tests preserved with call targets repointed
• Mock paths correctly reflect the new deferred-import location
• The personal-corpus guard test leverages the auto-created personal corpus from the signal rather than creating a manual fixture — good use of the data model

---

Overall

High-quality, mechanically disciplined refactor. The service split is well-motivated, the execution is clean, and the test coverage is comprehensive. The corpus_id type change to make_corpus_public_task (item 1) and the getattr pattern in update_description (item 3) are the only items worth a follow-up before merge; the rest are minor polish suggestions.

[claude]

Code Review - Service Layer Phase 2B & 2C

Overview

This PR completes the CorpusObjsService monolith decomposition (issue #1716):

• Phase 2B: Adds CorpusService(BaseService) - the sixth segmented corpus service, owning the write surface of the Corpus row itself
• Phase 2C: Migrates all ~23 production callers and the test suite off the deprecated CorpusObjsService facade, then deletes the shim

The diff is large (49 files, +1255/-1523) but the vast majority is mechanical import substitution. The code is clean, well-tested, and follows established patterns throughout.

---

What Works Well

Service design is consistent. CorpusService matches the classmethod-based, ServiceResult-returning shape of the five existing segmented services. The circular-import risk from make_corpus_public_task is correctly handled with a local deferred import inside set_visibility.

assert_embedder_change_allowed deliberately returns str. The docstring explains why (pre-save guard with only an optional error output). The explicit callout prevents future "normalise this" refactors that would be net-negative.

Permission semantics are preserved exactly.

• update_description: old corpus.creator != user becomes corpus.creator_id != getattr(user, "id", None) - avoids an extra FK hydration, same logic
• set_visibility / delete_corpus IDOR envelope unchanged
• The embedder guard reorders short-circuit conditions (new_embedder != corpus.preferred_embedder before corpus.has_documents()) - logically equivalent but slightly faster in the common no-change case

Mock patch target fix in test_corpus_visibility.py is correct. The old patch hit config.graphql.corpus_mutations.make_corpus_public_task (a now-gone module-level import). The new patch correctly hits the definition site opencontractserver.tasks.permissioning_tasks.make_corpus_public_task, which is where CorpusService.set_visibility resolves the symbol at call time via its local import.

Test coverage for the new service is solid. 16 tests covering success, creator denial (description), personal-corpus reject, user-lock reject, permission denial, visibility no-op, embedder guard (same/different/blocked), and explicit permission grant. The test docstrings correctly explain the creator short-circuit to prevent assertions from being vacuous.

Phase 2C is purely mechanical. No permission semantics, transaction boundary, or query shape changed. Every scenario and assertion in the ~290-test regression suite is preserved - only call targets are repointed.

---

One Gap to Address

grant_creator_permissions is missing log_action.

Every other mutating method in CorpusService calls cls.log_action:

• update_description: cls.log_action("Updated description for", corpus, user) - YES
• delete_corpus: cls.log_action("Deleted", corpus, user) - YES
• set_visibility (both branches): YES
• grant_creator_permissions: MISSING

The set_permissions_for_obj_to_user call grants CRUD + PUBLISH + PERMISSION on a corpus row - the highest-privilege action in the service. The fact that CreateCorpusMutation logs the corpus creation does not cover the permission grant itself.

Suggested addition at the end of grant_creator_permissions in corpus_service.py:

cls.log_action("Granted creator permissions on", corpus, user)

---

Minor Notes (Non-blocking)

__all__ ordering in services/__init__.py. CorpusService is inserted mid-list between CorpusDocumentService and DocumentLifecycleService. Since it owns the corpus row itself (not corpus contents), placing it first or last would read more intuitively. Nit only.

Local vs. top-level imports in corpus_mutations.py. SetCorpusVisibility, CreateCorpusMutation, etc. import CorpusService at module top level, while AddDocumentsToCorpus and RemoveDocumentsFromCorpus still use local deferred imports for CorpusDocumentService. Pre-existing inconsistency, not a regression from this PR.

---

Security

No concerns. Every entry point that accepts a user-supplied corpus ID continues to run get_for_user_or_none (READ gate) before handing the corpus object to the service. PERMISSION / DELETE gates are then enforced inside the service. The IDOR-safe unified error message is preserved throughout.

---

Summary

The log_action omission in grant_creator_permissions is the only substantive gap worth fixing before merge. Architecture, permission semantics, test coverage, and documentation are all in excellent shape. The phased approach (shim in 2A, full removal in 2C) was the right call for a migration of this scope.

[claude]

Code Review — Phase 2B & 2C: CorpusService + Caller Migration (#1716)

Overview

This PR adds CorpusService as the sixth segmented corpus service (Phase 2B), then migrates every CorpusObjsService call site to the owning segmented service and deletes the deprecated shim (Phase 2C). The diff spans 49 files, 1,256 additions, and 1,523 deletions, but the signal-to-noise is high: most churn is deterministic import path renames and the test suite repoint.

---

Strengths

• CorpusService design is correct. All five methods follow the established BaseService pattern (@classmethod, ServiceResult[T] return, cls.require_permission / cls.log_action). The creator-only guard in update_description (checking creator_id rather than a guardian grant) is the right call — it keeps description history attributable and is clearly documented.
• No behavioral changes. Permission semantics, transaction boundaries, and GraphQL contract are unchanged. The five mutations (SetCorpusVisibility, UpdateCorpusDescription, DeleteCorpusMutation, CreateCorpusMutation, UpdateCorpusMutation) slim down to thin wrappers without changing what they do.
• Phase 2C is a pure rename. The 438-test regression suite passes unchanged; only call targets are repointed. The method name→owning-service mapping is deterministic because the five segmented services have disjoint method names.
• No dead code left behind. corpus_objs_service.py is deleted, test_corpus_services_package.py gains CorpusService coverage and drops the now-obsolete shim structural tests, CLAUDE.md rule 7 / permissioning guide / models.py docstrings are all updated.
• Test design in test_corpus_service.py is sharp. The grant_creator_permissions test grants permissions to a grantee (non-creator), not to the creator — this correctly exercises the explicit guardian grant path rather than relying on the creator short-circuit. The delete_corpus tests hit all three gate layers in isolation (personal-corpus, lock, permission denial), and the embedder guard tests use patch.object(Corpus, "has_documents") rather than creating real document rows.
• Circular import handled cleanly. Local import of make_corpus_public_task inside set_visibility and TYPE_CHECKING guard on model types both prevent circular-import issues at module load time.

---

Observations

1. transaction.on_commit gap in set_visibility (pre-existing, now centralized)

# corpus_service.py, set_visibility
make_corpus_public_task.si(corpus_id=corpus.pk).apply_async()

The Celery task is dispatched without transaction.on_commit(). If the enclosing DB transaction rolls back (e.g., a mid-request exception), the task fires against uncommitted state. This matches the pre-refactor mutation code, so it is not a regression, and the PR correctly notes the behavior is preserved. However, now that this logic lives in a shared service method that will be called from more places over time, it is worth opening a follow-up issue. Wrapping it as:

from django.db import transaction
transaction.on_commit(lambda: make_corpus_public_task.si(corpus_id=corpus.pk).apply_async())

would make the cascade robust at no cost.

2. Mock patch target moved to the source module (correct, non-obvious)

Tests for set_visibility moved their patch target from config.graphql.corpus_mutations.make_corpus_public_task (a top-level import in the old mutation file) to opencontractserver.tasks.permissioning_tasks.make_corpus_public_task (the source module). This is the correct target for a local import (from ... import name inside the method body resolves the attribute on the source module at call time). The test_corpus_service.py tests use the same target. ✓

3. corpus_id type change to Celery task (documented, benign)

Old mutation: corpus_id=str(self.corpus.id) (string returned by from_global_id).
New service: corpus_id=corpus.pk (integer).

The PR body documents that make_corpus_public_task accepts either. The test assertion was correctly updated to assert_called_once_with(corpus_id=self.corpus.id) (integer). If make_corpus_public_task's interface is ever tightened, this coercion will surface clearly in that task's own tests.

4. assert_embedder_change_allowed removes the has_documents() short-circuit guard

Old UpdateCorpusMutation:

if corpus is not None and corpus.has_documents():
    new_embedder = kwargs["preferred_embedder"]
    if new_embedder != corpus.preferred_embedder:
        ...

New code:

if corpus is not None:
    embedder_error = CorpusService.assert_embedder_change_allowed(
        corpus, kwargs["preferred_embedder"]
    )

assert_embedder_change_allowed internally does if new_embedder != corpus.preferred_embedder and corpus.has_documents(), so behavior is identical. The refactored path does call has_documents() for all corpus updates (not only those with documents), but has_documents() should be a cheap EXISTS subquery. No functional concern.

---

Minor nits

• corpus_service.py line 141 has a cosmetic multi-line comment block (# User-lock check: ...) that extends to 3 lines for a fairly simple guard. Consistent with other services but could be a single line. Not worth a change request.
• The assert_embedder_change_allowed docstring calls out "Deliberately returns a plain str rather than ServiceResult" — good rationale, and the pattern is documented for future readers.

---

Summary

Clean, well-executed refactoring. The new CorpusService is correctly designed, all tests pass, no behavioral changes, no dead code, and all documentation is updated. The one pre-existing gap worth a follow-up issue is the missing transaction.on_commit wrapper around make_corpus_public_task.apply_async() — the PR is not the right place to fix it (it would expand scope), but centralizing this logic in set_visibility makes the gap more visible now that it has a single, auditable home.

Verdict: approve with the transaction.on_commit note logged as a follow-up.

[claude]

Code Review - PR #1745: Service Layer Phase 2B & 2C

Note: This PR is already merged. Review provided for the record.

Overview

This PR completes issue #1716 (service-layer centralization) in two phases:

• 2B: Adds CorpusService — the sixth segmented corpus service, owning the write surface of the Corpus row itself (delete, visibility, description versioning, creator-permission grant, embedder guard)
• 2C: Migrates ~23 production callers and the test suite off the deprecated CorpusObjsService facade, then deletes corpus_objs_service.py entirely

The migration is pure relocation of import paths and call targets — no permission semantics, transaction boundaries, or query shapes change.

---

Strengths

Architecture is clean and principled. CorpusService follows the BaseService classmethod pattern consistently and closes the design-doc gap: corpus contents had a service but the corpus row did not.

Zero semantic changes in Phase 2C. Each CorpusObjsService.X maps to a single owning service with disjoint method names. The method-to-service map in the refactor plan doc makes the migration deterministic and auditable.

assert_embedder_change_allowed returns plain str rather than ServiceResult. The docstring explicitly justifies this — it is a pre-save guard whose only output is an optional error message, so the ServiceResult success/value channel would be dead weight. The "Do not normalise it" callout prevents future well-meaning but wrong cleanups.

test_corpus_objs_service_shim_is_deleted pin test is a clever addition — asserting ModuleNotFoundError prevents accidental resurrection of the deprecated module.

Patch target change for make_corpus_public_task is correct. Moving from config.graphql.corpus_mutations.make_corpus_public_task to opencontractserver.tasks.permissioning_tasks.make_corpus_public_task is right since the import is now deferred inside the method body and resolves at call time.

delete_corpus gate ordering is correct — personal corpus check, then user-lock check, then DELETE permission — matching the former inline mutation logic exactly.

---

Issues / Suggestions

Minor: Bare assert in Django TestCase — prefer self.assertIsNotNone

opencontractserver/tests/test_corpus_service.py lines ~139 and ~252 use bare assert result.value is not None. Bare assert in django.test.TestCase subclasses bypasses Django's test runner error context and produces less helpful output on failure. The rest of the class uses self.assert* methods — self.assertIsNotNone(result.value) here would be consistent.

Acknowledged: corpus_id type change to make_corpus_public_task (str → int)

The old mutation called make_corpus_public_task.si(corpus_id=corpus_pk) where corpus_pk was a string from from_global_id. The new service calls make_corpus_public_task.si(corpus_id=corpus.pk) (integer). The PR description acknowledges this and states "the task accepts either", and tests were updated accordingly. Low-risk, but worth confirming the task coerces the argument explicitly.

Observation: grant_creator_permissions returns None, not ServiceResult

This is the only mutating method on CorpusService that does not follow the ServiceResult convention — intentionally, since set_permissions_for_obj_to_user has no signallable failure path. Fine for now; worth noting for consistency if a failure mode is ever added.

Suggestion: Add a note about corpus row creation to the class docstring

CreateCorpusMutation keeps its DRFMutation machinery; CorpusService.grant_creator_permissions is invoked as a post-create hook. The PR description explains this deliberate design choice well. A one-liner in the CorpusService class docstring noting that row creation stays in CreateCorpusMutation + DRFMutation would save a future reader from wondering why create_corpus is absent.

---

Test Coverage

Area	Assessment
test_corpus_service.py (16 new tests)	All 5 methods: success, permission-denied, no-op, personal/lock guards
test_corpus_objs_service.py (~290 tests)	Same scenarios, all call targets repointed — regression coverage maintained
test_corpus_services_package.py	Shim/facade tests correctly dropped; CorpusService added; deletion pinned with ModuleNotFoundError
test_corpus_visibility.py	Patch targets and corpus_id assertion updated
PR author's test run	438 + 444 passed across the relevant test files

One gap: no integration-level test verifying assert_embedder_change_allowed fires end-to-end through UpdateCorpusMutation. The service-unit tests in TestCorpusServiceEmbedderGuard are solid, but a mutation-level test (similar to test_corpus_visibility.py) would fully close the loop.

---

Verdict

Clean, disciplined refactor with no semantic changes to production behaviour. Architecture is sound, migration is complete, documentation is thorough, test coverage is solid. The two nits (bare assert style, missing corpus-creation context in the class docstring) are cosmetic. The acknowledged corpus_id type change is low-risk and properly tested.