Fix twenty findings from the Motion review

Each was raised by a reviewer and then survived an independent attempt to
refute it. The four that mattered most:

- A third of the starter library was invisible. Three templates authored
  `fields` shapes no renderer read — decisions, blockingSet, checks,
  steps and the rest — so about forty records rendered as no DOM at all,
  in the library and again on the engagement that instantiated them.
  Nothing failed: a renderer returns null for a key set it does not
  recognise, and a header-plus-body page looks like a template written
  that way. FieldsView now reads every key the seeds carry.
- "Add a framework" opened a picker that could never match, because the
  dialog was seeded with both the forced kind and the deal's stage, and
  qualification serves only the qualification stage. The stage is now
  dropped when MOTION_KIND_STAGES says the pair is incoherent.
- Piggy reported the promotion count as an exact figure capped at 8,
  against a tile showing the true count beside it. It is now counted in
  SQL, and all three motion tools carry a ResultScope whose denominator
  is shared lineages — never rows, never private drafts.
- No Motion test went through createApp, so the whole feature could be
  unmounted with a green suite. That is the AGENTS.md §5 trap that
  already cost this project read-guards.ts and learn.ts.

Also: both sides of the instantiate/edit race now lock, so a template
cannot be rewritten under an artefact that has copied it; concurrent
engagement opens queue on the deal row and get the 409 the handler
already promised rather than a 500; latestScore uses DISTINCT ON instead
of losing engagements past a 200-row cap; the migration adds the
scored_by_user_id foreign key the schema declares; and the demo clear
refunds usage_count for engagements it reaches by cascade, which
otherwise left starter templates permanently un-editable.

Verified on a fresh database: 16 migrations apply and re-apply as a
no-op, both seeds idempotent, usage_count back to zero after --clear.
564 unit tests pass. Every Motion route measures zero horizontal
overflow at 393 and 1440 in both themes, and all twelve seeded field
trees are asserted onto the screen by scripts/motion-fields-check.mjs.

One thing left open deliberately: the shipped qualification scorecard's
five bands and MOTION_BANDS' four are calibrated differently. The
framework's table is now titled as its own guidance rather than the
product's verdict, which removes the contradiction on screen. Making the
framework's calibration authoritative over the persisted band column is
a product decision nobody has made.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

apps/api/src/routes/motion.ts

@@ -42,7 +42,7 @@ import type {
   MotionTemplate,
   QualificationScore,
 } from '@pig/db';
-import { demandDeals, engagementArtifacts, engagements, motionTemplates } from '@pig/db';
+import { demandDeals, engagementArtifacts, engagements, motionTemplates, users } from '@pig/db';
 import { eq } from 'drizzle-orm';
 import { Hono } from 'hono';
 import { z } from 'zod';
