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

[JSv4]

Summary

Resolves the actionable items from the PR #1836 code review tracked in #1864. Each reported issue was evaluated against the current main (PR #1836 is now merged) before acting — two were already correct/fixed and one was based on an incorrect premise, so only the genuinely valid items were changed.

Closes #1864

Assessment of the 6 reported issues

#	Issue	Verdict	Action
1	Detail test wrapper mock drains on retries	Valid (latent fragility)	Added maxUsageCount: Number.POSITIVE_INFINITY
2	duration_seconds computed-property invariant undocumented	Valid	Added docstring + guard test
3	Async catch blocks discard the exception	Already fixed	No change — both already console.error(...)
4	snake_case locals in CorpusResearchReportCards	Invalid	No change — see below
5	CancelResearchReportOutput.obj.status typed string	Valid (minor)	Narrowed to JobStatus | string
6	Slug index	Already correct	No change — unique=True + db_index=True

Changes

• frontend/src/graphql/mutations.ts — CancelResearchReportOutput.obj.status was 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-query mock now carries maxUsageCount: Number.POSITIVE_INFINITY (mirroring CorpusResearchReportCardsTestWrapper). The detail view uses notifyOnNetworkStatusChange and can refetch (terminal-notification path) or poll (non-terminal states), so a fixed-bucket mock risked a "No more mocked responses" error if a future test exercised a non-terminal state. (All current tests use terminal states, so this is hardening, not a live bug fix.)
• 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.
• opencontractserver/tests/research/test_research_report_model.py — added test_duration_seconds_computed_from_timestamps pinning the invariant.

Why #4 was rejected (snake_case locals)

The review claimed the codebase uses camelCase for these reactive-var locals. The opposite is true: the codebase uses snake_case for reactive-var shadow locals (≈113 vs 75), and the two direct sibling components — CorpusExtractCards.tsx and CorpusAnalysesCards.tsx — use the identical opened_corpus / auth_token pattern. The snake_case naming also deliberately avoids shadowing the imported camelCase reactive vars (openedCorpus, etc.); renaming the locals to camelCase would collide with the imports (const openedCorpus = useReactiveVar(openedCorpus) is a use-before-declaration error). Renaming would have been a regression, so it was left as-is.

Verification

• tsc --noEmit — clean (exit 0).
• yarn test:ct ResearchReportDetail.ct.tsx --reporter=list — 3/3 pass with the maxUsageCount change.
• prettier --check — clean on the changed frontend files; husky lint-staged ran on commit.
• black / flake8 — clean on the changed backend files. The new test mirrors a pattern already used by the type-checked (non-baselined) test_corpus_action_execution.py, so it is mypy-clean. (Full backend test suite was not run — Docker is unavailable in this environment — but the new test follows the same create→set-timestamps→save→assert pattern as the existing, passing test_research_status_tool.py duration tests.)

[claude]

Code Review

Overall: Clean, well-scoped follow-up PR. The changes are minimal and well-justified. The PR description's table (documenting which items were changed and which were intentionally left alone, and why) is a good model.

Issue 1 — maxUsageCount in ResearchReportDetailTestWrapper

Correct fix. The detail view uses notifyOnNetworkStatusChange and can trigger refetches, so a fixed-bucket mock is fragile. This mirrors the pattern in CorpusResearchReportCardsTestWrapper. The inline comment explaining the reasoning is appropriate given the non-obvious test infrastructure behavior.

Minor nit: Infinity is more idiomatic in modern TypeScript than Number.POSITIVE_INFINITY (they are identical values). Not worth a standalone change, but worth noting for future test additions.

Issue 2 — duration_seconds docstring + guard test

New test is solid. The three-step structure (create, assert None, set timestamps directly + assert computed value) cleanly validates the invariant. assertAlmostEqual without an explicit delta uses the default places=7, which is fine for a 125-second interval.

The docstring has a CLAUDE.md violation worth fixing. From CLAUDE.md:

"Don't reference the current task, fix, or callers ('used by X', 'added for the Y flow', 'handles the case from issue #123'), since those belong in the PR description and rot as the codebase evolves."

The docstring currently names two specific test files (test_completed_report_shows_duration in test_research_status_tool.py and test_research_report_model.py). Those references will rot as tests are renamed or moved. The invariant sentence and the migration warning are the parts worth keeping. Suggested trim:

"""Wall-clock run time, always derived from timestamps — never stored.

If this is ever converted to a stored field, any test that sets
started_at / completed_at directly (bypassing finalize()) will need
to set the stored field explicitly instead.
"""

Issue 5 — CancelResearchReportOutput.obj.status: JobStatus | string

Correct alignment. Brings CancelResearchReportOutput in line with ResearchReportType.status and ResearchReportListItem.status.

Context worth noting (not a change request): JobStatus | string is technically equivalent to plain string from TypeScript's type-system perspective — string is a supertype of the enum so the union collapses to string for assignability purposes. The narrow type still helps callsite readability and IDE autocompletion for enum values, so this is a net positive despite the type erasure. Strictly enforcing JobStatus would require removing the | string fallback from ResearchReportType.status itself — a broader change and out of scope here.

The new inline comment explaining why the type is JobStatus | string rather than just JobStatus is a CLAUDE.md-conformant case: it explains a non-obvious design decision that would confuse a reader. Acceptable.

