Review fixes for #2391: cross-corpus historical reads, unactionable review state, CI

[JSv4]

Summary

Addresses the review findings and the three CI failures on #2391, and merges current main into that branch. Targets implementation/2389-annotation-versioning so the fixes land inside #2391 rather than beside it.

All three CI failures share one root cause: the two new test modules imported to_global_id from graphql_relay, a package the strawberry migration removed. That errored test_reference_versioning collection, failed test_annotation_version_review::test_graphql_exposes_review_state_and_refreshes_the_stale_count and tripped the test_graphql_dependencies guard that exists to catch exactly this.

Changes

Security — historical reads skipped the corpus-linkage check (opencontractserver/annotations/services/annotation_service.py)

check_current_version=root.is_current (config/graphql/document_types.py:309) skipped get_document_annotations' DocumentPath check entirely for a superseded version. That check is not about currency alone: _compute_effective_permissions enforces only MIN(document, corpus), and structural annotations are shared across corpuses through structural_set and are explicitly kept by the corpus filter. READ on a historical document plus READ on any corpus therefore exposed that document's parsed structure under the foreign corpus. The linkage requirement is now unconditional; check_current_version decides only which path counts — the live one, or any surviving one for historical reads. Deleted-document recovery reads (check_current_version=False) still work, because a soft delete adds a new path node and leaves the prior one is_deleted=False.

Correctness — review state that no mutation would accept (opencontractserver/annotations/services/version_review.py)

state_for_annotation resolved the "next version" with no currency filter while _lock_review required a current, immediately-following target. After a second re-import, a v1 annotation rendered STALE forever with no reachable way to review or dismiss it. A single _successor() definition now backs the versionState badge, stale_count and both mutations, so the three can't disagree. Recorded decisions are history and are still reported after later versions land; only the unactionable STALE call-to-action goes away.

Robustness — staleAnnotationCount nulled its own document (config/graphql/document_types.py)

The field is Int!, so the PermissionDenied from _scope for an unreachable corpus nulled the entire document object rather than the count. It returns 0.

Performance — placement source parsed up to 3× under the review locks (version_review.py)

carry_forward called propose() (full PAWLS parse + PlasmaPDF layer) and then _manual_placement() loaded the same source again. One _load_placement_source now serves both, halving the work done while SELECT … FOR UPDATE is held on the annotation and target document.

Frontend

• DocumentReferencesPanel.tsx — the cited-version link went through a router Link. Annotation.link_url may be an absolute external URL (LAW citations), so clicking one navigated the tab away and tore down the SPA. It now goes through openSafeUrl, like every other link in the panel.
• navigationUtils.ts — getDocumentVersionUrl rewrote the last path segment unconditionally. The knowledge-base viewer is a route-independent overlay, so picking a version from a corpus route replaced the corpus slug. It now rewrites only on a parseRoute(...).type === "document" path.
• annotationVersionReview.ts — dropped "GetDocumentAnnotationsOnly" from REVIEW_REFETCH_QUERIES; its only observer is skip: true, so Apollo parks it in standby and the refetch never fired (it only logged a warning per decision).
• Removed the unused AnnotationVersionReviewService.pending().

Docs — docs/architecture/querying_annotations_on_versioned_docs.md (P3: linkage is separate from currency) and docs/features/document_versioning.md (review is a single hop). Changelog fragment changelog.d/2391-versioned-annotation-review-review.fixed.md.

Reviewed and not changed

• A failed review placement should be released. I made this change and reverted it in bd006c3 — AnnotationVersionReview.ct.tsx:55 ("a rejected placement remains pending and never appears as a saved annotation") pins the opposite, deliberately. A rejected placement is not silent: the role="status" banner stays up, an error toast names the failure, no phantom local annotation is created, and "Cancel placement" is the explicit way out. Staying armed is what lets a reviewer retry the same stale annotation after a transient failure instead of having their next selection become an unrelated new annotation. AnnotationHooks.tsx and its unit test are byte-identical to the base branch.
• Historical relationship endpoints filter out analyzer annotations. Real, but not a regression: on main the default prefetch runs through _get_queryset_AnnotationType → visible_to_user, which hides all annotations on a superseded version. The new Prefetch restores the human ones; analyzer endpoints were already invisible there.
• pin_existing_mention_links is nondeterministic for multi-reference annotations. Mirrors existing behaviour; no action.
• Could state_for_annotation see two decisions for one annotation? No — 783a5b8 records why at the call site. Document.parent is assigned only by the version-up in documents/versioning.py:540, which supersedes the current version, while corpus add/fork roots a new content tree with parent=None. With the row already unique per (annotation, target_document), at most one decision exists and the ordering is defensive.

