Address deep-research review follow-ups (#1864)

CHANGELOG.md

@@ -9,6 +9,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Deep-research code-review follow-ups (#1864).** Addressed the actionable
+  items from the PR #1836 review:
+  - `frontend/src/graphql/mutations.ts` — `CancelResearchReportOutput.obj.status`
+    was typed as the bare `string`; narrowed to `JobStatus | string` to match
+    `ResearchReportType.status` so callers comparing against `JobStatus` values
+    get type checking on this payload.
+  - `frontend/tests/ResearchReportDetailTestWrapper.tsx` — the single detail
+    mock now carries `maxUsageCount: Number.POSITIVE_INFINITY` (mirroring
+    `CorpusResearchReportCardsTestWrapper`). The detail view uses
+    `notifyOnNetworkStatusChange` and can refetch/poll, so a fixed-bucket mock
+    risked a "No more mocked responses" error if a future test exercised a
+    non-terminal state.
+  - `opencontractserver/research/models.py` — documented that
+    `ResearchReport.duration_seconds` is always a computed `@property` (never a
+    stored field), since the status-tool duration tests set `started_at` /
+    `completed_at` directly and depend on that invariant. Added a guard test
+    (`test_research_report_model.py::test_duration_seconds_computed_from_timestamps`).
+  - Reviewer items that needed no change, after verification: async catch blocks
+    in `ResearchReportDetail.tsx` / `StartResearchModal.tsx` already log via
+    `console.error`; the `slug` column is already indexed (`unique=True` +
+    `db_index=True`); and the snake_case reactive-var locals in
+    `CorpusResearchReportCards.tsx` (`opened_corpus`, `research_search_term`,
+    `auth_token`) intentionally match the established codebase convention used by
+    the sibling `CorpusExtractCards`/`CorpusAnalysesCards` and avoid shadowing the
+    imported camelCase reactive vars — renaming them would have been a regression.
+
 - **WebSocket 1011 reconnect churn on a 5-second beat: channels-redis 4.3.0 vs.
   redis-py 8.0 default `socket_timeout=5` (#1886).** Every consumer's idle
   channel-layer receive loop crashed ~every 5s, so Daphne closed the socket with
@@ -104,7 +130,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `LABEL_TEXT_TO_GEOCODE_LABEL_TYPE` to a `Literal` value type; and documented
   the no-superuser migration-skip recovery path in
   `docs/agents/location_tagger.md`.
-
 - **Corpus chat duplicate-response loop on reconnect**
   (`frontend/src/components/corpuses/CorpusChat.tsx`). Submitting a query from
   the corpus home search bar navigated into the chat view and auto-sent the

frontend/src/graphql/mutations.ts

@@ -16,6 +16,7 @@ import {
   UserExportType,
   CorpusActionType,
   ResearchReportType,
+  JobStatus,
 } from "../types/graphql-api";
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -3703,7 +3704,9 @@ export interface CancelResearchReportOutput {
     message: string;
     obj: {
       id: string;
-      status: string;
+      // Mirror ResearchReportType.status (JobStatus | string) so callers
+      // comparing against JobStatus values get type checking on this payload.
+      status: JobStatus | string;
       cancelRequested: boolean;
     } | null;
   };

frontend/tests/ResearchReportDetailTestWrapper.tsx

@@ -113,6 +113,13 @@ export const ResearchReportDetailTestWrapper: React.FC<{
   const mocks: MockedResponse[] = [
     {
       request: { query: GET_RESEARCH_REPORT, variables: { id: report.id } },
+      // maxUsageCount=Infinity lets this single mock serve every fire of the
+      // query — the detail view uses notifyOnNetworkStatusChange and may
+      // refetch (terminal-notification path) or poll (non-terminal states).
+      // Without it a second trip drains the bucket and resolves to a "No more
+      // mocked responses" error, which the view's not-found state would surface.
+      // Mirrors CorpusResearchReportCardsTestWrapper.
+      maxUsageCount: Number.POSITIVE_INFINITY,
       result: { data: { researchReport: report } },
     },
   ];

opencontractserver/research/models.py

@@ -211,6 +211,12 @@ def save(self, *args, **kwargs):
     # ------------------------------------------------------------------
     @property
     def duration_seconds(self) -> float | None:
+        """Wall-clock run time, always derived from the timestamps — never stored.
+
+        If this is ever converted to a stored field, any caller that sets
+        ``started_at`` / ``completed_at`` directly (bypassing ``finalize()``)
+        will need to set the stored field explicitly instead.
+        """
         if self.started_at and self.completed_at:
             return (self.completed_at - self.started_at).total_seconds()
         return None

opencontractserver/tests/research/test_research_report_model.py

@@ -2,8 +2,11 @@
 
 from __future__ import annotations
 
+from datetime import timedelta
+
 from django.contrib.auth import get_user_model
 from django.test import TestCase
+from django.utils import timezone
 
 from opencontractserver.corpuses.models import Corpus
 from opencontractserver.research.models import ResearchReport
@@ -69,6 +72,32 @@ def test_is_terminal(self):
         report.save(update_fields=["status"])
         self.assertFalse(report.is_terminal)
 
+    def test_duration_seconds_computed_from_timestamps(self):
+        """duration_seconds is derived from started_at/completed_at, not stored.
+
+        Guards the invariant relied on by the status-tool duration tests, which
+        set the timestamps directly (bypassing finalize()) and expect the
+        property to compute the elapsed wall-clock time.
+        """
+        report = ResearchReport.objects.create(
+            creator=self.user,
+            corpus=self.corpus,
+            prompt="x",
+        )
+        # No timestamps yet → None (not 0).
+        self.assertIsNone(report.duration_seconds)
+
+        # Set timestamps directly, bypassing finalize(): the property must still
+        # compute correctly, proving it reads from the fields at access time.
+        now = timezone.now()
+        report.started_at = now - timedelta(seconds=125)
+        report.completed_at = now
+        report.save(update_fields=["started_at", "completed_at"])
+        duration = report.duration_seconds
+        self.assertIsNotNone(duration)
+        assert duration is not None  # narrow Optional for type-checker
+        self.assertAlmostEqual(duration, 125.0)
+
     def test_visible_to_user_creator_only(self):
         report = ResearchReport.objects.create(
             creator=self.user,