feat(conventions): front-load testing + forms idiom guides (test churn 61%→16% live)

packages/core/src/agent/agent.constants.ts

@@ -366,9 +366,11 @@ export const PULL_CONVENTIONS_TOOL = {
             "forms",
             "data-fetching",
             "lint-gotchas",
+            "testing",
+            "api-service",
           ],
           description:
-            "which guide: component-anatomy (where a component lives + one-per-file), file-layout (no inline types/constants/helpers), jsx (no computation in markup), state (hooks, not component body), no-casts (type guards instead of `as`/`!`), routing (thin route files), forms, data-fetching (api-client, never raw fetch), lint-gotchas (await promises, no void-expr values, no stringified errors, no duplicate strings).",
+            "which guide: component-anatomy (where a component lives + one-per-file), file-layout (no inline types/constants/helpers), jsx (no computation in markup), state (hooks, not component body), no-casts (type guards instead of `as`/`!`), routing (thin route files), forms, data-fetching (api-client, never raw fetch), lint-gotchas (await promises, no void-expr values, no stringified errors, no duplicate strings), testing (.test.ts vs .test.tsx, the vi.hoisted api-client mock, createApp/app.handle route tests, enforced test rules), api-service (mutating service methods record an audit event; throw ApiError).",
         },
       },
       required: ["topic"],

packages/core/src/loop/conventions.ts

@@ -23,6 +23,8 @@ const TOPICS = [
   "forms",
   "data-fetching",
   "lint-gotchas",
+  "testing",
+  "api-service",
 ] as const;
 
 export type ConventionTopic = (typeof TOPICS)[number];
@@ -60,18 +62,35 @@ export const TOPIC_RULES: Readonly<Record<ConventionTopic, readonly string[]>> =
       "no-error-stringify",
       "no-duplicate-string",
     ],