Test plan

Run against a local PostgreSQL 16 + pgvector and Python 3.12.

• pytest opencontractserver/tests/permissioning/ opencontractserver/tests/architecture/ test_annotation_version_review.py test_reference_versioning.py test_corpus_fork_round_trip.py test_structural_annotations_graphql_backwards_compat.py test_analysis_annotation_import.py test_get_document_knowledge_optimizations.py test_query_optimizer_structural_sets.py test_annotation_privacy.py test_corpus_annotations_query.py test_doc_annotations_prefetch_n_plus_one.py test_schema_parity.py -n 4 --dist loadscope — all pass, including the two modules and the dependency guard that were red in CI. Re-run on the current head with --create-db.
• vitest run navigationUtils.test.ts AnnotationHooks.test.tsx — 157 pass.
• playwright test -c playwright-ct.config.ts tests/AnnotationVersionReview.ct.tsx tests/DocumentReferencesPanel.ct.tsx — 11 pass.
• tsc --noEmit -p frontend/tsconfig.json — clean.
• pre-commit run --files <changed> (black, isort, flake8, mypy, changelog validation) — pass; prettier clean on the changed frontend files.

Every new test was confirmed to fail with its fix reverted and pass with it applied:

Test	Guards
test_historical_reads_still_require_a_path_into_the_corpus	structural annotations of a superseded document stay invisible under an unrelated corpus, and its own corpus still reads them
test_a_further_version_retires_state_no_target_would_accept	badge and count go quiet exactly when both possible targets start rejecting the mutation
test_a_corpus_the_document_does_not_belong_to_counts_zero_stale	the non-null count returns 0 instead of nulling the document
getDocumentVersionUrl() × 3	slug swap, ?v removal with other params kept, corpus route untouched
routes an external cited version through the app's URL safety gate	window.open receives the external URL, panel stays mounted

Note on this PR's own checks: Backend CI is scoped to pull_request: branches: [master, main, v*], so the pytest job — the one that was red on #2391 — does not run here, and Codecov has no backend upload to score the diff against. Frontend CI has no branch filter and does run. The backend suite gets a real run on #2391 once this merges into it.

Checklist

• Tests pass locally for any code this PR touches
• pre-commit passes on the changed files (black, isort, flake8, mypy, prettier)
• TypeScript compiles cleanly
• A changelog fragment was added under changelog.d/
• No new dependencies

Contributor License Agreement

By submitting this pull request, you agree to license your contribution
under the project's Contributor License Agreement. First-time
contributors: a bot will comment on this PR asking you to confirm by
replying with a short sign phrase — no separate signup required.

---

Generated with Claude Code

https://claude.ai/code/session_01Mfn98w6aBkJFqMUSK1e8Vi

[JSv4]

Note on what this PR's own checks can and cannot prove: Backend CI is scoped to pull_request: branches: [master, main, v*], and this PR targets implementation/2389-annotation-versioning, so the pytest job — the one that was red on #2391 — does not run here. Frontend CI has no branch filter, so Lint / Unit / Component / Build do run and cover the frontend changes.

The backend fix is verified locally instead (PostgreSQL 16 + pgvector, Python 3.12): the two modules that were erroring and the test_graphql_dependencies guard all pass, alongside opencontractserver/tests/permissioning/, opencontractserver/tests/architecture/, test_schema_parity and every module touching the changed annotation-read surfaces. pytest re-runs for real on #2391 once this merges into it.

---

Generated by Claude Code

[claude]

Review

I focused on the PR's own commit (e6470c6, "Review follow-ups...") since the rest of the diff is main merging in via dependabot bumps and the already-separately-reviewed self-service automation-token PR (#2390).

Backend

