PR 1825 Follow Up

[JSv4]

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.

Originally posted by @claude[bot] in #1825 (comment)