+    // Tests were 61% of the edits on a live CRUD build (measured) — the model doesn't
+    // know the stack's test idioms so it flails: guesses .test.ts vs .test.tsx (and makes
+    // BOTH), reinvents the api-client mock (getting `any`-typed `data`), uses the wrong
+    // vi mock method. These rules fire on those mistakes; the guide teaches the idiom.
+    testing: [
+      "test-sibling-required",
+      "test-file-mirrors-source",
+      "no-focused-tests",
+      "no-conditional-expect",
+      "no-real-network-in-unit-tests",
+      "fake-timers-must-be-restored",
+    ],
+    // The audit-event rule is a boringstack-OWN eslint rule (not a tsforge meta-rule), so it
+    // isn't keyed here for the reactive PUSH — the front-loaded guide is the delivery path.
+    "api-service": [],
   };
 
 const GUIDES: Readonly<Record<ConventionTopic, string>> = {
   "component-anatomy":
     "COMPONENT ANATOMY (boringstack). A feature lives in `src/features/<feature>/`. " +
-    "Components go under `src/features/<feature>/components/<Name>/`: `<Name>.tsx` " +
-    "renders props (it does NOT own state), and `index.ts` re-exports the default — " +
-    "ONE component per file. State/effects/memo live in `<Name>.hooks.ts`, never in " +
-    "the component body — the component imports the hook and consumes its return " +
-    "value. Feature-level files sit at `src/features/<feature>/`: `<Feature>.types.ts`, " +
-    "`<Feature>.constants.ts`, `<Feature>.queries.ts`, `<Feature>.mutations.ts`. shadcn " +
-    "primitives in `src/components/ui/` are exempt.",
+    "Components go under `src/features/<feature>/components/<Name>/`, and component-folder-" +
+    "structure requires the FULL sibling set — create ALL of them or the gate rejects the " +
+    "folder ('missing required siblings'): `<Name>.tsx` (renders props, does NOT own state), " +
+    "`<Name>.hooks.ts` (all state/effects/memo — never in the body), `<Name>.types.ts` (its " +
+    "Props interface), `<Name>.stories.tsx` (a Storybook story — REQUIRED, easy to forget), " +
+    "`<Name>.test.tsx` (or `.test.ts`), and `index.ts` (`export { default as <Name> } from " +
+    '"./<Name>"`). ONE component per file. Feature-level files sit at `src/features/<feature>/`: ' +
+    "`<Feature>.types.ts`, `<Feature>.constants.ts`, `<Feature>.queries.ts`, " +
+    "`<Feature>.mutations.ts`. shadcn primitives in `src/components/ui/` are exempt.",
   "file-layout":
     "FILE PURITY (boringstack). A component `.tsx` holds ONLY imports + the component " +
     "— nothing else atop it. Move each out and import it back: a type → " +
@@ -84,7 +103,14 @@ const GUIDES: Readonly<Record<ConventionTopic, string>> = {
     "already-computed values. A derived value → a `useMemo` in `<feature>.hooks.ts`; " +
     "a pure transform → a function in `src/lib`. A simple ternary is fine; a " +
     "`.map()`/`.filter()`/arithmetic/`Object.entries()` in the markup is not (extract " +
-    "it). Every `<button>` needs an explicit `type`.",
+    "it). Every `<button>` needs an explicit `type`. A function passed to a JSX prop " +
+    "(`onClick`, `onChange`, `onSubmit`) must be a STABLE reference — `react/jsx-no-bind` " +
+    "rejects BOTH an inline arrow (`onClick={() => …}`) AND a plain arrow defined in the " +
+    "component body (it's recreated every render). Make it stable: for a list ROW, give " +
+    "the row its own component with an `onEdit(id)`/`onDelete(id)` prop and pass that prop " +
+    "straight to `onClick`; the parent supplies each callback via `useCallback` in " +
+    "`<feature>.hooks.ts`. A handler needing an argument → `useCallback(() => onEdit(id), " +
+    "[onEdit, id])` in the row's hook, not an inline `() => onEdit(id)` in the markup.",
   state:
     "STATE (boringstack). ALL `useState`/`useReducer`/`useEffect`/`useMemo`/" +
     "`useCallback` live in `<feature>.hooks.ts`, never in a component body. Server " +
@@ -106,9 +132,21 @@ const GUIDES: Readonly<Record<ConventionTopic, string>> = {
     "router (`src/app/router/routes.tsx`) pointing at `@/features/<feature>` — never " +
     "hand-write a component's body in a route file.",
   forms:
-    "FORMS (boringstack). Use react-hook-form's `useForm` inside `<Component>.hooks.ts` " +
-    "(not the component body). Map server/validation errors back onto the form fields; " +
-    "keep the component rendering the field state the hook returns.",
+    "FORMS (boringstack). Use react-hook-form's `useForm<T>({ resolver: zodResolver(schema) })` " +
+    "(from `react-hook-form` + `@hookform/resolvers/zod`) inside `<Component>.hooks.ts`, not the " +
+    "component body. Submit via the returned `handleSubmit(onSubmit)` — do NOT hand-type the " +
+    "submit handler with React's `FormEvent` (it's the wrong type here and a repeatedly-invented " +
+    "error); if you must name the event it is a `BaseSyntheticEvent`, and fire it as " +
+    "`void handleSubmit(onSubmit)(event)` (the `void` satisfies no-floating-promises). In the Zod " +
+    "schema use the TOP-LEVEL validators — `z.email()`, `z.url()`, `z.uuid()` — NOT the deprecated " +
+    "`z.string().email()`. RESOLVER TYPES: do NOT put `.optional()`/`.default()` on a form field's " +
+    "schema — it makes the Zod INPUT type differ from the OUTPUT type, so `zodResolver` yields a " +
+    "`Resolver<In, any, Out>` that won't match `useForm`/`SubmitHandler` (a persistent 'Resolver … " +
+    "not assignable' / 'not assignable to SubmitHandler' error). Keep every field required in the " +
+    "schema and supply its initial value in `useForm({ defaultValues })`; type the hook as " +
+    "`useForm<z.infer<typeof schema>>` so onSubmit's input matches `SubmitHandler`. Map " +
+    "server/validation errors back onto the fields (`setError`); keep " +
+    "the component rendering the field state the hook returns.",
   "data-fetching":
     "DATA-FETCHING (boringstack). ALL HTTP goes through the generated client " +
     "`@/lib/api/client` — never `fetch`/`axios` (lint-banned). Call it as " +
@@ -133,6 +171,66 @@ const GUIDES: Readonly<Record<ConventionTopic, string>> = {
     '`log.error({ err }, "failed")`, never `` log.error(`${err}`) `` or `String(err)`.\n' +
     "• HOIST heavily-repeated string literals — the same literal 5+ times is a no-duplicate-string " +
     "error; pull it into a `const` (or `<Feature>.constants.ts`) and reference that.",
+  testing:
+    "TESTING (boringstack). Every logic file needs a co-located test (test-sibling-required), " +
+    "and tests are where builds waste the MOST turns — because the model guesses the idioms. " +
+    "Write them right the first time:\n" +
+    "• EXTENSION — pick ONE, never both: `.test.tsx` ONLY if the test renders JSX (`renderHook` " +
+    "with a wrapper, `render`, any `<X/>`); `.test.ts` for pure logic (schemas, utils, services, " +
+    "API route tests). The sibling check accepts EITHER, so if you create both, the unused twin " +
+    "becomes a knip `unused file` error. A `<X>` inside a `.test.ts` parses as a generic and fails " +
+    "with `'>' expected` — that means the file must be `.test.tsx`.\n" +
+    "• UI query/mutation/hook tests (`.test.tsx`) — mock the api-client HOISTED at the top, exactly " +
+    "this shape:\n" +
+    "    const apiMock = vi.hoisted(() => ({ GET: vi.fn(), POST: vi.fn(), PATCH: vi.fn(), PUT: vi.fn(), DELETE: vi.fn() }));\n" +
+    '    vi.mock("@/lib/api/client", () => ({ apiClient: apiMock }));\n' +
+    "  Return a CONCRETE typed object from the mock — `apiMock.POST.mockResolvedValueOnce({ data: { " +
+    "...the real response shape... } })`. A concrete literal keeps `data` typed; a bare `vi.fn()` " +
+    "return is `any`, which then trips no-unsafe/no-unnecessary-condition in the code under test " +
+    "(do NOT try to fix that by casting or changing production code). Use `mockResolvedValueOnce` " +
+    "per call; use `mockResolvedValue` only for a call that retries (e.g. a GET-me loop). RESET the " +
+    "mocks between tests — `beforeEach(() => { apiMock.GET.mockReset(); apiMock.POST.mockReset(); … })` " +
+    "(or `vi.resetAllMocks()`; NOT `vi.clearAllMocks()`, which clears only call history and leaves " +
+    "queued return values in place) — or a leftover `mockResolvedValue` leaks into the next test (it " +
+    "passes alone but fails in the suite). Wrap the " +
+    "hook in a `QueryClient` with `retry:false` via `renderHook(() => useX(), { wrapper })` and " +
+    "assert with `await waitFor(() => …)`. Imports: `vitest` (`describe, it, expect, vi, beforeEach`) " +
+    "+ `@testing-library/react` (`renderHook, waitFor, act`) + `@tanstack/react-query`.\n" +
+    "• API tests (apps/api) run under `bun:test`, NOT vitest — `import { describe, expect, test } " +
+    'from "bun:test"` (vitest + `vi.*` are UI-only; using them in an apps/api test fails to resolve). ' +
+    "For a `*.service.ts` whose function hits the DB (Drizzle), do NOT unit-test it in isolation — you'll " +
+    "fight `string | SQLWrapper` types and need a live DB. Its behaviour is covered by the route test " +
+    "below; the `*.service.test.ts` only needs to satisfy the test-sibling floor with a minimal smoke " +
+    'test (e.g. `expect(typeof myServiceFn).toBe("function")`). Put the real assertions in the route test.\n' +
+    "• API route tests (`apps/api/tests/**/*.routes.test.ts`, pure `.ts`) — handle the app in-process, " +
+    "never a real network (no-real-network-in-unit-tests):\n" +
+    '    const app = createApp(); // from "../../../src/config/app"\n' +
+    '    const res = await app.handle(new Request("http://localhost/api/v1/<path>", { method, headers, body }));\n' +
+    "    expect(res.status).toBe(200);\n" +
+    "  Read the body as `unknown` then TYPE-GUARD it (`const isX = (v: unknown): v is {…} => …; if " +
+    "(!isX(body)) throw new Error(…)`) — never `as`. For an authed route, define a local `loginCookie` " +
+    "helper (copy it verbatim from an existing `*.routes.test.ts`) and pass the returned cookie in headers.\n" +
+    "• RULES the gate enforces: no `.only`/`.skip` (no-focused-tests); never put `expect` inside an " +
+    "`if`/loop/`switch`/ternary — assert unconditionally (no-conditional-expect); if you use fake " +
+    "timers, restore them in `afterEach` (fake-timers-must-be-restored); the test path/name mirrors " +
+    "its source (test-file-mirrors-source).\n" +
+    "• After you write a test the harness AUTO-FORMATS it (imports reordered, quotes/commas normalized), " +
+    "so your next `edit` oldString won't match — RE-READ the file and copy oldString from its current " +
+    "content; do NOT recreate the whole file.",
+  "api-service":
+    "API SERVICE (boringstack). apps/api resource logic lives in `<entity>.service.ts`. Every " +
+    "MUTATING method (create/update/delete) MUST record an audit event — the gate rejects one that " +
+    "doesn't (`Mutating method '…' does not record an audit event`). Import `{ AUDIT_ACTIONS, " +
+    'auditLogService } from "../../lib/audit-log"` and, after the mutation, call ' +
+    "`void auditLogService.record({ userId, action: AUDIT_ACTIONS.<ENTITY_ACTION>, metadata: { … } })` " +
+    "(the `void` satisfies no-floating-promises). Read-only methods (get/list) don't need it. Surface " +
+    "failures by THROWING an `ApiError` (e.g. 404/409), not by returning an error envelope — the route " +
+    "layer maps thrown ApiErrors to responses. Every ROUTE must declare a single-content-type " +
+    "`response:` schema (`.get(path, handler, { response: <Entity>ResponseSchema })`) — the UI's " +
+    "api-client is generated from this. If a route omits `response:` (or allows multiple content " +
+    "types), openapi-fetch types the UI's `data` as `Readable<SuccessResponse<…>>` instead of the " +
+    "JSON body, and the UI can't consume it (a persistent 'not assignable to …' error you canNOT fix " +
+    "on the UI side — fix the ROUTE's response schema).",
 };
 
 /** The guide for a topic (the exact string pushed or pulled). */

packages/core/tests/boringstack-conventions.test.ts

@@ -31,6 +31,49 @@ describe("convention registry", () => {
     expect(g).toContain("src/features/<feature>/components/");
     // The dead UI-only-React `src/views` layout must be gone.
     expect(g).not.toContain("src/views/");
+    // The FULL required sibling set incl the easily-forgotten .stories.tsx (build9 arch wall).
+    expect(g).toContain(".stories.tsx");
+    expect(g).toContain("missing required siblings");
+  });
+
+  test("jsx guide gives the exact jsx-no-bind fix for list-row handlers (build7 parking residual)", () => {
+    const g = conventionGuide("jsx");
+
+    expect(g).toContain("react/jsx-no-bind");
+    expect(g).toContain("STABLE reference");
+    // Both an inline arrow AND a body-defined arrow are rejected — the model kept doing the latter.
+    expect(g).toContain("recreated every render");
+    expect(g).toContain("useCallback");
+    expect(g).toContain("onEdit(id)");
+  });
+
+  test("api-service guide teaches the audit-event idiom on mutating methods (build6 hard-gate residual)", () => {
+    const g = conventionGuide("api-service");
+
+    expect(g).toContain("auditLogService.record");
+    expect(g).toContain("AUDIT_ACTIONS");
+    expect(g).toContain("audit event");
+    // Mutations only; reads exempt; throw ApiError (not error envelopes).
+    expect(g).toContain("create/update/delete");
+    expect(g).toContain("ApiError");
+    // Panel-diagnosed root cause of the Readable<SuccessResponse> UI residual: route response schema.
+    expect(g).toContain("response:");
+    expect(g).toContain("Readable<SuccessResponse");
+  });
+
+  test("forms guide steers away from the invented FormEvent + deprecated z.string().email() (live-build residuals)", () => {
+    const g = conventionGuide("forms");
+
+    expect(g).toContain("zodResolver");
+    expect(g).toContain("handleSubmit");
+    // The model repeatedly invented React's FormEvent and used the deprecated Zod string email.
+    expect(g).toContain("FormEvent");
+    expect(g).toContain("z.email()");
+    expect(g).toContain("z.string().email()");
+    expect(g).toContain("BaseSyntheticEvent");
+    // build8 park (#67): the rhf resolver input≠output type mismatch from .optional()/.default().
+    expect(g).toContain("SubmitHandler");
+    expect(g).toContain("defaultValues");
   });
 
   test("data-fetching guide states the apiClient pattern and forbids response.error", () => {
@@ -41,6 +84,58 @@ describe("convention registry", () => {
     expect(g).toContain("response.error");
     expect(g).toContain("throwOnError");
   });
+
+  test("testing guide teaches the exact idioms the model kept failing (extension, hoisted mock, route tests, rules)", () => {
+    const g = conventionGuide("testing");
+
+    // The .test.ts vs .test.tsx decision + the never-both rule (the dual-extension churn).
+    expect(g).toContain(".test.tsx");
+    expect(g).toContain(".test.ts");
+    expect(g).toContain("never both");
+    // The api-client mock idiom the model reinvented 16× (getting `any`-typed data).
+    expect(g).toContain("vi.hoisted");
+    expect(g).toContain('vi.mock("@/lib/api/client"');
+    expect(g).toContain("mockResolvedValueOnce");
+    expect(g).toContain("keeps `data` typed");
+    // Mock reset between tests (deepseek-flagged gap): the pass-alone-fail-in-suite trap.
+    expect(g).toContain("mockReset");
+    expect(g).toContain("fails in the suite");
+    // The correct global reset is resetAllMocks (resets queued return values); clearAllMocks
+    // only clears call history and would NOT prevent the leak — the guide must steer away from it.
+    expect(g).toContain("resetAllMocks");
+    expect(g).toContain("NOT `vi.clearAllMocks()`");
+    // The API route test idiom from the shipped example.
+    expect(g).toContain("createApp()");
+    expect(g).toContain("app.handle");
+    expect(g).toContain("loginCookie");
+    // API runner is bun:test (not vitest) + the service-test smoke idiom (build4 residual:
+    // 14 edits grinding a DB-hitting supplier.service.test.ts against Drizzle types).
+    expect(g).toContain('from "bun:test"');
+    expect(g).toContain("smoke");
+    expect(g).toContain('expect(typeof myServiceFn).toBe("function")');
+    // The enforced test rules, each named so the model connects error → fix.
+    expect(g).toContain("no-focused-tests");
+    expect(g).toContain("no-conditional-expect");
+    expect(g).toContain("fake-timers-must-be-restored");
+    // The auto-reformat re-read (the not-found edit-reject churn).
+    expect(g).toContain("AUTO-FORMATS");
+  });
+});
+
+describe("topicForRule (testing)", () => {
+  test("EVERY testing rule maps to the testing topic so its gate error pushes the guide", () => {
+    // Lock all six — a typo in any would silently disable the reactive PUSH for that error.
+    for (const rule of [
+      "test-sibling-required",
+      "test-file-mirrors-source",
+      "no-focused-tests",
+      "no-conditional-expect",
+      "no-real-network-in-unit-tests",
+      "fake-timers-must-be-restored",
+    ]) {
+      expect(topicForRule(rule)).toBe("testing");
+    }
+  });
 });
 
 describe("topicForRule", () => {