annotation_service.py (cross-corpus linkage fix) — correct and well-targeted. Making the DocumentPath linkage check unconditional (only the is_current filter toggles on check_current_version) closes the hole cleanly: MIN(document, corpus) really does require a DocumentPath, and structural annotations shared via structural_set are exactly the leak vector described. The new test (test_historical_reads_still_require_a_path_into_the_corpus) exercises both the negative (unrelated corpus → 0) and positive (own corpus → 1) cases.

version_review.py (_successor() consolidation) — good simplification; having the badge, stale count, and both mutations all resolve "the one reviewable target" through a single method removes a real class of drift bugs. I traced through whether a single annotation_id could ever pick up more than one AnnotationVersionDecision row (which would make the state_for_annotation dict-merge's order_by("target_document_id") load-bearing rather than defensive) — it can't: _lock_review re-derives the successor from the original annotation.document_id each time, and once an annotation is decided, later reviews create a new Annotation row on the successor document rather than re-deciding the same row. So at most one decision ever exists per annotation_id within a given (document_id, corpus_id) scope, and the order_by is harmless rather than fragile. Worth double-checking that invariant holds if this method is ever reused for a "resolve multiple pending decisions" feature later, since the ordering assumption (higher target_document_id = more recent) would then start doing real work.

document_types.py — catching PermissionDenied and returning 0 for the non-null staleAnnotationCount field is the right fix for the field-nulling problem; consistent with how other non-null fields in this codebase seem to be defended.

carry_forward single-load optimization — straightforward, reduces PAWLS/PlasmaPDF parsing from up to 3x to 1x while SELECT ... FOR UPDATE is held. No concerns.

Frontend

• DocumentReferencesPanel.tsx: swapping the router Link for openSafeUrl on the cited-version link is consistent with how the rest of the panel already handles external link_urls (e.g. group.linkUrl a few lines below at line 526). Good catch on the tab-navigation-tears-down-SPA bug.
• AnnotationHooks.tsx: releasing pendingReview on a failed carryForwardAnnotation (instead of only on success) is correct — the old code would leave pendingReview armed indefinitely after any mutation error, silently discarding every subsequent manual annotation on that document. The new AnnotationHooks.test.tsx case (mocking a carryForward error and asserting pending is null + no phantom local annotation) directly covers this.
• navigationUtils.ts::getDocumentVersionUrl: gating the slug rewrite on parseRoute(location.pathname).type === "document" is correct for the "viewer as overlay on a corpus route" case, and the three new unit tests cover the pin/restore/leave-alone cases well.
• annotationVersionReview.ts: dropping the skip: true-only refetch query is a reasonable dead-code removal; the comment explaining why (Apollo parks it, so a name-based refetch just logs a warning) is helpful.

Nits (non-blocking)

• version_review.py::state_for_annotation: the .order_by("target_document_id") on the decisions query has no comment explaining why it's there, given the analysis above that it's currently a no-op for correctness. A one-line comment ("at most one decision per annotation; ordering is defensive, not load-bearing") would save the next reader the trace I just did.
• The NavMenu.tsx "Automation Credentials" link and the automation_credentials.py/automation_credential_api.py changes are all pulled in from main via the merge commit (PR Allow self-service automation tokens without cross-user issuance #2390), not authored here — flagging only so it's clear they aren't part of this PR's own review surface; nothing further to add there.

Test coverage

Coverage looks solid and matches the "fails without the fix, passes with it" discipline described in the PR body — I spot-checked several of these tests by reading rather than executing (backend suite wasn't run in this environment), and the logic each one exercises matches the corresponding code change. No gaps I'd ask to be filled in before merge.

Nice work — this is a tightly scoped fix-up PR with good regression coverage for a genuine cross-corpus permission leak.

[JSv4]

Thanks — both nits read correctly. Your trace of the order_by("target_document_id") matches mine (_lock_review always re-derives the successor from the original annotation.document_id, and a decided annotation's next hop creates a fresh Annotation row rather than re-deciding the old one, so at most one decision exists per annotation_id in scope); it stays as is for now and the explanatory one-liner rides this PR's next code push if one comes. The NavMenu.tsx / automation_credentials.py files are indeed from the main merge (#2390), not this PR's surface.

---

Generated by Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 73.91304% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...tractserver/annotations/services/version_review.py	18.75%	13 Missing ⚠️
...tserver/annotations/services/annotation_service.py	20.00%	4 Missing ⚠️
...omponents/admin/AutomationCredentialManagement.tsx	72.72%	2 Missing and 1 partial ⚠️
...nowledge_base/document/DocumentReferencesPanel.tsx	66.66%	1 Missing and 1 partial ⚠️
...actserver/users/services/automation_credentials.py	95.83%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[JSv4]

Component Tests was red on e6470c6, and it caught a real mistake of mine. Pushed bd006c3 to revert it.

The failing test was AnnotationVersionReview.ct.tsx:55 — "a rejected placement remains pending and never appears as a saved annotation". That test states the intended contract in its own name: when carryForwardAnnotation is rejected, the placement stays armed. My commit released it, so the role="status" banner disappeared and the assertion timed out.

Re-reading it, the original behaviour is correct and the review finding I acted on was wrong on its premise. A failed placement is not silent:

• the banner stays visible — "Select the corrected passage on this document to save the review";
• an error toast names the failure;
• no phantom local annotation is created (reviewing already suppresses the local fallback);
• "Cancel placement" is right there as the explicit way out.

Staying armed is what lets a reviewer retry the same stale annotation after a transient failure, instead of having their next selection silently become an unrelated new annotation on the document. That's the better design, and it was deliberate.

My process error: I changed behaviour a test in this very feature pinned, without opening that test file first. I'd only run the CT file I had touched (DocumentReferencesPanel.ct.tsx) locally rather than the feature's other CT file. Reverted AnnotationHooks.tsx and its unit test to exactly the PR head, and dropped the corresponding changelog bullet.

Every other fix in this PR stands — they're backed by tests I confirmed fail without them. Re-verified locally on the new head: AnnotationVersionReview.ct.tsx + DocumentReferencesPanel.ct.tsx (11 passed), vitest (157 passed), tsc --noEmit clean.

---

Generated by Claude Code

[JSv4]

codecov/patch (75.00% of diff, target 84.22%) and codecov/patch/Backend (73.61%, target 89.62%) went red on e6470c6. These are not this PR's to fix, and no push can clear them on this base branch — flagging once so a repeat on bd006c3 doesn't need a second comment.

The backend coverage flag is uploaded by exactly one step: Upload Coverage Reports to Codecov in the pytest job of backend.yml (line 282). That workflow is scoped to pull_request: branches: [master, main, v*], and this PR targets implementation/2389-annotation-versioning, so the job never runs and no backend coverage report is uploaded for this PR at all. Codecov therefore scores every changed backend line as a miss — which is exactly what it reports: version_review.py at 18.75% and annotation_service.py at 20%.

Those lines are in fact covered. _successor(), the stale_count gate and the corpus-linkage branch are all exercised by test_a_further_version_retires_state_no_target_would_accept, test_a_corpus_the_document_does_not_belong_to_counts_zero_stale and test_historical_reads_still_require_a_path_into_the_corpus, each verified to fail with its fix reverted.

Two further signs the diff Codecov is scoring isn't this PR's: AutomationCredentialManagement.tsx and users/services/automation_credentials.py appear in its missing-lines table, and both come from the main merge (#2390), not from anything authored here.

Not re-running these checks: a re-run recomputes the same threshold from the same absent upload. Patch coverage gets a real measurement on #2391, where the full backend suite runs and uploads.

---

Generated by Claude Code

[claude]

Review

Reviewed the actual content of this PR (the two claude/pr-2391-review-* commits -- the annotation-versioning fixes, plus the automation-credential and dependabot changes that arrived via merging main in). Diffed against origin/implementation/2389-annotation-versioning to isolate what's new.

Security fix -- annotation_service.py::get_document_annotations

The change from if check_current_version and corpus_id: to always requiring a non-deleted DocumentPath when corpus_id is given (with is_current only applied when check_current_version=True) is correct and closes the IDOR the PR describes. I traced both callers that pass check_current_version=False (get_annotations_for_path, relationship_service.py) and confirmed the "recovery read" case still works: delete_document() in versioning.py only flips is_current on the prior path row and creates a new is_deleted=True row, so the prior row stays is_deleted=False and satisfies the new linkage check. Good catch, well-targeted fix, and the new test (test_historical_reads_still_require_a_path_into_the_corpus) exercises exactly the exposure path (shared structural_set under an unrelated corpus).

_successor() consolidation -- version_review.py

Unifying the badge/stale-count/mutation logic behind one _successor() definition is a solid fix for the "STALE with no reachable target" bug, and test_a_further_version_retires_state_no_target_would_accept demonstrates it well.

One thing worth a second look: state_for_annotation (version_review.py:220-238) dropped the target_document=child filter on the decisions query and replaced it with .order_by("target_document_id") before folding results into a dict keyed by annotation_id (so the last-processed row -- i.e. highest target_document_id -- wins on collision). In the common case (one decision per annotation, since _lock_review rejects a second decision against the same (annotation, target_document) pair) this is equivalent to the old behavior. But it now implicitly assumes target_document_id ordering tracks chronological/version order to pick "the" decision when more than one exists for the same annotation -- worth confirming that's actually unreachable (e.g. via corpus forking creating multiple children with the same parent_id) rather than just unlikely, since nothing here would make a wrong pick loud (it would silently show the wrong decision, not error).

staleAnnotationCount -> 0 on PermissionDenied

Correct fix for the non-null field being nulled. Good test coverage (test_a_corpus_the_document_does_not_belong_to_counts_zero_stale).

Placement-source load consolidation (perf)

_load_placement_source now runs once and is threaded through propose/_manual_placement via the loaded= kwarg. Straightforward and reduces lock hold time as described. No issues.

Frontend

• DocumentReferencesPanel.tsx: routing the cited-version link through openSafeUrl instead of router Link is correct -- an absolute external URL passed to <Link to=...> was indeed broken, and the new code matches the existing pattern already used for outbound reference rows in the same file. New CT test (routes an external cited version...) verifies both the window.open call and that the SPA stays mounted.
• navigationUtils.ts::getDocumentVersionUrl: gating the slug rewrite on parseRoute(...).type === "document" looks right for the described overlay-vs-route bug, and the three new unit tests cover the meaningful cases (pin, restore, corpus-route no-op).
• annotationVersionReview.ts: dropping the dead GetDocumentAnnotationsOnly refetch name is a reasonable cleanup given its only consumer is skip: true.
• The AnnotationHooks.tsx revert (final commit) is well-justified in the commit message -- the CT test's contract (a rejected placement remains pending...) makes staying armed on failure the correct behavior, not the earlier "release on failure" change.

Scope note

The diff also carries the automation-credential self-service changes (config/graphql/automation_credential_api.py, opencontractserver/users/services/automation_credentials.py, AutomationCredentialManagement.tsx, etc.) and the dependabot bumps, but these came from main via the merge commit (already-merged PR #2390 and dependabot PRs), not new work in this PR -- I didn't re-review them here.

Overall

The fixes are well-scoped, each backed by a regression test that fails on revert (per the PR description), and the docs/changelog fragment accurately describe the change. No blocking issues found; the target_document_id-ordering point above is a minor robustness note, not a correctness bug I could construct a concrete failing scenario for.

[JSv4]

On the state_for_annotation decisions-query question — I traced reachability rather than leaving it at "unlikely, and a wrong pick would be silent." Two decisions for one annotation would need two target_documents that each passed _lock_review, i.e. two Documents sharing a parent_id in the same corpus. That can't happen:

• Document.parent is assigned in exactly one place — documents/versioning.py:540 (parent=old_doc), on the version-up path. That path supersedes the current version at a corpus path, so v1 gets superseded once and the next update supersedes v2, never v1 again.
• Corpus add and fork explicitly root a new content tree: corpuses/models.py:986 sets parent=None with a fresh version_tree_id. So forking produces no siblings — it was the right thing to check, and it's the case that makes the invariant hold rather than breaks it.
• restore_document creates a DocumentPath, not a Document.

Combined with the row being unique per (annotation, target_document), at most one decision exists per annotation, so nothing collides in the dict and the ordering never does real work.

Since you and the previous review both had to derive that independently, the code was the problem, not the logic — pushed 783a5b8 recording the invariant and its two load-bearing reasons at that call site. No behaviour change. This is the "next code push" the earlier nit was waiting on, so that one is now carried too.

Re-verified on the new head with a fresh database: test_annotation_version_review.py, permissioning/, architecture/ and test_reference_versioning.py all pass.

---

Generated by Claude Code