@@ -62,6 +62,7 @@ import {
   loadEngagement,
   loadTemplateForWrite,
   lockNewestVersion,
+  lockTemplateForWrite,
   motionSlug,
   MotionService,
   newestVisibleInLineage,
@@ -247,6 +248,27 @@ async function checkedTemplateId(
   await loadTemplateForWrite(tx, viewerOf(principal), id);
 }
 
+/**
+ * An owner id somebody sent, checked before it is stored.
+ *
+ * `owner_user_id` is a foreign key with nothing in front of it, so an id for a
+ * user who has since been removed leaves as a 500 from the constraint rather
+ * than the 404 every other reference here answers with. Only ever the id the
+ * client actually supplied: an engagement created without one defaults to the
+ * caller, and re-reading a user the request has just authenticated would be a
+ * query bought with nothing — the same decision `calendar.ts` makes, and one a
+ * test there pins. It also leaves an explicit `ownerUserId: null` — unassigning
+ * — as the no-op it is.
+ */
+async function checkedOwnerUserId(
+  tx: MotionTransaction,
+  id: string | null | undefined,
+): Promise<void> {
+  if (!id) return;
+  const [owner] = await tx.select({ id: users.id }).from(users).where(eq(users.id, id)).limit(1);
+  if (!owner) throw MutationError.notFound('User');
+}
+
 /** The same gate as `authorizePublish`, applied once the body says `shared`. */
 function requirePublishFor(principal: Principal, visibility: string | undefined): void {
   if (visibility === 'shared') requireAnyTeamCapability(principal, 'motion:publish');
@@ -365,7 +387,11 @@ export function motionTemplateUpdateDefinition(): MutationDefinition<
     invalidMessage: 'Invalid motion template change.',
     async mutate({ input, params, principal, tx, now }) {
       const viewer = viewerOf(principal);
-      const existing = await loadTemplateForWrite(
+      // Locked, so the `usage_count` that §7a is about to be judged on is the
+      // one belonging to the row this transaction goes on to write. Read
+      // without the lock it is a count that a concurrent instantiation can
+      // move between the check and the UPDATE.
+      const existing = await lockTemplateForWrite(
         tx,
         viewer,
         requiredId(params, 'Motion template'),
@@ -581,13 +607,22 @@ export function motionEngagementCreateDefinition(): MutationDefinition<
     permission: authorizeWrite,
     invalidMessage: 'Invalid engagement.',
     async mutate({ input, principal, tx, now }) {
+      // Locked, because the existence check below is only worth making if it
+      // cannot be raced. Two simultaneous opens of one deal both saw nothing,
+      // both inserted, and the loser got `23505` on
+      // `engagements_demand_deal_key` — not a `MutationError`, so a 500 with
+      // no way to tell that an engagement now exists. Concurrent opens queue
+      // on this row instead, and the loser's check sees the committed
+      // engagement and answers with its id, as the comment below promises.
       const [deal] = await tx
         .select()
         .from(demandDeals)
         .where(eq(demandDeals.id, input.demandDealId))
-        .limit(1);
+        .limit(1)
+        .for('update');
       if (!deal) throw MutationError.notFound('Demand deal');
       await checkedTemplateId(tx, principal, input.playbookTemplateId);
+      await checkedOwnerUserId(tx, input.ownerUserId);
 
       // Checked rather than left to the unique constraint, so the caller gets
       // the id of the engagement that already exists instead of a 500.
@@ -643,6 +678,7 @@ export function motionEngagementUpdateDefinition(): MutationDefinition<
     async mutate({ input, params, principal, tx, now }) {
       const { engagement, deal } = await loadEngagement(tx, requiredId(params, 'Engagement'));
       await checkedTemplateId(tx, principal, input.playbookTemplateId);
+      await checkedOwnerUserId(tx, input.ownerUserId);
 
       const [updated] = await tx
         .update(engagements)

apps/api/src/services/motion.ts

@@ -692,22 +692,28 @@ export class MotionService {
           ),
         )
         .groupBy(engagementArtifacts.engagementId),
-      // Newest first, then the first sighting of each engagement wins. A
-      // `DISTINCT ON` would be tidier but this stays one bounded query and the
-      // history panel needs the same ordering anyway.
+      // `DISTINCT ON`, because folding a `scored_at desc` scan in memory drops
+      // engagements rather than truncating a list. A hundred engagements
+      // carrying three scores each can spend the whole 200-row budget on the
+      // sixty most recently scored, and every other row then renders "never
+      // scored" beside an exact artefact count, with nothing anywhere saying
+      // so. Postgres requires the ORDER BY to lead with the distinct
+      // expression, which is why `engagement_id` comes first; that is also the
+      // leading column of `qualification_scores_engagement_idx`, so this reads
+      // straight off the index. The result is bounded by `ids.length` and the
+      // limit is left as a cap rather than a budget.
       this.db
-        .select()
+        .selectDistinctOn([qualificationScores.engagementId])
         .from(qualificationScores)
         .where(inArray(qualificationScores.engagementId, ids))
-        .orderBy(desc(qualificationScores.scoredAt))
+        .orderBy(qualificationScores.engagementId, desc(qualificationScores.scoredAt))
         .limit(SCORE_LIMIT),
     ]);
 
     const countById = new Map(counts.map((row) => [row.engagementId, row.total]));
-    const latest = new Map<string, QualificationScore>();
-    for (const score of scores) {
-      if (!latest.has(score.engagementId)) latest.set(score.engagementId, score);
-    }
+    const latest = new Map<string, QualificationScore>(
+      scores.map((score) => [score.engagementId, score]),
+    );
 
     return rows.map(({ engagement, dealName, stage, accountId, accountName }) => ({
       id: engagement.id,
@@ -759,11 +765,52 @@ export async function loadTemplateForWrite(
   viewer: MotionViewer,
   id: string,
 ): Promise<MotionTemplate> {
-  const [row] = await tx
+  return templateForWrite(tx, viewer, id, false);
+}
+
+/**
+ * The same read, holding the row until this transaction commits.
+ *
+ * `usage_count` is the whole of §7a, and under READ COMMITTED reading it
+ * without the lock is a decision taken about a row somebody else is already
+ * changing. Two requests against a template at zero: an instantiation copies
+ * the body and increments the count, while a PATCH that read zero a moment
+ * earlier blocks on the row lock and then applies anyway — leaving an artefact
+ * whose `template_id` names a template that no longer contains what it copied,
+ * which is the provenance failure the rule exists to prevent.
+ *
+ * Both sides therefore take this lock, and locking only the edit does not
+ * work: the copy has to hold the row until its increment commits, or it is
+ * still reading a body that is about to change. Whichever transaction locks
+ * first serialises the other, and a blocked locking read re-fetches the
+ * committed tuple — so the edit honestly sees `usage_count = 1` and answers
+ * `409 template_in_use`, or the instantiation honestly copies the edited body.
+ *
+ * `no key update` rather than `update`, because the row is a foreign-key
+ * target — `engagement_artifacts.template_id` — and nothing here is a reason
+ * to block an insert that merely references it.
+ */
+export async function lockTemplateForWrite(
+  tx: MotionTransaction,
+  viewer: MotionViewer,
+  id: string,
+): Promise<MotionTemplate> {
+  return templateForWrite(tx, viewer, id, true);
+}
+
+/** One predicate for both, so the lock cannot drift away from the read filter. */
+async function templateForWrite(
+  tx: MotionTransaction,
+  viewer: MotionViewer,
+  id: string,
+  lock: boolean,
+): Promise<MotionTemplate> {
+  const query = tx
     .select()
     .from(motionTemplates)
     .where(and(eq(motionTemplates.id, id), visibleTemplates(viewer)))
     .limit(1);
+  const [row] = lock ? await query.for('no key update') : await query;
   if (!row) throw MutationError.notFound('Motion template');
   return row;
 }
@@ -905,8 +952,11 @@ export async function instantiateArtifact(
   input: InstantiateInput,
   now: Date,
 ): Promise<{ artifact: EngagementArtifact; template: MotionTemplate | null }> {
+  // Locked, not merely read: the body copied below has to be the body the
+  // `usage_count` increment at the end of this function is counting, or a
+  // concurrent edit rewrites the template between the two.
   const template = input.templateId
-    ? await loadTemplateForWrite(tx, viewer, input.templateId)
+    ? await lockTemplateForWrite(tx, viewer, input.templateId)
     : null;
   // Every read path hides an archived template, so instantiating one can only
   // happen by holding an id from before it was archived — and it would leave

apps/api/test/motion.test.ts

@@ -17,6 +17,11 @@
  * queries in call order and records what ran; it is deliberately not a
  * database, because a fake that pretends to run SQL is a fake that will one day
  * assert a broken query works.
+ *
+ * That fake is also why the last suite goes through `createApp` instead. A
+ * definition driven directly, or a route mounted by this file's own `mounted`,
+ * passes whether or not `app.ts` ever calls `createMotionRoutes` — the state
+ * `read-guards.ts` and `learn.ts` were both in while their tests were green.
  */
 import { strict as assert } from 'node:assert';
 import { describe, it } from 'node:test';
@@ -24,8 +29,12 @@ import { getTableName, isSQLWrapper, isTable, type SQL } from 'drizzle-orm';
 import { PgDialect } from 'drizzle-orm/pg-core';
 import { motionScoreBasisPoints } from '@pig/core';
 import type { Database, MotionTemplate } from '@pig/db';
+import { teamMemberships, users } from '@pig/db';
 import { Hono } from 'hono';
+import { createApp } from '../src/app';
 import { AuthError, type Principal } from '../src/lib/auth';
+import type { AuthProvider } from '../src/lib/auth-provider';
+import { loadConfig } from '../src/lib/config';
 import { executeMutation, MutationError, type ApiEnv } from '../src/lib/mutation';
 import {
   createMotionRoutes,
@@ -134,7 +143,13 @@ function database(script: unknown[][]): { db: Database; log: Recorded } {
     // can follow it, as it does on every query in this feature that allocates
     // a version number. The chain is a thenable, so `await` still ends it.
     limit: () => selectChain,
-    for: () => selectChain,
+    // Recorded, not merely tolerated: two of the rules here are held shut by a
+    // row lock, and a lock that quietly stops being taken changes nothing a
+    // behavioural assertion can see.
+    for: (strength: string) => {
+      log.events.push(`for:${strength}`);
+      return selectChain;
+    },
     then: (resolve: (rows: unknown[]) => unknown) => resolve(next()),
   };
 
@@ -281,6 +296,26 @@ describe('a used template is never edited in place', () => {
     assert.equal(log.updated[0]?.values.title, 'POC plan, tightened');
   });
 
+  it('locks the row before it trusts usage_count, so a concurrent instantiation cannot be missed', async () => {
+    const { db, log } = database([[template({ usageCount: 0 })]]);
+
+    await executeMutation(
+      db,
+      member,
+      async () => ({ body: '# Tightened' }),
+      motionTemplateUpdateDefinition(),
+      { id: TEMPLATE_ID },
+    );
+
+    // Read without the lock, `usage_count` is a number another transaction is
+    // already moving: an instantiation copies the body and increments the
+    // count while this PATCH, having seen zero, waits on the row and then
+    // rewrites the body anyway — leaving an artefact whose `template_id` names
+    // a template that no longer contains what it copied. `no key update`
+    // rather than `update` because the row is a foreign-key target.
+    assert.deepEqual(log.events.slice(0, 3), ['transaction', 'select', 'for:no key update']);
+  });
+
   it('refuses an edit to somebody else\'s template even when it is shared — publishing is not donating', async () => {
     const { db, log } = database([[template({ visibility: 'shared', ownerUserId: OTHER })]]);
 
@@ -661,6 +696,30 @@ describe('instantiating a template', () => {
     // back into the library.
     assert.equal(log.inserted[0]?.row.body, template().body);
   });
+
+  it('holds the template row while it copies the body, not only while it counts the use', async () => {
+    const { db, log } = database([
+      [{ engagement, deal }],
+      [template({ visibility: 'shared', ownerUserId: OTHER })],
+    ]);
+
+    await executeMutation(
+      db,
+      member,
+      async () => ({ templateId: TEMPLATE_ID }),
+      motionArtifactCreateDefinition(),
+      { id: ENGAGEMENT_ID },
+    );
+
+    // Locking only the PATCH does not close the race: the copy has to hold the
+    // row until its own increment commits, or an edit lands between the read
+    // that copied the body and the count that was supposed to have shut the
+    // template to edits.
+    assert.deepEqual(
+      log.events.slice(0, 4),
+      ['transaction', 'select', 'select', 'for:no key update'],
+    );
+  });
 });
 
 describe('one engagement per deal', () => {
@@ -682,6 +741,27 @@ describe('one engagement per deal', () => {
     );
     assert.deepEqual(log.inserted, []);
   });
+
+  it('locks the deal before it looks, so two simultaneous opens queue instead of both inserting', async () => {
+    const { db, log } = database([[deal], []]);
+
+    await executeMutation(
+      db,
+      member,
+      async () => ({ demandDealId: DEAL_ID }),
+      motionEngagementCreateDefinition(),
+    );
+
+    // The race is not expressible against this fake — it runs no SQL and has
+    // no concurrency — so what is pinned is the lock that removes it. Without
+    // it both requests' checks above see nothing, both insert, and the loser
+    // gets `23505` on `engagements_demand_deal_key`, which is not a
+    // `MutationError` and so leaves as `500 Internal error` with no way to
+    // tell that an engagement now exists. The lock is taken on the deal
+    // because the row the loser must wait behind is the engagement that does
+    // not exist yet.
+    assert.deepEqual(log.events.slice(0, 3), ['transaction', 'select', 'for:update']);
+  });
 });
 
 describe('qualification scores', () => {
@@ -784,3 +864,86 @@ describe('an id that cannot name a row is a row that does not exist', () => {
     assert.deepEqual(log.events, ['transaction'], 'refused on shape, without a query');
   });
 });
+
+// --------------------------------------------------- the mount, not the mock
+
+const SUBJECT = 'motion-auth-subject';
+
+/**
+ * The whole app, with the cheapest database that can carry an authenticated
+ * request through it — the fixture `http-auth.test.ts` uses, which answers by
+ * table identity rather than in call order because the queries `loadPrincipal`
+ * and the overview make are an implementation detail.
+ */
+function createdApp(): ReturnType<typeof createApp> {
+  const user = {
+    id: OWNER,
+    email: 'seller@example.com',
+    name: 'Seller',
+    authSubject: SUBJECT,
+    deactivatedAt: null,
+    isPlatformAdmin: false,
+  };
+
+  function chain(rows: unknown[]): Record<string, unknown> {
+    const self: Record<string, unknown> = {
+      leftJoin: () => self,
+      innerJoin: () => self,
+      where: () => self,
+      orderBy: () => self,
+      groupBy: () => self,
+      limit: () => self,
+      then: (resolve: (value: unknown[]) => unknown) => resolve(rows),
+    };
+    return self;
+  }
+
+  const db = {
+    select: () => ({
+      from: (table: unknown) => {
+        if (table === users) return chain([user]);
+        if (table === teamMemberships) return chain([{ team: 'demand', role: 'member' }]);
+        return chain([]);
+      },
+    }),
+    update: () => ({ set: () => ({ where: async () => undefined }) }),
+    transaction: async (work: (tx: unknown) => Promise<unknown>) => work({}),
+  } as unknown as Database;
+
+  const provider: AuthProvider = {
+    name: 'test',
+    async verifyAccessToken(token: string) {
+      if (token !== 'good-token') throw new Error('bad token');
+      return { subject: SUBJECT, email: user.email };
+    },
+  };
+
+  // A real `loadConfig`, for the reason `http-auth.test.ts` gives: a hand-built
+  // Config object would let this pass under one the server refuses to start on.
+  const config = loadConfig({
+    NODE_ENV: 'test',
+    DATABASE_URL: 'postgres://pig:pig@localhost:5432/pig-not-connected',
+    PIG_PUBLIC_URL: 'http://localhost:8920',
+    PIG_ADMIN_EMAILS: '',
+  } as NodeJS.ProcessEnv);
+
+  return createApp(config, db, provider);
+}
+
+describe('the routes are mounted in app.ts, not only in this file', () => {
+  it('answers a member holding book:read on GET /api/motion', async () => {
+    // AGENTS.md §5, verbatim: every other test here mounts the factory itself,
+    // so all twelve routes could be dropped from `app.ts` and this file would
+    // stay green. The request is authenticated deliberately — `app.use('/api/*')`
+    // authenticates ahead of every feature route, so an anonymous GET answers
+    // 401 whether or not anything is mounted behind it, and the same assertion
+    // would pass against a feature that had been deleted outright.
+    const response = await createdApp().request('/api/motion', {
+      headers: { authorization: 'Bearer good-token' },
+    });
+
+    assert.notEqual(response.status, 404, 'createMotionRoutes is not mounted in createApp');
+    assert.notEqual(response.status, 401, 'the request never reached the motion handler');
+    assert.notEqual(response.status, 403, 'the read guard answered, so the mount is untested');
+  });
+});