Address PR #1825 review follow-ups: CAML edit lock ordering + description follow-up tracking (#1848)

[JSv4]

Addresses #1848 — the follow-up review of the Canonical-CAML corpus description refactor (PR #1825).

Each of the six reported issues was evaluated against the actual code before deciding whether to change anything. Three warranted a code change; the others were assessed as already-correct or premise-incorrect, and the reasoning is recorded below.

Changes made

Item 5 (Medium) — lock ordering in _apply_caml_article_edit

opencontractserver/llms/tools/core_tools/caml_article.py. The function locked the head Document (select_for_update) inside transaction.atomic() but then fetched Corpus.objects.get(pk=corpus_id) unlocked right before import_document. It now fetches the Corpus with select_for_update() too, with a comment documenting the lock order (Document → Corpus → DocumentPath) so the read-modify-write can't act on a stale Corpus and concurrent writers acquire locks in a consistent order. (The identical construct is already used on main in opencontractserver/agents/memory.py.)

Item 1 (Medium) — fragile _is_readme_caml_document signal guard

opencontractserver/corpuses/signals.py. The guard keys on title == CAML_ARTICLE_TITLE and file_type == MARKDOWN_MIME_TYPE. A false positive is already harmless (the deferred refresh re-derives the owning corpus via the canonical DocumentPath head and no-ops when there is none). The bare "left as a follow-up" prose is replaced with a concrete TODO(#1848) pointing at the Document.is_corpus_readme hardening.

Item 3 (Low) — get_corpus_description scopes the CAML lookup as corpus.creator

opencontractserver/llms/tools/core_tools/descriptions.py. The accepted-risk comment becomes a TODO(#1848) so "thread the live caller through once the agent framework injects it, and drop the corpus.creator fallback" is tracked.

Nit — test class typo

opencontractserver/tests/test_caml_review_tools.py: AapplyCamlArticleEditCreatesVersionTreeSiblingTest → ApplyCamlArticleEditCreatesVersionTreeSiblingTest.

CI fixes on this branch

Migration 0018 — an intermediate commit on this branch had reverted #1825's inlining of calculate_description_filepath into the historical 0018 migration, reintroducing a dangling opencontractserver.corpuses.models.calculate_description_filepath reference (removed in the refactor) and breaking Django's migration import. Commit c17b59d restores the inlined helper; git diff main HEAD -- .../0018_*.py is now empty (identical to main, importable on fresh installs). This resolves the earlier 🔴 review note.

pytest (test_caml_review_tools.py) — the branch forked from main at 8f53b24, before main's commit 26a864b ("Fix CI on main: route CAML test fixture through import_document (#1838 follow-up)"). #1838 added a leading-/ precondition to CorpusPathService.disambiguate_path; the test helper _create_caml_doc still built its fixture via corpus.add_document(path="Readme.CAML"), which now correctly rejects the slashless sentinel — failing all 48 tests in the file in setUp. This was a pre-existing breakage already fixed on main, unrelated to this branch's edits (production code paths.py/versioning.py/models.py is byte-identical to main). Merged origin/main (14425eb) to pick up the fixture fix. Verified locally: test_caml_review_tools.py + test_agent_memory.py pass (pytest exit 0), including the tests that directly drive the new Corpus lock.

Assessed — no code change needed

Item 2 (Low) — N+1 storage read in CorpusDescriptionRevisionType.resolve_snapshot

The suggestion was to pre-read every revision body in the list resolver. Doing so would regress the common case: resolve_description_revisions returns the revision list, but snapshot is resolved lazily per field. A query that lists revisions without selecting snapshot currently performs zero storage reads; pre-reading all bodies in the list resolver would force N reads even when the client wants none. The lazy per-field read (read_caml_body in resolve_snapshot) is the correct trade-off. No change.

Item 4 (Low) — migration dependency on the documents app

The premise doesn't hold against the actual files. 0053_add_readme_caml_fk already declares ("documents", "0039_add_preferred_enrichers_to_pipeline_settings"), and 0054_canonical_caml_backfill (which actually creates Documents/DocumentPaths) also pins it. 0055_drop_legacy_description_storage references no documents objects and inherits the dependency transitively via 0053 → 0054 → 0055. The graph is already correctly ordered. No change.

Item 6 — historical 0018 migration edit

PR #1825 correctly inlined calculate_description_filepath into 0018 because the model-level helper was removed in the refactor; inlining keeps the historical migration importable on fresh installs. This is necessary and correct, and the branch preserves it (see CI fixes above).

Remaining nits (non-blocking, not changed)

• noop_reverse → migrations.RunPython.noop in 0054: cosmetic and behaviour-identical. Left as-is to avoid churning an already-applied data migration for a style preference.
• Over-commenting / lazy-import consistency: subjective; left as-is.

https://claude.ai/code/session_01Wa4U1t8WG2uAff2JY9d7z1

[claude]

Code Review

Summary

Small, focused PR addressing three items from the #1825 review. The only functional change is the select_for_update() addition in caml_article.py; the other two changes are comment housekeeping. Overall the changes are correct and improve the codebase.

---

Functional change — caml_article.py line 528: Corpus lock

Verdict: Correct and appropriate.

Adding select_for_update() here plugs the real race window: the outer transaction.atomic() already holds a row-level lock on the Document, and without also locking Corpus, a concurrent corpus-delete or readme_caml_document_id cache refresh could mutate the corpus row between the Document lock and the import_document call. The lock order (Document → Corpus → DocumentPath inside import_document) is internally consistent: import_document issues its own DocumentPath.select_for_update() inside a nested transaction.atomic() that becomes a SAVEPOINT, so all three locks are held in the documented order within the same DB transaction.

One thing worth noting (non-blocking): select_for_update() with no nowait=True will block indefinitely if another transaction holds either the Document or Corpus row. For the CAML-edit flow this is likely fine (approval-gated, low concurrency), but if this function is ever called from a latency-sensitive path, consider switching to select_for_update(nowait=True) and surfacing a RetryLater-style error to the caller instead of an unbounded wait.

Pre-existing non-issue: Corpus.objects.select_for_update().get(pk=corpus_id) raises Corpus.DoesNotExist if the corpus was deleted between Document lock acquisition and the Corpus fetch. This was already the case with the unlocked .get() and is not a regression. Access validation upstream makes this path vanishingly unlikely.

Deadlock safety: The comment documents the intended lock order for this call site, which is good. If there are other call sites that touch both Corpus and Document rows under select_for_update() in the opposite order (Corpus → Document), that would be a latent deadlock. Worth a quick grep of other select_for_update() usages against Corpus to confirm no inversions exist — I did not spot any in a brief scan, but it is not exhaustive.

---

Comment changes — signals.py and descriptions.py

Both changes convert vague "left as a follow-up" prose into traceable TODO(#1848) markers with enough context to act on later. This is strictly better: the issue number creates a stable pointer that will not rot if someone reads this file months from now.

---

PR description note

The description lists Items 2, 4, 6, and nits as "still being addressed on this branch." If those are landing in subsequent commits on the same PR, the description should be updated as each one lands so reviewers are not left wondering whether any of those items are already included. If they are deferred to a follow-up PR, calling that out explicitly would avoid confusion.

---

Tests

No automated tests for the lock ordering change. Deterministic concurrency tests are hard to write well in Django, so the absence is not unreasonable, but a brief integration test using threading.Barrier or concurrent.futures.ThreadPoolExecutor to force both paths into the critical section simultaneously would give future refactors a safety net.

---

Bottom line: Approve the functional change. The other two changes are fine comment hygiene. No blocking issues.

[claude]

Code Review — PR #1855

Overview

This PR addresses three of the six reviewer items from the PR #1825 follow-up review (#1848): lock ordering in _apply_caml_article_edit, signal-guard comment hardening, and a get_corpus_description comment improvement. It also fixes a test-class name typo and edits migration 0018. The remaining items (N+1, migration dependency, historical migration verification, nits) are called out as still in progress.

---

✅ Approved Changes

Lock ordering — caml_article.py:531
Adding select_for_update() to the Corpus.objects.get() is correct. A concurrent corpus delete or readme_caml_document_id cache refresh between the Document lock acquisition and import_document is a real race. The lock-order comment (Document → Corpus → DocumentPath) is exactly the right documentation artifact here — future writers will know the ordering contract without hunting through import_document's internals.

Signal-guard comment — signals.py:337
TODO(#1848) with a concrete model attribute proposal (Document.is_corpus_readme) is a clear improvement over "left as a follow-up." The linked issue means the comment won't silently rot.

get_corpus_description comment — descriptions.py:59
Same reasoning — TODO(#1848) with the explicit remediation ("thread the invoking user through once the agent framework injects it, and drop the corpus.creator fallback") is actionable and tracked.

Test-class rename — test_caml_review_tools.py:1153
AapplyCamlArticleEditCreatesVersionTreeSiblingTest → ApplyCamlArticleEditCreatesVersionTreeSiblingTest. Correct fix; worth verifying no manage.py test invocations hard-code the old name (a quick grep -r AapplyCaml should confirm).

---

🔴 Critical Issue — Migration 0018

The migration change introduces a fresh-install breakage.

The removed code inlined calculate_description_filepath directly inside the migration file with an explicit docstring explaining why:

"Originally lived as opencontractserver.corpuses.models.calculate_description_filepath; the helper and the field it served were removed in migration 0053 … We inline the callable here so this historical migration remains importable on fresh installs even after the runtime model code drops it."

The replacement imports from the live model module:

upload_to=opencontractserver.corpuses.models.calculate_description_filepath,

But calculate_description_filepath does not exist in corpuses/models.py. A search across the entire repo confirms it appears only in this migration file. On a fresh install, Django will import migration 0018 early in the migration run (before 0055 removes the field), hit an AttributeError on opencontractserver.corpuses.models.calculate_description_filepath, and fail.

The original defensive inline was the correct approach. The PR description lists Item 6 ("verify/handle the historical 0018 migration edit") as still being addressed, but the current change makes things worse rather than neutral — the inline was working, and this replacement is broken.

Recommendation: Revert the migration 0018 change and restore the inlined function, or if there is a reason to reference the models module, add calculate_description_filepath back to models.py as a standalone top-level function (not on the Corpus class, since the field is gone) that is never imported anywhere except historical migrations.

---

⚠️ Minor Observations

Lock comment placement (caml_article.py:526–532): The comment is clear but slightly long for an inline block. Consider keeping just the one-line lock-order summary (# Lock order: Document → Corpus → DocumentPath) and moving the "why" prose into the function or class docstring. Not a blocker.

Incomplete PR state: The PR is explicitly partial (Items 2, 4, 6, and several nits are outstanding). If this is intended as an intermediate merge, the critical migration issue above needs resolution first. If it's a draft, marking it [Draft] would help reviewers calibrate.

---

Summary

Change	Verdict
select_for_update() on Corpus	✅ Correct fix
Signal guard TODO(#1848)	✅
get_corpus_description TODO(#1848)	✅
Test-class typo fix	✅
Migration 0018 rewrite	🔴 Breaks fresh-install migrations — calculate_description_filepath is not in models.py

Block on the migration issue; the rest is ready to merge.

[claude]

Code Review

Overview

This is a focused follow-up to PR #1825 addressing two medium-priority items from the prior review. The diff is small (13 additions, 4 deletions across 4 files) and the PR body is admirably transparent about what is and is not addressed here.

---

Lock Ordering Fix — caml_article.py ✅

The core substantive change: adding select_for_update() to the Corpus fetch inside _apply_caml_article_edit's transaction.atomic() block.

The fix is correct. Without it, a concurrent corpus delete or a readme_caml_document_id cache refresh could modify the Corpus row between the Document lock and the import_document call, causing the edit to write against a stale or tombstoned corpus.

One thing to verify: after this change, if the corpus was deleted between the Document lock acquisition and Corpus.objects.select_for_update().get(pk=corpus_id), a Corpus.DoesNotExist will propagate up. The prior unlocked get() had the same failure mode, but it is worth confirming this exception surfaces cleanly to the LLM as an error string per the agent tool fault-tolerance contract (operational exceptions → error string to LLM, not unhandled propagation).

The lock ordering comment is valuable and appropriate — documenting a hidden ordering invariant is exactly what the project comment policy calls for.

---

TODO(#1848) Conversions — signals.py + descriptions.py ✅

Both convert vague "left as a follow-up" prose into TODO(#1848) references with a concrete description. This is a strict improvement for traceability. No issues.

---

Test Class Typo Fix — test_caml_review_tools.py ✅

AapplyCamlArticleEditCreatesVersionTreeSiblingTest → ApplyCamlArticleEditCreatesVersionTreeSiblingTest. Clean. Worth a quick grep to confirm no CI script or manage.py invocation references the old class name by string — unlikely given the double-A, but easy to check.

---

Remaining Items (per PR body)

The PR body lists six items still outstanding. A few are higher-risk than the nits:

• Item 2 (N+1 storage read in CorpusDescriptionRevisionType.resolve_snapshot) — latency and scalability concern; should land before the CAML description feature gets broader use.
• Item 4 (migration dependency on the documents app) — migration dependency order bugs are silent until a squash or fresh DB setup. Worth landing soon.
• Item 6 (historical 0018 migration edit) — if this touches a migration already present in production databases, needs careful handling to avoid MigrationHistoryConsistencyError.

---

Summary

Approve with minor note. The lock ordering fix is correct and well-documented. The TODO conversions are clean housekeeping. The only thing worth a quick sanity check is the Corpus.DoesNotExist propagation path after the new lock — if the tool is already wrapped in the agent harness fault-tolerance layer, this is already handled.

[claude]

Code Review

Overview

Small, focused follow-up to PR #1825. Four files changed: one correctness fix (lock ordering), two comment-precision improvements (vague prose → trackable TODOs), and one test-class typo fix. The PR description is thorough and transparent about what was and wasn't changed and why — good practice.

---

Lock ordering fix — caml_article.py (Item 5)

The change is correct.

I traced the full lock graph:

Transaction step	Lock acquired
Document.objects.select_for_update().get(pk=doc.pk)	Document row (exclusive)
Corpus.objects.select_for_update().get(pk=corpus_id) ← new	Corpus row (exclusive)
Inside import_document: DocumentPath.objects.filter(...).select_for_update().first()	DocumentPath row (exclusive)

Checked codebase-wide: Corpus.objects.select_for_update() is only called here. No other path acquires Corpus → Document (opposite order), so the new Document → Corpus → DocumentPath order introduces no deadlock risk.

One minor wording point in the comment at line 526:

DocumentPath (locked inside import_document)

Inside import_document (versioning.py:196), DocumentPath is locked via .select_for_update().first() on a filtered queryset, which can return None if the path doesn't yet exist (new-document case). The comment implies a guaranteed lock; slightly more accurate would be "DocumentPath locked inside import_document when the path already exists." Non-blocking nit.

Also worth noting: the signal handler (refresh_description_cache_for_corpus) is called via post_save inside the same transaction.atomic() block (through Django savepoints from import_document's nested atomic). It does a plain Corpus.objects.filter(pk).update(...) on the same row — this is safe; PostgreSQL allows a transaction to UPDATE a row it already holds SELECT FOR UPDATE on.

---

TODO(#1848) comment updates — signals.py and descriptions.py

Both conversions from "left as a follow-up" to TODO(#1848) are an improvement: the issue is now searchable and won't silently rot. No concerns.

---

Test class typo — test_caml_review_tools.py

AapplyCaml... → ApplyCaml...: correct, and the PR body confirms this is the only reference in the repo.

---

Test coverage

No new tests for the lock ordering. This is understandable — deterministic concurrent tests for row locking require two threads/connections coordinated through barriers, which is non-trivial in Django's test runner. The existing suite validates the functional behaviour of _apply_caml_article_edit; the lock is a correctness-under-concurrency guarantee rather than a functional change.

If a concurrent test is added later (e.g. as part of #1848), TransactionTestCase (already used by the sibling test class in this file) would be the right base since it doesn't wrap each test in a transaction.

---

Summary

• Lock ordering fix: correct, well-documented, no new deadlock risk. Minor comment wording nit (non-blocking).
• TODO comments: good hygiene, improves traceability.
• Typo fix: straightforward.
• Remaining nit on noop_reverse (noted in PR body as intentionally deferred): reasonable not to churn an already-applied data migration for style.

LGTM with the comment-wording nit flagged for awareness.

[claude]

Code Review

Summary

Small, focused follow-up to PR #1848 — three code changes (Corpus lock ordering, two TODO annotations) and one test typo fix. The remaining three review items were assessed as no-change-needed with clear justification.

---

What Looks Good

Lock ordering fix (caml_article.py:533) — Correct and well-reasoned. The prior code locked Document via select_for_update inside transaction.atomic(), then fetched Corpus unlocked. Adding Corpus.objects.select_for_update().get(pk=corpus_id) before import_document establishes a consistent lock order (Document → Corpus → DocumentPath) that eliminates the race window. Lock ordering is safe against deadlocks: description_cache.py uses a non-locking bulk update(), memory.py never holds both locks simultaneously, and the inner transaction.atomic() inside import_document becomes a savepoint on DocumentPath only — no crossing paths.

TODO(#1848) references in descriptions.py:59 and signals.py:333 — replacing vague prose with issue-anchored TODOs is the right pattern; they'll be findable by grep and in the tracker.

Test class rename (AapplyCamlArticleEditCreatesVersionTreeSiblingTest → ApplyCamlArticleEditCreatesVersionTreeSiblingTest) — simple fix, no issues.

Migration 0018 — inlined calculate_description_filepath is correct and ensures the migration is importable on fresh installs where the model-level helper no longer exists.

No-change justifications — the reasoning on Items 2 (N+1 storage read), 4 (migration dependency graph), and 6 (historical migration) is accurate against the actual code; correctly left untouched.

---

Issues

Blocker: claude.ai session URL in PR description and commit body (CLAUDE.md violation)

The PR description ends with https://claude.ai/code/session_01Wa4U1t8WG2uAff2JY9d7z1, and the same URL appears in commit c17b59d's body. CLAUDE.md is explicit: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." This isn't a Co-Authored-By line, but it is direct session attribution. Please remove the URL from the PR description before merge; and either amend / reword the commit body or note it in the merge commit message.

Should Fix: No CHANGELOG entry for the concurrency fix

CLAUDE.md: "Always update CHANGELOG.md when making significant changes to the codebase." The Corpus row lock fix is a concurrency-correctness change to production code (classified "Medium" in the original review). Suggested entry:

### Fixed
- **CAML article edit race condition** (`opencontractserver/llms/tools/core_tools/caml_article.py:533`):
  `_apply_caml_article_edit` now locks the `Corpus` row with `select_for_update()` inside the
  existing `transaction.atomic()` block (lock order: `Document → Corpus → DocumentPath`) to
  prevent a concurrent corpus delete or `readme_caml_document_id` cache refresh from racing
  between the `Document` lock and `import_document` call. Closes #1848 item 5.

Non-blocking: Unhandled Corpus.DoesNotExist inside transaction.atomic() (caml_article.py:533)

corpus = Corpus.objects.select_for_update().get(pk=corpus_id)

If the corpus is hard-deleted between _load_caml_document_for_user (which validates corpus visibility before the transaction begins) and this select_for_update().get() call, Django raises a bare Corpus.DoesNotExist that propagates out of the atomic block. The transaction rolls back correctly, and the PydanticAIToolWrapper at pydantic_ai_tools.py:612 catches all Exception and converts it to a descriptive error string — so it won't crash the agent loop. However, it's inconsistent with the rest of this function, which wraps all DoesNotExist in ValueError with user-facing messages. Suggested fix:

try:
    corpus = Corpus.objects.select_for_update().get(pk=corpus_id)
except Corpus.DoesNotExist:
    raise ValueError(
        f"Corpus id={corpus_id} was deleted concurrently; cannot apply edit."
    )

Low severity given the tool wrapper safety net, but worth fixing for consistency.

---

Pre-existing nit (not introduced here, no action needed)

opencontractserver/corpuses/migrations/0054_canonical_caml_backfill.py:271,284 defines def noop_reverse(apps, schema_editor): pass rather than using migrations.RunPython.noop. Behavior-identical but diverges from Django convention. Pre-existing; outside scope of this PR.

---

Verdict

The core change is correct. Two items should be addressed before merge: remove the session attribution URL and add a CHANGELOG entry. The DoesNotExist handling is a non-blocking improvement.