fix(provenance): address #143 review — page-stamp tests, confine is_agent, doc fixes

Resolves the open items from the latest PR #143 code review:

- test(page): cover the four agentSourceFields stamp sites (create, update,
  movePage, movePageToSpace) with agent + normal-user payload assertions;
  add findById({ includeIsAgent: true }) wiring guards to the JWT and collab
  auth-seam specs so a future drop of the option is caught.
- fix(privacy): drop `isAgent` from UserRepo.baseFields and gate it behind a
  new opt-in `findById({ includeIsAgent })`, requested only by the two auth
  seams that derive provenance — stops the flag leaking via the workspace
  member list and generic user payloads.
- docs: correct the agentSourceFields JSDoc and the two UPDATE-site comments
  to distinguish INSERT (omitted column → DB default 'user') from UPDATE
  (omitted column → existing value kept, Kysely writes only present keys).
- style(page): collapse three stray double blank lines left by an earlier edit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

apps/server/src/collaboration/extensions/authentication.extension.spec.ts

@@ -207,6 +207,14 @@ describe('AuthenticationExtension.onAuthenticate', () => {
 
     expect(ctx.actor).toBe('user');
     expect(ctx.aiChatId).toBeNull();
+    // Wiring guard (#143): the collab seam MUST opt into the isAgent flag —
+    // it is not in baseFields, so without this option findById omits it and a
+    // flagged service account's collab edits would silently persist as 'user'.
+    expect(userRepo.findById).toHaveBeenCalledWith(
+      USER_ID,
+      WORKSPACE_ID,
+      expect.objectContaining({ includeIsAgent: true }),
+    );
   });
 
   it('is_agent user with NO claim → actor=agent (collab seam consults the signed identity)', async () => {

apps/server/src/collaboration/extensions/authentication.extension.ts

@@ -44,7 +44,9 @@ export class AuthenticationExtension implements Extension {
     const userId = jwtPayload.sub;
     const workspaceId = jwtPayload.workspaceId;
 
-    const user = await this.userRepo.findById(userId, workspaceId);
+    const user = await this.userRepo.findById(userId, workspaceId, {
+      includeIsAgent: true,
+    });
 
     if (!user) {
       throw new UnauthorizedException();

apps/server/src/common/decorators/auth-provenance.decorator.ts

@@ -40,11 +40,13 @@ export function resolveProvenance(
 /**
  * Agent-edit write-stamp fields for a repository insert/update (#143 review).
  * Spread into the row being written: for an agent it stamps the `*Source`
- * column 'agent' and the AI-chat id; for a normal user it returns `{}` so the
+ * column 'agent' and the AI-chat id; for a normal user it returns `{}` — on an
- * column keeps its default ('user'). The only per-table variation is the column
+ * INSERT the omitted column falls back to its DB default ('user'); on an UPDATE
- * names, passed as `sourceKey`/`chatKey`, so the agent-stamp idiom lives in ONE
+ * the column simply keeps its existing stored value (Kysely only writes the keys
- * place instead of being hand-reimplemented at every write site (where a wrong
+ * present). The only per-table variation is the column names, passed as
- * literal or a forgotten `aiChatId` could drift).
+ * `sourceKey`/`chatKey`, so the agent-stamp idiom lives in ONE place instead of
+ * being hand-reimplemented at every write site (where a wrong literal or a
+ * forgotten `aiChatId` could drift).
  *
  *   insertComment({ ..., ...agentSourceFields(p, 'createdSource', 'aiChatId') })
  *   updatePage({ ..., ...agentSourceFields(p, 'lastUpdatedSource', 'lastUpdatedAiChatId') })

apps/server/src/core/auth/strategies/jwt.strategy.spec.ts

@@ -47,7 +47,7 @@ describe('JwtStrategy — provenance derivation', () => {
   });
 
   it("stamps actor='agent' for an is_agent user (derived from the signed identity)", async () => {
-    const { strategy } = makeStrategy({
+    const { strategy, userRepo } = makeStrategy({
       id: 'user-1',
       isAgent: true,
       deactivatedAt: null,
@@ -60,6 +60,14 @@ describe('JwtStrategy — provenance derivation', () => {
     expect(req.raw.actor).toBe('agent');
     // External MCP agent: no internal ai_chats row → null.
     expect(req.raw.aiChatId).toBeNull();
+    // Wiring guard (#143): the seam MUST opt into the isAgent flag, otherwise
+    // findById omits it (it is not in baseFields) and provenance silently
+    // degrades to 'user'.
+    expect(userRepo.findById).toHaveBeenCalledWith(
+      'user-1',
+      'ws-1',
+      expect.objectContaining({ includeIsAgent: true }),
+    );
   });
 
   it("stamps actor='user' for an ordinary user", async () => {

apps/server/src/core/auth/strategies/jwt.strategy.ts

@@ -56,7 +56,9 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
     if (!workspace) {
       throw new UnauthorizedException();
     }
-    const user = await this.userRepo.findById(payload.sub, payload.workspaceId);
+    const user = await this.userRepo.findById(payload.sub, payload.workspaceId, {
+      includeIsAgent: true,
+    });
 
     if (!user || isUserDisabled(user)) {
       throw new UnauthorizedException();

apps/server/src/core/page/services/page.service.spec.ts

@@ -147,4 +147,246 @@ describe('PageService', () => {
       expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe('agent provenance stamping (#143)', () => {
+    // Provenance handed to the four write sites. The agent case must surface the
+    // signed source marker + chat id on the persisted payload; the user case must
+    // leave both keys absent so the column keeps its INSERT default / existing
+    // UPDATE value (agentSourceFields returns {} for a non-agent).
+    const AGENT = { actor: 'agent', aiChatId: 'chat-7' } as any;
+    const USER = { actor: 'user', aiChatId: null } as any;
+
+    // A general-queue stub whose `.add(...)` returns a `{ catch }` thenable —
+    // the service does `generalQueue.add(...).catch(...)` and never awaits it.
+    const makeGeneralQueue = () =>
+      ({ add: jest.fn().mockReturnValue({ catch: jest.fn() }) }) as any;
+
+    // Build a PageService where only the deps a given site touches are real
+    // stubs; everything else stays a bare object. db is supplied per-test.
+    const makeSvc = (overrides: {
+      pageRepo?: any;
+      generalQueue?: any;
+      db?: any;
+    }) =>
+      new PageService(
+        (overrides.pageRepo ?? {}) as any, // pageRepo
+        {} as any, // pagePermissionRepo
+        {} as any, // attachmentRepo
+        (overrides.db ?? {}) as any, // db
+        {} as any, // storageService
+        {} as any, // attachmentQueue
+        {} as any, // aiQueue
+        (overrides.generalQueue ?? makeGeneralQueue()) as any, // generalQueue
+        {} as any, // eventEmitter
+        {} as any, // collaborationGateway
+        {} as any, // watcherService
+        {} as any, // transclusionService
+      );
+
+    describe('create() → insertPage', () => {
+      const run = async (provenance: any) => {
+        const pageRepo = {
+          insertPage: jest.fn().mockResolvedValue({ id: 'p1' }),
+        };
+        const svc = makeSvc({ pageRepo, generalQueue: makeGeneralQueue() });
+        // nextPagePosition runs a real db query; stub it out.
+        jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0' as any);
+        // No content/format → the prosemirror parse branch is skipped. No
+        // parentPageId → no parent lookup.
+        await svc.create(
+          'u1',
+          'w1',
+          { title: 't', spaceId: 's1' } as any,
+          provenance,
+        );
+        return pageRepo.insertPage.mock.calls[0][0];
+      };
+
+      it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => {
+        const payload = await run(AGENT);
+        expect(payload).toEqual(
+          expect.objectContaining({
+            lastUpdatedSource: 'agent',
+            lastUpdatedAiChatId: 'chat-7',
+          }),
+        );
+      });
+
+      it('omits the source columns for a normal user', async () => {
+        const payload = await run(USER);
+        expect(payload).not.toHaveProperty('lastUpdatedSource');
+        expect(payload).not.toHaveProperty('lastUpdatedAiChatId');
+      });
+    });
+
+    describe('update() → updatePage', () => {
+      const run = async (provenance: any) => {
+        const pageRepo = {
+          updatePage: jest.fn().mockResolvedValue(undefined),
+          findById: jest.fn().mockResolvedValue({ id: 'p1' }),
+        };
+        const svc = makeSvc({ pageRepo, generalQueue: makeGeneralQueue() });
+        const page = {
+          id: 'p1',
+          contributorIds: [],
+          spaceId: 's1',
+          workspaceId: 'w1',
+          slugId: 'sl1',
+          title: 't',
+          parentPageId: null,
+        } as any;
+        // dto carries no content/operation/format → updatePageContent skipped.
+        await svc.update(page, {} as any, { id: 'u1' } as any, provenance);
+        return pageRepo.updatePage.mock.calls[0][0];
+      };
+
+      it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => {
+        const payload = await run(AGENT);
+        expect(payload).toEqual(
+          expect.objectContaining({
+            lastUpdatedSource: 'agent',
+            lastUpdatedAiChatId: 'chat-7',
+          }),
+        );
+      });
+
+      it('omits the source columns for a normal user', async () => {
+        const payload = await run(USER);
+        expect(payload).not.toHaveProperty('lastUpdatedSource');
+        expect(payload).not.toHaveProperty('lastUpdatedAiChatId');
+      });
+    });
+
+    describe('movePage() → updatePage', () => {
+      const VALID_POSITION = 'a0';
+      const run = async (provenance: any) => {
+        const pageRepo = {
+          findById: jest.fn().mockResolvedValue({
+            id: 'dest-parent',
+            deletedAt: null,
+            spaceId: 'space-1',
+          }),
+          updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }),
+        };
+        const svc = makeSvc({
+          pageRepo,
+          db: {} as any,
+        });
+        // Legitimate move: destination ancestors do NOT include the moved page.
+        jest
+          .spyOn(svc, 'getPageBreadCrumbs')
+          .mockResolvedValue([{ id: 'dest-parent' }, { id: 'root' }] as any);
+        // eventEmitter is a bare {} stub; movePage emits PAGE_MOVED, so give it
+        // an emit. Re-wire via the private field to avoid threading it through.
+        (svc as any).eventEmitter = { emit: jest.fn() };
+        const movedPage = {
+          id: 'page-1',
+          parentPageId: 'old-parent',
+          spaceId: 'space-1',
+          workspaceId: 'ws-1',
+          slugId: 'slug-1',
+          title: 'Page 1',
+          icon: null,
+        } as any;
+        const dto = {
+          pageId: 'page-1',
+          position: VALID_POSITION,
+          parentPageId: 'dest-parent',
+        } as any;
+        await svc.movePage(dto, movedPage, provenance);
+        return pageRepo.updatePage.mock.calls[0][0];
+      };
+
+      it('stamps lastUpdatedSource/lastUpdatedAiChatId for an agent', async () => {
+        const payload = await run(AGENT);
+        expect(payload).toEqual(
+          expect.objectContaining({
+            lastUpdatedSource: 'agent',
+            lastUpdatedAiChatId: 'chat-7',
+          }),
+        );
+      });
+
+      it('omits the source columns for a normal user', async () => {
+        const payload = await run(USER);
+        expect(payload).not.toHaveProperty('lastUpdatedSource');
+        expect(payload).not.toHaveProperty('lastUpdatedAiChatId');
+      });
+    });
+
+    describe('movePageToSpace() → root-page updatePage', () => {
+      // movePageToSpace runs its writes inside executeTx(this.db, cb), which
+      // calls this.db.transaction().execute(fn => fn(trx)). A permissive
+      // chainable Proxy stands in for the Kysely trx so arbitrary chains resolve.
+      const makeChain = () => {
+        const c: any = new Proxy(function () {}, {
+          get: (_t, p) =>
+            p === 'then'
+              ? undefined
+              : p === 'execute' || p === 'executeTakeFirst'
+                ? () => Promise.resolve([])
+                : () => c,
+        });
+        return c;
+      };
+
+      const run = async (provenance: any) => {
+        const trxStub = makeChain();
+        const db = {
+          transaction: () => ({ execute: (fn: any) => fn(trxStub) }),
+        } as any;
+        const rootPage = {
+          id: 'root',
+          spaceId: 'src-space',
+          parentPageId: null,
+          workspaceId: 'ws-1',
+        } as any;
+        const pageRepo = {
+          getPageAndDescendants: jest.fn().mockResolvedValue([rootPage]),
+          updatePage: jest.fn().mockResolvedValue(undefined),
+          updatePages: jest.fn().mockResolvedValue(undefined),
+        };
+        const svc = makeSvc({ pageRepo, db });
+        // The single-accessible-page path still runs the bulk side-effect writes
+        // (attachments/watchers/ai-queue) AFTER the root updatePage we assert on;
+        // stub them so the transaction completes without throwing.
+        (svc as any).attachmentRepo = {
+          updateAttachmentsByPageId: jest.fn().mockResolvedValue(undefined),
+        };
+        (svc as any).watcherService = {
+          movePageWatchersToSpace: jest.fn().mockResolvedValue(undefined),
+        };
+        (svc as any).aiQueue = { add: jest.fn().mockResolvedValue(undefined) };
+        // Single accessible page (the root) → pagesToOrphan is empty, so the
+        // root updatePage is the first/only provenance-carrying updatePage call.
+        // filterAccessibleTreePages is private; spy via an `any` cast.
+        jest
+          .spyOn(svc as any, 'filterAccessibleTreePages')
+          .mockResolvedValue([rootPage] as any);
+        jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0' as any);
+        await svc.movePageToSpace(rootPage, 'dst-space', 'u1', provenance);
+        return pageRepo.updatePage.mock.calls[0][0];
+      };
+
+      it('stamps the moved root with the agent source + chat id', async () => {
+        const payload = await run(AGENT);
+        expect(payload).toEqual(
+          expect.objectContaining({
+            spaceId: 'dst-space',
+            lastUpdatedSource: 'agent',
+            lastUpdatedAiChatId: 'chat-7',
+          }),
+        );
+      });
+
+      it('omits the source columns on the moved root for a normal user', async () => {
+        const payload = await run(USER);
+        expect(payload).toEqual(
+          expect.objectContaining({ spaceId: 'dst-space' }),
+        );
+        expect(payload).not.toHaveProperty('lastUpdatedSource');
+        expect(payload).not.toHaveProperty('lastUpdatedAiChatId');
+      });
+    });
+  });
 });

apps/server/src/core/page/services/page.service.ts

@@ -138,7 +138,6 @@ export class PageService {
       ydoc = createYdocFromJson(prosemirrorJson);
     }
 
-
     const page = await this.pageRepo.insertPage({
       slugId: generateSlugId(),
       title: createPageDto.title,
@@ -228,7 +227,6 @@ export class PageService {
     contributors.add(user.id);
     const contributorIds = Array.from(contributors);
 
-
     // Detect a real title/icon change so the WS tree listener can broadcast an
     // `updateOne` to the space (rename / icon swap) WITHOUT re-broadcasting on a
     // content-only save. Only treat a field as changed when the DTO actually
@@ -246,7 +244,8 @@ export class PageService {
         icon: updatePageDto.icon,
         lastUpdatedById: user.id,
         // Agent-edit provenance: annotate the source without changing the
-        // responsible author. A normal user request leaves the column default.
+        // responsible author. A normal user request leaves the existing source
+        // value unchanged.
         ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'),
         updatedAt: new Date(),
         contributorIds: contributorIds,
@@ -934,13 +933,12 @@ export class PageService {
       }
     }
 
-
     const updateResult = await this.pageRepo.updatePage(
       {
         position: dto.position,
         parentPageId: parentPageId,
         // Agent-edit provenance: annotate the source on an agent move. A normal
-        // user request leaves the column default ('user').
+        // user request leaves the existing source value unchanged.
         ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'),
       },
       dto.pageId,

apps/server/src/database/repos/user/user.repo.ts

@@ -36,9 +36,6 @@ export class UserRepo {
     'updatedAt',
     'deletedAt',
     'hasGeneratedPassword',
-    // AI agent identity flag — needed by the JWT strategy to derive a
-    // non-spoofable 'agent' provenance from the signed server-side identity.
-    'isAgent',
   ];
 
   async findById(
@@ -48,6 +45,12 @@ export class UserRepo {
       includePassword?: boolean;
       includeUserMfa?: boolean;
       includeScimExternalId?: boolean;
+      // Opt-in: `isAgent` is internal provenance state, not part of the generic
+      // user payload. Keeping it out of `baseFields` stops it from leaking into
+      // the workspace member list / `/users/me` (an enumeration leak). Only the
+      // JWT + collab auth seams opt in, because they derive a non-spoofable
+      // 'agent' provenance from the signed server-side identity.
+      includeIsAgent?: boolean;
       trx?: KyselyTransaction;
     },
   ): Promise<User> {
@@ -58,6 +61,7 @@ export class UserRepo {
       .$if(opts?.includePassword, (qb) => qb.select('password'))
       .$if(opts?.includeUserMfa, (qb) => qb.select(this.withUserMfa))
       .$if(opts?.includeScimExternalId, (qb) => qb.select('scimExternalId'))
+      .$if(opts?.includeIsAgent, (qb) => qb.select('isAgent'))
       .where('id', '=', userId)
       .where('workspaceId', '=', workspaceId)
       .executeTakeFirst();