Canonical-CAML corpus description: one source, derived projections

[JSv4]

Summary

Consolidates corpus description storage to a single canonical source: the corpus's Readme.CAML Document body. Eliminates four-place description sprawl (Corpus.md_description FileField, Corpus.description, Corpus.description_preview added by #1805, and the Readme.CAML Document) by making Corpus.description / description_preview / readme_caml_document auto-maintained read-only cache columns refreshed via signal on Readme.CAML save.

GraphQL surface is fully backward-compatible: description, descriptionPreview, mdDescription, and descriptionRevisions preserve their observable contracts; only the underlying storage shifts. Additive readmeCamlDocument field exposes the canonical Document directly for clients that want revision history / permissions / version-tree access.

Replaces #1805 — the architectural critique that surfaced during that PR's review (that the bug is canonical-source consolidation, not a missing field) is addressed here.

What changed

• Single source of truth — Readme.CAML Document body. All reads + writes converge on it.
• One write mechanism — both the editor (CorpusService.update_description) and the agent's targeted-replacement tool (aapply_caml_article_edit) now route through opencontractserver.documents.versioning.import_document, which atomically creates a new Document sharing the existing version_tree_id and flips DocumentPath.is_current flags. Frontend deep-links resolve to the head of the version tree via corpus.readmeCamlDocument (or DocumentPath join with is_current=True), no longer to a specific Document pk.
• Revision history uses existing Document.version_tree_id siblings — CorpusDescriptionRevisionType becomes a graphene.ObjectType facade exposing the historical id / version / author / snapshot / created shape (drop diff; clients compute via difflib from successive snapshots).
• Export schema bumped to V3 — drops md_description and md_description_revisions from the top-level payload; Readme.CAML rides in annotated_docs like any other Document. V2 imports remain supported via a back-compat shim that synthesizes a Readme.CAML doc + replays revisions as version-tree siblings + runs oc-import:// link rewriting. Validator gates required fields by version.
• Removed — Corpus.md_description FileField, CorpusDescriptionRevision model + table, model-level update_description / _read_md_description_content / _summarize_for_preview / _markdown_to_plain_text helpers, Corpus.save() override branch for description_preview (signal owns it now), LandingMarkdownSection styled component (PR Corpus description preview field + render markdown on the corpus home page #1805 workaround).

Migrations

• 0051_add_readme_caml_fk.py — adds the Corpus.readme_caml_document FK column.
• 0052_canonical_caml_backfill.py — for every Corpus with non-empty md_description, creates a Readme.CAML Document + DocumentPath, replays each CorpusDescriptionRevision row as a version-tree sibling (preserving historical created timestamps), populates the cache columns. Idempotent via lookup-before-create on DocumentPath. Large deployments should schedule this in a maintenance window.
• 0053_drop_legacy_description_storage.py — drops Corpus.md_description column + CorpusDescriptionRevision table. Forward-only.

Spec

docs/superpowers/specs/2026-05-27-canonical-caml-description-refactor-design.md

Test plan

• docker compose -f test.yml run --rm django pytest opencontractserver/tests/test_corpus_description_cache.py opencontractserver/tests/test_corpus_description.py opencontractserver/tests/test_corpus_canonical_caml_migration.py opencontractserver/tests/test_v2_import_back_compat.py opencontractserver/tests/test_export_v3.py opencontractserver/tests/test_caml_review_tools.py opencontractserver/tests/test_llm_corpus_patch_tools.py opencontractserver/tests/test_corpus_service.py --create-db → 116 passed
• cd frontend && npx tsc --noEmit → clean
• Apply migrations 0051–0053 on a non-empty DB and confirm every Corpus with md_description content now has a Readme.CAML Document + populated cache columns
• Manual: edit a corpus description via the editor, confirm the Readme.CAML doc gets a new version-tree sibling and the card excerpt refreshes
• Manual: export a corpus as V3, validate via validate_export.py, dry-run import into a staging instance
• Manual: import a legacy V2 fixture, confirm the synthesized Readme.CAML doc receives the md_description body with oc-import:// links rewritten
• Run yarn test:ct --reporter=list -g "CorpusHome|CorpusLandingView|CorpusListView|CorpusDescriptionEditor|AddToCorpusModal"

Caveats

• One small mutation of historical migration 0018_corpus_md_description_corpusdescriptionrevision.py inlines its calculate_description_filepath helper (live model function is gone in 0053). Fresh-install safe.
• frontend/src/graphql/queries.ts + mutations.ts drop the diff field from descriptionRevisions selection — the GraphQL type no longer exposes it.
• The agent tool update_corpus_description and CorpusService.update_description now return a Document (the new head) instead of a CorpusDescriptionRevision. The corpus-mutations resolver derives the integer version via calculate_content_version; the pydantic_ai tool wrapper wraps the helper in sync_to_async. Test fixtures updated.

[claude]

Code Review: Canonical-CAML Corpus Description Refactor

This is an excellent architectural refactor that correctly identifies and eliminates a four-way description sprawl. The core design — canonical source in Readme.CAML Document body, derived cache columns refreshed via signal — is sound. The backward-compat story (V2 import shim, preserved GraphQL surface) is well-thought-out. Below are the issues I found, roughly ordered by severity.

---

Bugs / Correctness

1. Stale comment in description_cache.py (line 150–156) — backfill_caml_doc_for_corpus

The docstring says:

"The Document post_save signal (Task 3, not yet shipped) will eventually cascade-refresh the cache columns…"

The signal is shipped in this PR (corpuses/signals.py). This will mislead anyone who reads backfill_caml_doc_for_corpus and thinks the signal is still missing.

2. Unprotected binary fallback in migration 0054's _read_md_description

except Exception:
    field.open("rb")   # ← no try/except; propagates on storage error
    try:
        return field.read().decode("utf-8", errors="ignore")
    finally:
        field.close()

If field.open("rb") raises (corrupted storage, missing blob), the exception escapes the outer handler and halts the migration mid-run. For a data migration processing potentially thousands of corpora, one corrupted file should be logged and skipped, not abort the whole run. The identical read_caml_body helper in description_cache.py has the same shape — check that too.

3. Migration numbering mismatch in PR description

The PR description mentions migrations 0051, 0052, 0053 but the actual files are 0053_add_readme_caml_fk.py, 0054_canonical_caml_backfill.py, 0055_drop_legacy_description_storage.py. Minor, but confusing for anyone following the migration trail in the changelog or backport notes.

---

Performance

4. N+1 query in CorpusDescriptionRevisionType.resolve_version (corpus_types.py)

def resolve_version(self, info) -> Any:
    ordered_ids = list(
        Document.objects.filter(version_tree_id=self.version_tree_id, ...)
        .order_by("created", "pk")
        .values_list("pk", flat=True)
    )
    return ordered_ids.index(self.pk) + 1

resolve_description_revisions already fetches all siblings as a list. resolve_version then re-queries the full tree per revision item. For a corpus with N revisions, requesting descriptionRevisions { version } is N+1 queries.

The fix is straightforward: annotate the 1-based position in the list resolver and attach it as an attribute, or pass a version_index map via graphene.ResolveInfo. The comment acknowledges this but the "fine for modal-only viewer" rationale doesn't hold if the modal ever shows the list with versions highlighted. Consider adding a # TODO: annotate version in list resolver if this becomes hot note, or just fix it now since the list resolver already has the ordered set.

5. resolve_snapshot does per-revision file I/O

Each snapshot field read triggers a txt_extract_file.open() call. In combination with the N+1 resolve_version this makes the revision history modal particularly expensive. Again, acknowledged as "modal-only viewer usage" — just make sure the future Task 16 frontend query removes snapshot from the list query and only fetches it on a single-revision drill-down.

---

Code Quality / Conventions

6. Private _refresh_description_cache_for_corpus imported across 3 unrelated modules

import_v2.py, import_tasks_v2.py, and _corpus_fixture.py all do:

from opencontractserver.corpuses.signals import _refresh_description_cache_for_corpus

Importing a _-prefixed private symbol across module boundaries is a CLAUDE.md anti-pattern (DRY + Single Responsibility). The function is semantically a cache utility, not a signal implementation detail. It should live in description_cache.py (or be a public function in signals.py). The module-level re-export comment for _read_caml_body already acknowledges this tension — that function was promoted to description_cache.py precisely to enable cross-module use without importing from signals. _refresh_description_cache_for_corpus deserves the same treatment.

7. corpus_service.py imports _read_caml_body from signals instead of the public description_cache module

from opencontractserver.corpuses.signals import _read_caml_body

The signals module re-exports read_caml_body under the private alias _read_caml_body "so legacy importers don't need to change." But corpus_service.py is new code in this PR — there's no legacy obligation. It should import from the canonical public location:

from opencontractserver.corpuses.services.description_cache import read_caml_body

8. backfill_caml_doc_for_corpus in description_cache.py has no production callers

The function is defined, documented, and tested, but no non-test code calls it. The migration uses its own inline logic. If it's intended as a management-command helper for future use, a brief # Used by management command X note (or a TODO) would clarify intent. As-is it looks like dead code (against CLAUDE.md rule 1: "No dead code").

9. fork_corpus — from opencontractserver.constants... import inside a loop

for ...:
    from opencontractserver.constants.document_processing import (
        CAML_ARTICLE_TITLE, MARKDOWN_MIME_TYPE,
    )
    if doc.title == CAML_ARTICLE_TITLE and doc.file_type == MARKDOWN_MIME_TYPE:

The import is cheap (cached after first call), but placing a module-level import inside a loop body is misleading style. Move it to the top of the function or the module's imports.

---

Security / Data Safety

10. readme_caml_document FK uses SET_NULL rather than PROTECT

If the canonical CAML Document is hard-deleted (e.g., via the admin or a bulk cleanup task), the FK silently becomes NULL and the next signal refresh zeros out corpus.description. There's no guardian permission protecting the CAML Document from deletion by anyone who has document-level DELETE on it. Consider either:

• Using PROTECT (like other load-bearing FKs in the codebase) and documenting that the CAML Document must be soft-deleted via DocumentPath first.
• Or adding a pre-delete check in the Document model / signal that prevents hard-deleting a Readme.CAML Document while it's referenced as a corpus's canonical doc.

The current clear_corpus_description_cache_on_caml_delete signal at least keeps the columns consistent if a delete does happen, which is good defensive coding.

---

Minor / Nits

• V3_REQUIRED_FIELDS in validate_export.py only lists V2-era additions, not V1 base fields. This is fine if V1 fields are checked elsewhere, but a brief comment explaining "V1 base fields are checked by _check_v1_required_fields" would prevent confusion.
• Several inline imports (e.g., from ... import CAML_ARTICLE_TITLE inside resolve_description_revisions) can be top-of-file imports without any circular-import risk. Consider moving them up for readability.
• The CorpusDescriptionRevisionType docstring mentions "Task 15 of the canonical-CAML refactor drops it [the legacy model]" — that already happened in migration 0055. The "on its way out" framing is stale.

---

Summary

The refactor is solid: single canonical source, signal-driven cache, backward-compatible GraphQL surface, and a well-sequenced set of migrations. The main actionable items before merge:

1. Fix the binary-fallback error handling in _read_md_description (migration 0054) — high risk in production.
2. Move _refresh_description_cache_for_corpus to description_cache.py or make it public — prevents future confusion.
3. Update corpus_service.py to import read_caml_body from its canonical location.
4. Fix the stale "not yet shipped" comment in description_cache.py.
5. Decide on the readme_caml_document FK deletion semantics and document the chosen approach.

[claude]

Code Review: Canonical-CAML Corpus Description Refactor

Overview

This is a well-scoped, architecturally sound refactor that eliminates four-way description storage drift by making the Readme.CAML Document body the single canonical source of truth. The GraphQL surface is backward-compatible, the migration is split cleanly into schema -> data -> schema-cleanup, and the export schema version bump is accompanied by a proper back-compat shim.

116 tests passing and TypeScript compiling clean are solid signals. The review focuses on issues that could surface in production or degrade correctness under edge cases.

---

Bugs / Correctness

[CRITICAL] backfill_caml_doc_for_corpus uses the wrong body for cache refresh when an existing doc is found

In opencontractserver/corpuses/services/description_cache.py, the else/doc=existing branch calls compute_cache_from_caml_body(md_description_body) where md_description_body is the caller-supplied legacy archive body, NOT the body stored in the existing document. If they differ, the cache columns end up reflecting the wrong content.

Fix: after doc = existing, reassign md_description_body = read_caml_body(doc) before computing plain/preview.

There is no test exercising the existing-is-not-None branch of backfill_caml_doc_for_corpus.

---

[MEDIUM] assert used as a runtime guard in production code

In opencontractserver/llms/tools/core_tools/descriptions.py, assert author_id is not None is stripped by Python -O/-OO flags. Should be raise ValueError(...). In production containers running with -O, this silently passes None to get(pk=None), surfacing as an unrelated DoesNotExist far from the real cause.

---

[LOW] Signal handler CAML identification relies on a user-editable title

_is_readme_caml_document keys on doc.title == "Readme.CAML" and file_type == "text/markdown". A user who renames their Readme.CAML doc or creates another doc with that exact title + MIME type will trigger spurious cache refreshes. Worth documenting the fragility with a comment or adding model-level protection.

---

Security / Permissions

[MEDIUM] get_corpus_description does the CAML lookup as the corpus creator, not the calling agent's user

In opencontractserver/llms/tools/core_tools/descriptions.py, CorpusDocumentService.get_corpus_caml_articles(corpus.creator, corpus) always passes the creator's identity. If an agent invokes this on behalf of a user without corpus READ access, the lookup silently succeeds, bypassing per-user permission checks. The actual caller's user should be threaded in here.

---

Performance

[MEDIUM] resolve_snapshot triggers one storage I/O per revision

CorpusDescriptionRevisionType.resolve_snapshot calls read_caml_body(self) which opens and reads txt_extract_file per sibling. The parent resolver uses select_related("creator") but does not bulk-read file bodies. If the client requests all revisions including snapshot, that is N storage round-trips. Consider capping revision count or adding pagination before revision history becomes unbounded.

[LOW] resolve_description_revisions loads entire version tree into memory

oldest_first = list(...) materialises all siblings with no upper bound. Fine now but should be capped or paginated before revision count grows large (e.g. active corpora with frequent agent edits).

---

Code Quality / Conventions

[LOW] Function-level import of calculate_content_version inside the GraphQL mutation

In config/graphql/corpus_mutations.py, calculate_content_version is imported inside the mutate method body. The project uses module-level imports generally; if there is no circular dependency forcing this, it should be at the module top.

[LOW] Migration doc comments vs. actual migration numbers

The PR body and 0055_drop_legacy_description_storage.py docstring reference "migration 0052" for the backfill and "0053" for the drop, but the actual files are 0053_add_readme_caml_fk, 0054_canonical_caml_backfill, and 0055_drop_legacy_description_storage. The PR Migrations section also lists 0051-0053. This mismatch will cause confusion for ops teams running the migration.

[LOW] CorpusDescriptionRevisionType dropped relay.Node - verify client global ID assumptions

The type changed from DjangoObjectType (base-64 relay global ID) to a plain graphene.ObjectType whose resolve_id returns the raw Document pk. Any client storing or comparing the id field across the refactor boundary will see different values. The Apollo cache may have stale relay-encoded entries from GET_CORPUS_WITH_HISTORY. Should be verified in the Playwright component tests listed in the test plan.

---

What is Well Done

• The three-migration split (schema -> data -> schema-cleanup) with the maintenance-window callout is exactly the right operational approach for a destructive schema change.
• Inlining calculate_description_filepath into migration 0018 to keep historical migrations importable is documented clearly and correctly done.
• The on_commit deferral in all signal handlers is correct - blobs are durable before cache refresh reads them back. The captureOnCommitCallbacks pattern in tests handles this properly.
• The V2 back-compat shim is a clean no-op on V3 archives (early-return on empty input) without any branching in the hot import path.
• with_readme_caml_doc with select_related prevents the N+1 on every corpus list render.
• Removing the storage orphan-cleanup callback in caml_article.py is the right call now that import_document retains historical blobs as version-tree siblings.
• The _version_index pre-annotation trick in resolve_description_revisions correctly avoids an N+1 per-row query for the version number.

---

Summary

The critical item to resolve before merge is the backfill_caml_doc_for_corpus wrong-body bug - the existing-doc branch should read the body from the document rather than using the legacy-field argument. The assert guard and the inline import are quick mechanical fixes. The permission semantics in get_corpus_description and the title-based CAML identification are worth addressing or documenting as accepted risks. Everything else is polish.

[claude]

Code Review — PR #1825: Canonical-CAML Corpus Description

The architectural intent here is sound: four-way description sprawl (FileField, plain-text cache, preview cache, Readme.CAML Document) is collapsed into a single canonical source with auto-maintained projections. The service-layer routing, backward-compatible GraphQL surface, and V2→V3 export schema are well thought out. Below are my findings, roughly ordered by severity.

---

Medium — Migration 0054 imports from live service code

0054_canonical_caml_backfill.py imports compute_cache_from_caml_body from opencontractserver.corpuses.services.description_cache at run time. Django migrations are supposed to be self-contained snapshots. If that function is ever renamed, moved, or its signature changes, this historical migration breaks on fresh installs. The per-corpus migration-local functions (_create_caml_doc, _replay_revisions) are correctly self-contained; the cache derivation should be too. Consider inlining the two-line markdown_to_plain_text → summarize_for_preview logic directly in backfill_all, or extracting it into a migration-local helper (just string transforms, no ORM). The same pattern applies to the modified 0050_corpus_description_preview.py, which now calls summarize_for_preview from the live service.

---

Medium — resolve_snapshot does N file reads on the revision modal

CorpusDescriptionRevisionType.resolve_snapshot calls read_caml_body(self), which opens the txt_extract_file FieldFile. When the frontend opens the revision history modal, GraphQL will call this resolver once per sibling in the version tree — each call is a synchronous file-storage read. For a corpus with dozens of revisions this becomes N round-trips to object storage (S3/GCS). The legacy implementation stored the snapshot in a DB column, so the list resolver could fetch all snapshots in a single query. If the revision modal loads all snapshots eagerly (which the current GET_CORPUS_WITH_HISTORY query suggests), consider bulk pre-reading the bodies in resolve_description_revisions and stashing them on the instances, or serve a URL the client fetches on demand rather than inlining the body.

---

Low — _is_readme_caml_document keys on user-editable fields

The signal pre-check keys on doc.title == CAML_ARTICLE_TITLE and doc.file_type == MARKDOWN_MIME_TYPE. The PR documents this fragility, but worth tracking the structural-marker follow-up as a concrete ticket so it does not get lost — a flag field on Document or a CorpusDocumentRole enum on DocumentPath would harden this.

---

Low — get_corpus_description uses corpus.creator for the CAML lookup

The accepted-risk comment in opencontractserver/llms/tools/core_tools/descriptions.py is clear. Still worth flagging: if a corpus creator is deleted or deactivated, corpus.creator could be None, and get_corpus_caml_articles would need to tolerate that. A defensive null guard would prevent a surprise AttributeError.

---

Low — Locking semantics in _apply_caml_article_edit

The function acquires select_for_update on the old Document row, but the write goes through import_document which creates a new Document and flips DocumentPath.is_current. The comment says import_document also takes select_for_update on the DocumentPath row. If outer and inner locks have different ordering or granularity this could deadlock under concurrent writes. Please verify (or add a test) that two concurrent calls on the same corpus serialize cleanly without deadlock.

---

Low — Historical migration 0018 modified inline

Inlining calculate_description_filepath is the right fix for fresh-install correctness. Worth confirming in the PR that existing deployments which have already run migration 0018 will not re-run it (Django skips already-applied migrations regardless of content changes) — this is standard behavior, but the note prevents confusion.

---

Nit — resolve_description_revisions eager-loads all siblings into Python memory

For a corpus with hundreds of description versions, materializing all of them is wasteful. Consider adding a reasonable cap with a has_more flag, or relay-style pagination, for long-lived corpora.

---

Positive callouts

• Service layer discipline is clean throughout: CorpusService.update_description → import_document is the single write path, and the GraphQL resolver, agent tool, and V2 import shim all go through it.
• with_readme_caml_doc as a queryset helper wired at both list and single-object entry points avoids the per-row readme_caml_document N+1.
• transaction.on_commit deferral in signal handlers is the right pattern; captureOnCommitCallbacks usage in tests is correct.
• The CorpusDescriptionRevisionType facade preserving the legacy GraphQL shape while backing it with Documents is a clean migration strategy.
• The V2 back-compat shim being a clean no-op on V3 archives is a nice design property.
• Test coverage for the signal handlers, version-tree sibling semantics, and idempotent backfill is thorough.
• The fork task correctly skipping the title prefix for the Readme.CAML Document is a good catch.

---

Summary: The core refactor is architecturally correct and the implementation is mostly clean. The migration's live-service import is the most durable concern; the N file reads on the revision snapshot resolver is the most user-visible performance risk. Everything else is low-severity or has a documented follow-up plan.

[claude]

Code Review — Canonical-CAML Corpus Description Refactor

Overview

This is a well-scoped architectural refactor that consolidates corpus description storage from four independent locations into a single source of truth (the Readme.CAML Document body), with Corpus.description, description_preview, and readme_caml_document as auto-maintained read-only projections. The design is clearly specified, the backward-compat shim for V2 imports is present, and the 116-test suite gives reasonable coverage. The migration sequencing (schema → data → drop) is correct.

That said, there are a few issues worth addressing before merge.

---

Bug: Migration 0054 computes cache from wrong source when head already exists

File: opencontractserver/corpuses/migrations/0054_canonical_caml_backfill.py, backfill_all

body = _read_md_description(corpus)   # reads the md_description FileField
head = _get_existing_caml_doc(...)
if head is None and body:
    head = _create_caml_doc(...)
if head is not None:
    plain, preview = _compute_cache_from_caml_body(body or "")  # uses md_description body

When head is not None but body is "" (a corpus that already has a DocumentPath-linked CAML doc but no md_description content), the cache columns are computed from the empty string and zeroed out — even though the existing head document may have real content. The correct fix is to read the cache body from the head document when body is empty:

effective_body = body or _read_body_from_head(head)
plain, preview = _compute_cache_from_caml_body(effective_body)

In practice, no corpus should have a CAML doc without md_description before this migration runs on a production DB, but the code should be defensively correct for any DB state, especially since the migration is advertised as idempotent.

---

Breaking change: CorpusDescriptionRevisionType drops the Relay Node interface

File: config/graphql/corpus_types.py

The type changed from DjangoObjectType (with interfaces = [relay.Node]) to a plain graphene.ObjectType. The resolve_id resolver now returns a bare integer pk, not a relay-encoded global ID. Any client executing node(id: "<relay-encoded-revision-id>") will silently fail to resolve. The test plan doesn't include a check that no production frontend code uses this query path.

The comment in resolve_version about "falls back to a per-row query when the instance is resolved outside that list path (e.g. node(id:) — uncommon for this facade type)" is also now unreachable since the type has no relay node registration.

---

Performance: resolve_snapshot is an unbounded per-revision storage read

File: config/graphql/corpus_types.py, CorpusDescriptionRevisionType.resolve_snapshot

The per-revision storage round-trip is acknowledged and tracked as Task 16, which is the right call. However, GET_CORPUS_WITH_HISTORY selects snapshot by default in the list query, meaning the revision-history modal will fire N storage reads on open. A short-term mitigation (e.g., cap the resolver at 20 revisions) would bound worst-case latency until Task 16 lands.

---

refresh_description_cache_for_corpus doesn't filter by document file_type

File: opencontractserver/corpuses/services/description_cache.py

head_path = DocumentPath.objects.filter(
    corpus_id=corpus_id,
    path=CAML_ARTICLE_TITLE,
    is_current=True,
    is_deleted=False,
).select_related("document").first()

This selects any current DocumentPath named "Readme.CAML", regardless of whether the linked Document has file_type=text/markdown. Signal handlers guard against non-markdown docs via _is_readme_caml_document, but refresh_description_cache_for_corpus (called directly from the import task and the V2 shim) has no such filter. Add document__file_type=MARKDOWN_MIME_TYPE to be consistent.

---

Accepted-risk note in LLM tool should be tracked as an issue

File: opencontractserver/llms/tools/core_tools/descriptions.py

The CAML lookup uses corpus.creator instead of the calling agent's user. This is correctly documented as an accepted trade-off, but it diverges from the project's "always go through services/ with a user context" invariant. Since the architecture enforcement test only scans config/graphql/, this will remain undetected by CI. A link to a tracking issue in the comment would help ensure it isn't forgotten.

---

Minor / style

• PR description migration numbers are stale: the description says 0051, 0052, 0053 but the actual files are 0053, 0054, 0055. Not a code issue, but confusing when cross-referencing the spec.

• Inline imports in corpus_types.py resolvers (resolve_description_revisions, resolve_version): necessary to break circular imports, but they fire a dict-lookup on every resolver call. Module-level TYPE_CHECKING guards or lazy module attributes would be cleaner.

• Concurrent write race in CorpusService.update_description: the no-op check (read current body → compare → skip) is not atomic with the subsequent import_document call. Two simultaneous saves of the same content would both pass and create a duplicate version entry. Low probability and idempotent in content, but a select_for_update on the corpus row or a docstring note would make the invariant explicit.

---

Summary

The architecture is sound and the implementation is thorough. The migration bug (cache zeroed from the wrong source) is the one issue that could cause silent data loss in certain DB states. The relay node interface removal and the refresh_description_cache_for_corpus filter gap are worth fixing before merge. The rest are improvement opportunities that can be tracked as follow-ups.

[claude]

Code Review: Canonical-CAML Corpus Description (#1825)

Overall: This is a well-architected refactor that correctly identifies and fixes a multi-source-of-truth problem. The signal-based cache pattern, backward-compatible V2 import shim, and self-contained historical migration fixes are all solid. A few issues worth addressing before merge.

---

Issues

Major

Migration 0054 — cache recomputation uses wrong body in idempotent re-run path

In backfill_all (0054_canonical_caml_backfill.py), when head exists but body is empty (an existing Readme.CAML doc with no legacy md_description — the idempotent re-run path after md_description was already cleared), the if head is not None block calls _compute_cache_from_caml_body(body or ""), which zeroes the cache even though the CAML doc has real content.

This is unreachable in the normal 3-migration sequence (0053 → 0054 → 0055), but becomes a real data-loss scenario if ops ever need to re-run 0054 standalone after data repair — exactly the documented use case for an idempotent migration. The fix is to read the existing CAML doc's body when body is empty:

if head is not None:
    cache_body = body if body else _read_caml_doc_body(head)
    plain, preview = _compute_cache_from_caml_body(cache_body)

The live backfill_caml_doc_for_corpus in description_cache.py already does this correctly (it calls read_caml_body(doc) on the existing doc), so the fix is aligning the migration's local copy with it.

---

Minor

Direct Document.objects.filter() inside config/graphql/ resolvers

corpus_types.py has two raw ORM accesses inside config/graphql/:

• CorpusType.resolve_description_revisions — Document.objects.filter(version_tree_id=tree_id, ...)
• CorpusDescriptionRevisionType.resolve_version fallback — same pattern

These won't trigger opencontracts.E001 (which scans for inline Tier-0 permission calls), but they violate the CLAUDE.md service layer rule: config/graphql/ code must reach models through services/. A CorpusDocumentService.get_caml_revision_history(corpus) classmethod would also make the _version_index annotation logic independently testable.

Task-number references in inline comments will rot

Per CLAUDE.md: task/issue references belong in the PR description, not inline code comments. The following will become opaque noise as the task backlog moves:

• corpus_mutations.py: comment referencing "Task 9 will reimplement descriptionRevisions"
• corpus_types.py: comments referencing "Task 9 of the canonical-CAML refactor" and "Task 16 will update the frontend query"

Replace each with a description of the invariant/constraint, or remove (the spec link already covers intent).

Migration number mismatch in PR description

The PR summary references migrations 0051_add_readme_caml_fk, 0052_canonical_caml_backfill, 0053_drop_legacy_description_storage, but the actual files are 0053, 0054, 0055. Worth correcting before merge to avoid ops confusion.

Accepted security deviation should have a concrete follow-up issue

descriptions.py get_corpus_description uses corpus.creator as the CAML lookup identity rather than the calling agent's user (the accepted-risk comment at ~line 49 correctly documents this). Worth confirming the follow-up is filed as an open issue before this ships, since it's a permission-model deviation even if the blast radius is low today.

---

Positive notes

• Signal handler correctly defers cache refresh to on_commit — prevents mid-transaction stale reads.
• _version_index pre-annotation in resolve_description_revisions prevents N+1 version lookups. The acknowledged N storage round-trips for resolve_snapshot are a fair trade-off given a human-browsable revision count.
• Historical migrations 0018 and 0050 are correctly made self-contained (helpers inlined, live-model imports removed) — the right approach for migration hygiene.
• with_readme_caml_doc select_related in both corpus_queries.py and CorpusType.get_queryset eliminates per-row Document fetches on list views.
• V2 import shim correctly identifies the idempotent sentinel (annotated_docs already has a Readme.CAML → skip) and threads oc-import:// rewriting only through the synthesized body, not historical revision snapshots.
• backfill_caml_doc_for_corpus in the live service already uses read_caml_body on existing docs — making the migration the only inconsistent place, which is the root of the issue above.

[claude]

Code Review: Canonical-CAML Corpus Description Refactor

Overview

This is a well-scoped architectural refactor that consolidates corpus description storage from four divergent sources down to one canonical source (the Readme.CAML Document body), with derived read-only cache columns refreshed via Django signals. The design is sound and the implementation is generally clean. A few issues below are worth addressing before merge.

---

Strengths

• Single source of truth is clearly maintained. The readme_caml_document FK + with_readme_caml_doc queryset helper prevents the O(N) per-row Document fetch that the old resolver pattern would have created.
• Signal architecture is correct. Using transaction.on_commit defers cache refresh until the CAML write is durable. The Corpus.objects.filter().update() pattern avoids re-firing Corpus.post_save, keeping the loop-free invariant.
• Migration is production-safe. Separating schema add (0053), data backfill (0054), and schema drop (0055) into three migrations lets ops run 0054 in a maintenance window on large datasets independently. The migration-local helper freezing is the correct approach.
• Backward-compatible GraphQL surface. Preserving description, descriptionPreview, mdDescription contracts with only additive readmeCamlDocument is the right call.
• Test coverage is extensive. 116 tests covering the cache signal, V2 import back-compat, V3 export, and the version-tree sibling behaviour of CAML edits.

---

Issues

1. Fragile signal guard — magic string coupling (Medium)

The _is_readme_caml_document guard in signals.py keys on doc.title == CAML_ARTICLE_TITLE and doc.file_type == MARKDOWN_MIME_TYPE. The PR already calls this out as a known fragility. The concern is real: a user who renames their Readme.CAML doc, or creates an unrelated text/markdown Document titled "Readme.CAML", silently trips the cache-refresh signal. A dedicated boolean flag on Document (e.g. is_corpus_readme) would harden this without impacting signal performance. The follow-up note is appropriate, but given this is the load-bearing invariant for the entire cache, consider opening a tracking issue and adding a TODO(#NNN) reference rather than leaving it as a bare prose comment.

2. resolve_description_revisions performs an N+1 storage read on snapshot (Low-Medium)

CorpusDescriptionRevisionType.resolve_snapshot calls read_caml_body(self) per revision row. The docstring acknowledges the trade-off ("bounded by the revision count a human is browsing"). However the list resolver already pre-fetches all siblings in one query and annotates _version_index to avoid per-row re-queries. The same pattern could apply to the body: read all bodies in the list resolver, stash as _body on each instance, and have resolve_snapshot fall back to read_caml_body only when called outside the list path. This mirrors the _version_index optimization already in place and would avoid serializing N storage round-trips in the revision modal.

3. get_corpus_description scopes the CAML lookup as corpus.creator, not the calling user (Low)

In descriptions.py, CorpusDocumentService.get_corpus_caml_articles(corpus.creator, corpus) is used because no user is injected by the agent framework. The comment is explicit about this being gated upstream, and the if corpus.creator is None: return "" guard is correct. Consider adding a TODO(#NNN) to track the follow-up for threading the live caller through, so it does not live only in a comment.

4. Migration 0055 dependency chain omits the documents app (Low)

0055_drop_legacy_description_storage.py only declares ("corpuses", "0054_canonical_caml_backfill") as a dependency. Migration 0053 adds a FK to documents.Document, so 0055 (which drops columns referencing that FK) should also pin the corresponding documents migration to make the dependency graph unambiguous. Low risk since Django resolves FK drops safely, but a belt-and-suspenders entry prevents surprises during future squash operations.

5. Lock-ordering in _apply_caml_article_edit: Corpus is fetched without a lock inside the transaction (Medium)

Inside the transaction.atomic() block, Document.objects.select_for_update().get(pk=locked_doc.pk) acquires a lock, but the subsequent Corpus.objects.get(pk=corpus_id) does not. If a concurrent corpus delete or a signal update to readme_caml_document_id races between the Document lock and the import_document call, the corpus object could be stale. Adding select_for_update() to the Corpus fetch inside the transaction would close this window. The lock ordering (Document to Corpus to DocumentPath) should also be documented explicitly to prevent future deadlocks.

6. Mutating historical migration 0018 (Low — documented)

The PR caveats this correctly. If your CI uses migration hash checking against a stored checksum (e.g. squawk or a custom check), mutating 0018 will cause migrate --check to diverge. Verify this does not affect your CI migration-check job before merging.

---

Minor Nits

• Over-commented code. CLAUDE.md guidelines say "default to writing no comments" and "only add one when the WHY is non-obvious." Many new comments in signals.py, description_cache.py, and corpus_types.py explain what the code does rather than why. The docstrings on resolve_snapshot (14 lines) and resolve_description_revisions (20 lines) could be cut by ~60% without losing meaning.

• Inconsistent lazy import pattern. signals.py mixes a module-level import (with # noqa: E402) placed below function definitions with inline imports inside method bodies. Consistent placement — all at top or all at function scope — would be cleaner.

• Trivial noop_reverse helper. Django's RunPython accepts migrations.RunPython.noop as the canonical no-op reverse, eliminating the wrapper function.

• Test class name typo. AapplyCamlArticleEditCreatesVersionTreeSiblingTest — the leading Aa looks like a merge artefact. Should probably be ApplyCamlArticleEditCreatesVersionTreeSiblingTest.

---

Test Plan Gaps

The automated test plan is strong. The three unchecked manual items (migration on a non-empty DB, live editor round-trip, V3 export/import smoke test) are the ones most likely to surface edge cases. Also confirm that validate_export.py's version-gate rejects a V2 payload parsed as V3 and vice-versa — the schema bump (removing top-level md_description) is a breaking change for any client that parses export archives directly.

---

Summary

The architecture is correct — consolidating to a single canonical source eliminates the description-drift problem cleanly. The signal-driven cache refresh, version-tree approach, and three-phase migration strategy are all solid. Items 1 and 5 (fragile signal guard and unguarded Corpus fetch inside the lock boundary) are the ones worth addressing before merging. Everything else is polish.