Issues 3, 4, 6 — Non-changes, correctly assessed

The rejection rationale for issue 4 (snake_case locals) is correct — the sibling components (CorpusExtractCards, CorpusAnalysesCards) use the identical convention and renaming would shadow the imported camelCase reactive vars, causing a use-before-declaration error.

Summary

Change	Verdict
maxUsageCount: Number.POSITIVE_INFINITY	Correct hardening
duration_seconds docstring	Trim test-file references per CLAUDE.md
Guard test	Correct
status: JobStatus or string	Correct
CHANGELOG update	Complete and accurate

One actionable item: trim the specific test-file names from the duration_seconds docstring. Everything else is good to merge.

[claude]

Code Review

Overview

Tight follow-up PR that addresses four of six items from the PR #1836 review (#1864). The PR description is thorough and the reasoning for the two rejected items is sound. Changes are small and well-scoped.

---

Feedback by file

frontend/src/graphql/mutations.ts

Nit (pre-existing pattern, not introduced here): JobStatus | string is structurally equivalent to string in TypeScript because string subsumes every string-enum member. The type annotation is purely documentary — the compiler will not flag a bare "UNKNOWN_STATE" being compared against a JobStatus member. If type-safety is the goal, the correct type is JobStatus alone.

That said, ResearchReportType.status already carries this same union (line 2077 of graphql-api.ts), so the PR is internally consistent. Worth filing a follow-up to narrow both to JobStatus when the GraphQL codegen story is clear, but this is the right call for now given the existing pattern.

frontend/tests/ResearchReportDetailTestWrapper.tsx

Correct fix. maxUsageCount: Number.POSITIVE_INFINITY mirrors the sibling wrapper and is the standard Apollo MockedProvider pattern for components that may refetch. The inline comment explaining why the mock can be re-used (terminal-notification path, polling) is genuinely helpful for future maintainers.

opencontractserver/research/models.py

The docstring is clear and the warning about stored-field conversion is exactly the right content for this invariant. No issues.

opencontractserver/tests/research/test_research_report_model.py

The test is well-structured. One minor style note on the double-assertion pattern:

self.assertIsNotNone(duration)
assert duration is not None  # narrow Optional for type-checker

Two consecutive assertions for the same condition is redundant — if assertIsNotNone passes, the bare assert adds no test value. A common alternative that satisfies both unittest coverage and the type-checker in one line is the bare assert duration is not None alone. Or, keep assertIsNotNone and add # type: ignore[union-attr] on the assertAlmostEqual call. Minor either way — the test is correct and readable.

assertAlmostEqual(duration, 125.0) without places or delta uses the default 7 decimal places. Since the computed value is timedelta(seconds=125).total_seconds() which is exactly 125.0 this is fine, but specifying delta=0.001 would make the acceptable tolerance explicit at a glance.

CHANGELOG.md

Thorough entry with good cross-references. The extra blank line removal is a minor cleanup.

---

Summary

All four changes are correct and appropriately scoped. The reasoning in the PR body for the two rejected items (async catch blocks already log; slug already indexed) is well-documented and convincing. No blockers — the nits above are all optional. Good work on the maxUsageCount hardening and the computed-property invariant test.

[claude]

Code Review: Address deep-research review follow-ups (#1864)

Overall: Clean, well-scoped cleanup PR. The description is exemplary — it documents each issue, its verdict, and the rationale for changes and non-changes. The diff is minimal and purposeful.

---

Strengths

PR description discipline: The table mapping each issue to verdict and action is exactly the kind of traceability that makes review follow-ups easy to audit. The rationale for rejecting item 4 (snake_case locals) is well-argued and backed by codebase evidence (113 vs 75 uses in the wild, sibling components use identical pattern).

Test quality (test_duration_seconds_computed_from_timestamps):

• Covers the None case explicitly (returns None, not 0)
• Sets timestamps directly, bypassing finalize() — the precise invariant being guarded
• assertAlmostEqual is appropriate for deterministic values; default 7dp tolerance is fine
• The inline assert duration is not None for type-narrowing is idiomatic

maxUsageCount: Number.POSITIVE_INFINITY fix: Correct approach. notifyOnNetworkStatusChange + polling means a single refetch can exhaust a fixed-bucket mock silently. Hardening proactively rather than waiting for a flaky test is the right call.

---

Minor Observations

Type narrowing in mutations.ts: JobStatus | string is pragmatically sound and matches ResearchReportType.status. One nuance: in TypeScript, JobStatus | string resolves to string at the type level (since JobStatus is a string subtype), so callers still won't get exhaustiveness checking. If stricter enforcement is ever wanted, the field would need to be JobStatus outright. The union as a document-intent-without-enforcing pattern is fine here; the inline comment makes the intent clear.

CHANGELOG.md entry placement: The new entry lands inside the existing Fixed block rather than a fresh Unreleased header. Consistent with surrounding content; noting it in case the release workflow expects a new section per batch of changes.

Docstring in models.py: Reads clearly. The field-promotion warning is a useful nudge for whoever does it. No issues.

---

Summary

No blocking concerns. The three changes are small, well-motivated, and leave the codebase in a better state — tighter types, a hardened test mock, and a pinned invariant with a guard test. The non-changes are correctly justified. Solid follow-up work.