Merge pull request #1398 from Open-Source-Legal/claude/resolve-issue-1385-BCMkJ

Fix inline-agent default instructions for document triggers (#1385)

Author: JSv4

CHANGELOG.md

@@ -23,6 +23,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`CreateCorpusActionModal` opened with the wrong default agent instructions for document triggers** (Issue #1385, `frontend/src/components/corpuses/CreateCorpusActionModal.tsx:136-144,168-171`): the `inlineAgentInstructions` state was initialised with `DEFAULT_MODERATOR_INSTRUCTIONS` even though the default trigger is `add_document` (a document trigger). The trigger-change handler at line 611 swaps to `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS`, but a user who created an inline agent on the default-selected trigger without first re-selecting the trigger would submit the moderator copy as the new agent's system instructions. Initialised both the `useState` default and `resetForm()` to `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS` so the pre-interaction value matches the default trigger. Updated `frontend/tests/CreateCorpusActionModal.ct.tsx` "inline-agent create: full happy path" mutation mock to expect `DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS` — the previous mock variable masked this bug because `MockedProvider` was matching the stale moderator default rather than the trigger-appropriate one.
+
+### Changed
+
+- **Test/type cleanup follow-ups from the PR #1383 review** (Issue #1385):
+  - Pinned the `isProcessing` contract for SYNC_CONTENT in `frontend/tests/CorpusChat.ct.tsx` "SYNC_CONTENT renders a complete message immediately": added an `expect(input).toBeEnabled()` assertion after the reply renders, locking the documented invariant that `setIsProcessing(true)` is owned solely by `ASYNC_START` and that a SYNC_CONTENT-only reply must never disable the input.
+  - Consolidated the duplicated `::: oc-component` fence dispatcher: extracted `OcComponentBlock` interface and a new `buildOcComponentCustomBlocks(renderMarkdown)` helper into `frontend/src/utils/camlComponents.ts`. Both `frontend/src/hooks/useCamlComponentRenderer.tsx` and `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx` now share the same helper instead of each casting `block` independently.
+  - Replaced `route: any` and `page: any` escape hatches with the proper `Route` and `Page` types from `@playwright/test` in `frontend/tests/CorpusDescriptionEditor.ct.tsx` (`setupMdRoute` and the abort-route test).
+  - Migrated `.version-number` CSS-class locators in `frontend/tests/CorpusDescriptionEditor.ct.tsx` to a semantic `data-testid="version-number"` matcher (`page.getByTestId("version-number")`); added the test id to the rendered version-row in `frontend/src/components/corpuses/CorpusDescriptionEditor.tsx`.
+
 - **`test_superuser_sees_all_queryset` miscounts personal corpuses by 1** (Issue #1394, `opencontractserver/tests/test_visibility_managers.py`, `opencontractserver/tests/test_resolvers.py`): Two `VisibleToUserTests.test_superuser_sees_all_queryset` cases asserted that `Corpus.objects.visible_to_user(superuser).count() == 4` (public + private + 2 personal), but the actual count is 5 because the test DB starts with a pre-existing personal corpus owned by django-guardian's `AnonymousUser` (created during fixture setup before/around the username-based skip in `opencontractserver/users/signals.py::user_created_signal`). The assertion is now scoped to corpuses created by the test's two users (`creator__in=[self.user, self.superuser]`), making it resilient to any fixture-level corpuses that exist at test DB init time. Production code is unchanged.
 - **Merged `frontend` Codecov flag drops to ~33% on every commit where Frontend CI's CT job fails** (`frontend/package.json` `test:coverage:ct`): the script chained `playwright test ... && mkdir -p ... && nyc report ...`, so a failing CT run short-circuited before `nyc report` could turn the per-test JSON files in `.nyc_output` into an `lcov.info`. The downstream `Upload CT Coverage to Codecov` step (`if: success() || failure()`) then errored with "No coverage reports found" and `frontend-component` did not upload for that SHA. Codecov's server-side aggregation of the `frontend` flag was left with only `frontend-unit` (~23%) and `frontend-e2e` (~24%), pulling the merged number down to ~33% even though the previous commit was at ~67% — observed on six consecutive main commits 2026-04-26T01:02..02:58Z (`2d7033f8`..`be5bcfc8`) before recovering on `30298391`. Mirrored the existing `test:e2e:coverage` pattern (`; CT_EXIT=$?; nyc report ... || echo "No coverage data to report"; exit $CT_EXIT`) so `nyc report` runs regardless of test outcome and the lcov ships even on red CT runs. `frontend-component` will still report a slightly lower number when tests fail (failed tests register fewer hits), but it will report — keeping the merged `frontend` flag's denominator stable.
 - **`User.__init__` shared-state mutation re-introduced by branch merge** (`opencontractserver/users/models.py:172-180` removed): PR #1374 (commit `50ed6740`) deleted the `User.__init__` override that mutated `Field.validators[0]` on every instantiation, but a subsequent merge (`b68c1cb4 → 6d2cddbf`) resurrected the override along with its mypy-narrowing changes. The current main on commit `6d2cddbf` therefore reproduced the original `#1358` bug: `User(...)` rebound `username_field.validators[0]` and clobbered any third-party validator prepended to the list. Removed the `__init__` override entirely; the class-body declaration `validators=[UserUnicodeUsernameValidator()]` on the `username` field (still present from PR #1374) is the canonical and only declaration. Also dropped the now-unused `Field` import. Regression coverage from PR #1374 (`opencontractserver/tests/test_user_username_validator.py`) was already on main and is what surfaced the regression in CI.

frontend/src/components/corpuses/CorpusDescriptionEditor.tsx

@@ -1078,7 +1078,10 @@ export const CorpusDescriptionEditor: React.FC<
                               whileTap={{ scale: 0.98 }}
                             >
                               <div className="version-header">
-                                <div className="version-number">
+                                <div
+                                  className="version-number"
+                                  data-testid="version-number"
+                                >
                                   Version {revision.version}
                                   {revision.version === currentVersion && (
                                     <span className="version-badge">

frontend/src/components/corpuses/CreateCorpusActionModal.tsx

@@ -133,8 +133,13 @@ export const CreateCorpusActionModal: React.FC<
   const [inlineAgentName, setInlineAgentName] = React.useState("");
   const [inlineAgentDescription, setInlineAgentDescription] =
     React.useState("");
+  // Initial trigger is "add_document" (a document trigger), so the default
+  // instructions must match — otherwise the textarea opens with the moderator
+  // copy that only applies to thread/message triggers. The dropdown's onChange
+  // swaps this value when the trigger changes; this initialiser keeps the
+  // pre-interaction state aligned with the default trigger.
   const [inlineAgentInstructions, setInlineAgentInstructions] = React.useState(
-    DEFAULT_MODERATOR_INSTRUCTIONS
+    DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS
   );
   const [selectedModerationTools, setSelectedModerationTools] = React.useState<
     string[]
@@ -162,7 +167,9 @@ export const CreateCorpusActionModal: React.FC<
     setUseInlineAgent(true);
     setInlineAgentName("");
     setInlineAgentDescription("");
-    setInlineAgentInstructions(DEFAULT_MODERATOR_INSTRUCTIONS);
+    // Reset to the default-trigger instructions; the trigger itself is reset to
+    // "add_document" above, so the document-agent default is the matching pair.
+    setInlineAgentInstructions(DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS);
     setSelectedModerationTools(DEFAULT_MODERATION_TOOLS.map((t) => t.name));
     setSelectedDocumentTools([]);
     setDisabled(false);

frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx

@@ -26,7 +26,7 @@ import {
   DirectiveHandlerContext,
 } from "./directiveRegistry";
 import {
-  OC_COMPONENT_FENCE,
+  buildOcComponentCustomBlocks,
   resolveComponentMarker,
   type CamlComponentRegistry,
 } from "../../../utils/camlComponents";
@@ -193,14 +193,7 @@ export const CamlDirectiveRenderer: React.FC<CamlDirectiveRendererProps> = ({
   // plain `[component:TYPE ...]` marker, so we delegate to the same
   // renderMarkdown path that handles inline markers.
   const customBlocks = useMemo(
-    () => ({
-      [OC_COMPONENT_FENCE]: (block: unknown) => {
-        const body = (
-          (block as { body?: string } | undefined)?.body ?? ""
-        ).trim();
-        return renderMarkdown(body);
-      },
-    }),
+    () => buildOcComponentCustomBlocks(renderMarkdown),
     [renderMarkdown]
   );
 

frontend/src/hooks/useCamlComponentRenderer.tsx

@@ -22,17 +22,11 @@ import { MarkdownMessageRenderer } from "../components/threads/MarkdownMessageRe
 import { ErrorBoundary } from "../components/widgets/ErrorBoundary";
 import { ComponentEmbedErrorFallback } from "../components/widgets/ComponentEmbedErrorFallback";
 import {
-  OC_COMPONENT_FENCE,
+  buildOcComponentCustomBlocks,
   resolveComponentMarker,
 } from "../utils/camlComponents";
 export type { CamlComponentRegistry } from "../utils/camlComponents";
 
-interface OcComponentBlock {
-  type: string;
-  body?: string;
-  attrs?: Record<string, string>;
-}
-
 export interface CamlComponentRendererBindings {
   /** Pass to `<CamlArticle renderMarkdown={...}>`. */
   renderMarkdown: (md: string) => React.ReactNode;
@@ -76,12 +70,7 @@ export function useCamlComponentRenderer(
   );
 
   const customBlocks = useMemo(
-    () => ({
-      [OC_COMPONENT_FENCE]: (block: unknown) => {
-        const body = ((block as OcComponentBlock)?.body ?? "").trim();
-        return renderMarkdown(body);
-      },
-    }),
+    () => buildOcComponentCustomBlocks(renderMarkdown),
     [renderMarkdown]
   );
 

frontend/src/utils/camlComponents.ts

@@ -137,6 +137,18 @@ export function buildComponentMarker(
  */
 export const OC_COMPONENT_FENCE = "oc-component";
 
+/**
+ * Shape of a parsed `::: oc-component` block produced by @os-legal/caml.
+ * Used by `customBlocks` handlers when the renderer dispatches an
+ * `oc-component` fence. Only `body` is read by the OpenContracts dispatcher;
+ * the other fields are included for completeness.
+ */
+export interface OcComponentBlock {
+  type: string;
+  body?: string;
+  attrs?: Record<string, string>;
+}
+
 /**
  * Wrap a marker in a CAML fence ready for insertion into the editor source.
  *
@@ -180,3 +192,23 @@ export function resolveComponentMarker(
   if (!Component) return null;
   return React.createElement(Component, { key, ...parsed.props });
 }
+
+/**
+ * Build the `customBlocks` object passed to `<CamlArticle>` for the
+ * project-specific `::: oc-component` fence.
+ *
+ * Both `useCamlComponentRenderer` and `CamlDirectiveRenderer` need to forward
+ * the fence body through their own `renderMarkdown` so the inline-marker path
+ * can resolve registered components. Centralising the lookup keeps the
+ * `OcComponentBlock` cast and the body-extraction logic in one place.
+ */
+export function buildOcComponentCustomBlocks(
+  renderMarkdown: (md: string) => React.ReactNode
+): Record<string, (block: unknown) => React.ReactNode> {
+  return {
+    [OC_COMPONENT_FENCE]: (block: unknown) => {
+      const body = ((block as OcComponentBlock | undefined)?.body ?? "").trim();
+      return renderMarkdown(body);
+    },
+  };
+}

frontend/tests/CorpusChat.ct.tsx

@@ -1000,6 +1000,12 @@ test.describe("CorpusChat", () => {
       timeout: 10000,
     });
 
+    // SYNC_CONTENT arrives without a preceding ASYNC_START, so isProcessing
+    // must never flip to true and the input must remain interactive after the
+    // reply lands. Pinning this guards the contract documented in CorpusChat:
+    // ASYNC_START is the only setter for setIsProcessing(true).
+    await expect(input).toBeEnabled({ timeout: 5000 });
+
     await component.unmount();
   });
 

frontend/tests/CorpusDescriptionEditor.ct.tsx

@@ -1,4 +1,5 @@
 import React from "react";
+import type { Page, Route } from "@playwright/test";
 import { test, expect } from "./utils/coverage";
 import { MockedResponse } from "@apollo/client/testing";
 import { CorpusDescriptionEditorTestWrapper } from "./CorpusDescriptionEditorTestWrapper";
@@ -87,8 +88,8 @@ const buildCorpusMockNoMd = (): MockedResponse => ({
   },
 });
 
-const setupMdRoute = async (page: any, body: string = INITIAL_MD) => {
-  await page.route("**/test-md/**", async (route: any) => {
+const setupMdRoute = async (page: Page, body: string = INITIAL_MD) => {
+  await page.route("**/test-md/**", async (route: Route) => {
     await route.fulfill({
       status: 200,
       contentType: "text/markdown",
@@ -237,7 +238,7 @@ test.describe("CorpusDescriptionEditor", () => {
     await expect(page.getByText("Version History")).toBeVisible({
       timeout: 5000,
     });
-    await expect(page.locator(".version-number")).toContainText("Version 1");
+    await expect(page.getByTestId("version-number")).toContainText("Version 1");
 
     // Hide again
     await page
@@ -271,7 +272,7 @@ test.describe("CorpusDescriptionEditor", () => {
       .getByRole("button", { name: /Show History/, exact: false })
       .click();
 
-    const versionNumber = page.locator(".version-number");
+    const versionNumber = page.getByTestId("version-number");
     await expect(versionNumber).toContainText("Version 1", { timeout: 5000 });
 
     // Click the version row to expand details
@@ -425,7 +426,7 @@ test.describe("CorpusDescriptionEditor", () => {
     await page
       .getByRole("button", { name: /Show History/, exact: false })
       .click();
-    const versionNumber = page.locator(".version-number");
+    const versionNumber = page.getByTestId("version-number");
     await expect(versionNumber).toContainText("Version 1", { timeout: 5000 });
     await versionNumber.first().click();
 
@@ -488,7 +489,7 @@ test.describe("CorpusDescriptionEditor", () => {
     await page
       .getByRole("button", { name: /Show History/, exact: false })
       .click();
-    const versionNumber = page.locator(".version-number");
+    const versionNumber = page.getByTestId("version-number");
     await expect(versionNumber).toContainText("Version 1", { timeout: 5000 });
     await versionNumber.first().click();
 
@@ -628,7 +629,7 @@ test.describe("CorpusDescriptionEditor", () => {
       .getByRole("button", { name: /Show History/, exact: false })
       .click();
 
-    const versionNumber = page.locator(".version-number");
+    const versionNumber = page.getByTestId("version-number");
     await expect(versionNumber).toContainText("Version 1", { timeout: 5000 });
     await versionNumber.first().click();
 
@@ -662,7 +663,7 @@ test.describe("CorpusDescriptionEditor", () => {
       .getByRole("button", { name: /Show History/, exact: false })
       .click();
 
-    const versionNumber = page.locator(".version-number");
+    const versionNumber = page.getByTestId("version-number");
     await expect(versionNumber).toContainText("Version 1", { timeout: 5000 });
 
     // Expand
@@ -718,7 +719,7 @@ test.describe("CorpusDescriptionEditor", () => {
       .click();
 
     // Expand the OLDER version (v1) — not the current one
-    const versionRows = page.locator(".version-number");
+    const versionRows = page.getByTestId("version-number");
     const olderRow = versionRows.filter({ hasText: "Version 1" }).first();
     await olderRow.click();
 
@@ -745,7 +746,7 @@ test.describe("CorpusDescriptionEditor", () => {
   }) => {
     // Abort the network request so the fetch promise rejects and the
     // component's .catch() branch (setCurrentContent("")) runs.
-    await page.route("**/test-md/**", async (route: any) => {
+    await page.route("**/test-md/**", async (route: Route) => {
       await route.abort("failed");
     });
 

frontend/tests/CreateCorpusActionModal.ct.tsx

@@ -13,7 +13,7 @@ import {
   CREATE_CORPUS_ACTION,
   UPDATE_CORPUS_ACTION,
 } from "../src/graphql/mutations";
-import { DEFAULT_MODERATOR_INSTRUCTIONS } from "../src/assets/configurations/constants";
+import { DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS } from "../src/assets/configurations/constants";
 import { docScreenshot } from "./utils/docScreenshot";
 
 const TEST_CORPUS_ID = "corpus-123";
@@ -900,8 +900,10 @@ test.describe("CreateCorpusActionModal", () => {
   }) => {
     let successCount = 0;
 
-    // DEFAULT_MODERATOR_INSTRUCTIONS is imported from the source module so the
-    // mutation variables stay in sync if the default ever changes.
+    // The default trigger is "add_document" (a document trigger), so the
+    // textarea initialises with DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS. Importing
+    // the constant keeps the mutation variables in sync if the default copy
+    // ever changes.
     const inlineCreateMock: MockedResponse = {
       request: {
         query: CREATE_CORPUS_ACTION,
@@ -917,10 +919,10 @@ test.describe("CreateCorpusActionModal", () => {
           createAgentInline: true,
           inlineAgentName: "Inline Doc Agent",
           inlineAgentDescription: undefined,
-          // The component initializes the agent-instructions textarea with
-          // DEFAULT_MODERATOR_INSTRUCTIONS; this test does not fill it and
-          // relies on that default matching the mutation variable.
-          inlineAgentInstructions: DEFAULT_MODERATOR_INSTRUCTIONS,
+          // The test does not fill the agent-instructions textarea, so the
+          // submitted value is whatever the component initialises it to for the
+          // default (document-add) trigger.
+          inlineAgentInstructions: DEFAULT_DOCUMENT_AGENT_INSTRUCTIONS,
           inlineAgentTools: ["read_document_text", "update_document_summary"],
           disabled: false,
           runOnAllCorpuses: false,