From 86c1307ed2651a951b5c3f34ec300ecf193002f3 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 3 Jul 2026 06:38:25 +0300 Subject: [PATCH] fix(#300 review): drop stray symlink, re-fetch enriched on comment update, cover history mapping (F1/F2/F3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: remove an accidentally-committed self-referential symlink packages/mcp/node_modules/node_modules -> an absolute build-machine path (leaked a dev home path, a pnpm artifact useless in the repo), and add a targeted ignore so it can't recommit. F2: the commentUpdated broadcast re-emitted the caller's pre-loaded comment mutated in place, so the {agent,launcher} stack survived only because the controller happened to load it with includeCreator:true — the fragile coupling that let the stack vanish on edit once already. update() now RE-FETCHES the enriched comment before broadcasting, symmetric with create()/resolveComment() (the row is already persisted), so all three broadcasts carry the stack regardless of any caller's pre-load. Adds a caller-contract test asserting all three broadcasts emit agent/launcher for an agent comment and neither for a non-agent one, spotlighting the update path (non-vacuous vs the old re-emit). F3: add a direct test of the page-history attachPageHistoryAgent mapping (its distinct lastUpdatedSource/lastUpdatedAiChatId/lastUpdatedBy column set): role / no-role / MCP / non-agent, and that the internal agentRole join column is stripped. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...mment.service.provenance-broadcast.spec.ts | 237 ++++++++++++++++++ .../src/core/comment/comment.service.ts | 20 +- .../repos/page/page-history.repo.spec.ts | 107 ++++++++ packages/mcp/.gitignore | 1 + packages/mcp/node_modules/node_modules | 1 - 5 files changed, 360 insertions(+), 6 deletions(-) create mode 100644 apps/server/src/core/comment/comment.service.provenance-broadcast.spec.ts create mode 100644 apps/server/src/database/repos/page/page-history.repo.spec.ts create mode 100644 packages/mcp/.gitignore delete mode 120000 packages/mcp/node_modules/node_modules diff --git a/apps/server/src/core/comment/comment.service.provenance-broadcast.spec.ts b/apps/server/src/core/comment/comment.service.provenance-broadcast.spec.ts new file mode 100644 index 00000000..2d962f1a --- /dev/null +++ b/apps/server/src/core/comment/comment.service.provenance-broadcast.spec.ts @@ -0,0 +1,237 @@ +import { CommentService } from './comment.service'; + +/** + * Caller-contract coverage for the three live comment broadcasts (#300/#304): + * - commentCreated (create @153) + * - commentUpdated (update @214) ← the fragile path this suite spotlights + * - commentResolved (resolveComment @283) + * + * All three must emit a payload carrying the {agent,launcher} avatar stack for an + * AGENT comment, and NEITHER field for a non-agent comment. The enrichment lives + * in CommentRepo.findById(..., {includeCreator:true}); the service contract these + * tests pin is that every broadcast reads its payload from that enriched + * single-row load rather than from an un-enriched object. + * + * NON-VACUITY for the update path: the service is handed an UN-enriched input + * comment (no agent/launcher), while findById returns the ENRICHED shape. The + * pre-#304 update() re-emitted the caller's object in place, so it would emit the + * un-enriched input and the `agent`/`launcher` assertions would FAIL. The fix + * re-fetches via findById, so the broadcast carries the stack regardless of how + * the caller pre-loaded the comment. + */ +describe('CommentService — broadcast carries the agent avatar stack', () => { + // An enriched agent comment as CommentRepo.findById(..., includeCreator:true) + // returns it: the {agent,launcher} pair is attached and agentRole is stripped. + const enrichedAgentComment = (over?: Record) => ({ + id: 'comment-new', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + content: { type: 'doc', content: [] }, + createdSource: 'agent', + agent: { name: 'Researcher', emoji: '🔬', avatarUrl: null }, + launcher: { name: 'Alice', avatarUrl: 'a.png' }, + ...over, + }); + + // A plain human comment: findById attaches neither agent nor launcher. + const plainHumanComment = (over?: Record) => ({ + id: 'comment-new', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + content: { type: 'doc', content: [] }, + createdSource: 'user', + ...over, + }); + + function makeService(findByIdReturn: unknown) { + const commentRepo: any = { + // In these flows findById is only the post-write enriched re-read + // (no parentCommentId is set, so no parent lookup path is taken). + findById: jest.fn(async () => findByIdReturn), + insertComment: jest.fn(async () => ({ id: 'comment-new' })), + updateComment: jest.fn(async () => undefined), + }; + const pageRepo: any = {}; + const wsService: any = { emitCommentEvent: jest.fn() }; + const collaborationGateway: any = { + handleYjsEvent: jest.fn(async () => undefined), + }; + const generalQueue: any = { add: jest.fn(() => Promise.resolve()) }; + const notificationQueue: any = { add: jest.fn(async () => undefined) }; + + const service = new CommentService( + commentRepo, + pageRepo, + wsService, + collaborationGateway, + generalQueue, + notificationQueue, + ); + + return { service, commentRepo, wsService }; + } + + // Pull the emitted event object (3rd arg of emitCommentEvent) for an operation. + const emittedEvent = (wsService: any, operation: string) => + wsService.emitCommentEvent.mock.calls + .map((c: any[]) => c[2]) + .find((e: any) => e.operation === operation); + + const page = { id: 'page-1', spaceId: 'space-1' } as any; + const user = (id = 'user-1') => ({ id }) as any; + const emptyDoc = JSON.stringify({ type: 'doc', content: [] }); + + describe('commentCreated', () => { + it('emits agent + launcher for an agent comment', async () => { + const { service, wsService } = makeService(enrichedAgentComment()); + + await service.create( + { page, workspaceId: 'ws-1', user: user() }, + { content: emptyDoc } as any, + { actor: 'agent', aiChatId: 'chat-1' }, + ); + + const event = emittedEvent(wsService, 'commentCreated'); + expect(event).toBeDefined(); + expect(event.comment.agent).toEqual({ + name: 'Researcher', + emoji: '🔬', + avatarUrl: null, + }); + expect(event.comment.launcher).toEqual({ name: 'Alice', avatarUrl: 'a.png' }); + }); + + it('emits neither field for a non-agent comment', async () => { + const { service, wsService } = makeService(plainHumanComment()); + + await service.create( + { page, workspaceId: 'ws-1', user: user() }, + { content: emptyDoc } as any, + ); + + const event = emittedEvent(wsService, 'commentCreated'); + expect(event).toBeDefined(); + expect(event.comment).not.toHaveProperty('agent'); + expect(event.comment).not.toHaveProperty('launcher'); + }); + }); + + describe('commentUpdated — the fragile path (spotlight)', () => { + it('emits agent + launcher even when the caller pre-loaded an UN-enriched comment', async () => { + // findById (the re-fetch) returns the enriched shape... + const { service, wsService, commentRepo } = makeService( + enrichedAgentComment(), + ); + + // ...but the caller hands in an object with NO agent/launcher. The pre-#304 + // update() re-emitted THIS object in place, so this test fails against it; + // the re-fetch fix makes the broadcast independent of the pre-load. + const inputComment: any = { + id: 'comment-new', + creatorId: 'user-1', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + content: { type: 'doc', content: [] }, + // deliberately no `agent` / `launcher` + }; + + await service.update( + inputComment, + { content: emptyDoc } as any, + user('user-1'), + ); + + // The broadcast must re-read the enriched row (persisted update, then load). + expect(commentRepo.updateComment).toHaveBeenCalled(); + expect(commentRepo.findById).toHaveBeenCalledWith('comment-new', { + includeCreator: true, + includeResolvedBy: true, + }); + + const event = emittedEvent(wsService, 'commentUpdated'); + expect(event).toBeDefined(); + expect(event.comment.agent).toEqual({ + name: 'Researcher', + emoji: '🔬', + avatarUrl: null, + }); + expect(event.comment.launcher).toEqual({ name: 'Alice', avatarUrl: 'a.png' }); + }); + + it('emits neither field for a non-agent comment', async () => { + const { service, wsService } = makeService(plainHumanComment()); + + const inputComment: any = { + id: 'comment-new', + creatorId: 'user-1', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + content: { type: 'doc', content: [] }, + }; + + await service.update( + inputComment, + { content: emptyDoc } as any, + user('user-1'), + ); + + const event = emittedEvent(wsService, 'commentUpdated'); + expect(event).toBeDefined(); + expect(event.comment).not.toHaveProperty('agent'); + expect(event.comment).not.toHaveProperty('launcher'); + }); + }); + + describe('commentResolved', () => { + it('emits agent + launcher for an agent comment', async () => { + const { service, wsService } = makeService(enrichedAgentComment()); + + await service.resolveComment( + { + id: 'comment-new', + creatorId: 'user-1', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + } as any, + true, + user('user-1'), + { actor: 'agent', aiChatId: 'chat-1' }, + ); + + const event = emittedEvent(wsService, 'commentResolved'); + expect(event).toBeDefined(); + expect(event.comment.agent).toEqual({ + name: 'Researcher', + emoji: '🔬', + avatarUrl: null, + }); + expect(event.comment.launcher).toEqual({ name: 'Alice', avatarUrl: 'a.png' }); + }); + + it('emits neither field for a non-agent comment', async () => { + const { service, wsService } = makeService(plainHumanComment()); + + await service.resolveComment( + { + id: 'comment-new', + creatorId: 'user-1', + pageId: 'page-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + } as any, + true, + user('user-1'), + ); + + const event = emittedEvent(wsService, 'commentResolved'); + expect(event).toBeDefined(); + expect(event.comment).not.toHaveProperty('agent'); + expect(event.comment).not.toHaveProperty('launcher'); + }); + }); +}); diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index 579438ef..98244b6a 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -207,17 +207,27 @@ export class CommentService { false, ); - comment.content = commentContent; - comment.editedAt = editedAt; - comment.updatedAt = editedAt; + // Re-fetch the enriched comment before broadcasting, symmetric with + // create()/resolveComment(). updateComment() above has already persisted the + // new content/timestamps, so this single-row read reflects the edit AND + // carries the same {agent,launcher} avatar stack (via includeCreator) as the + // other two broadcasts. This deliberately does NOT reuse the caller's + // pre-loaded `comment`: relying on the controller happening to load it with + // includeCreator:true is exactly the fragile coupling that let the agent + // stack silently vanish on edit once already (#300/#304) — a future caller + // dropping that flag must not regress the broadcast. + const updatedComment = await this.commentRepo.findById(comment.id, { + includeCreator: true, + includeResolvedBy: true, + }); this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { operation: 'commentUpdated', pageId: comment.pageId, - comment, + comment: updatedComment, }); - return comment; + return updatedComment; } async resolveComment( diff --git a/apps/server/src/database/repos/page/page-history.repo.spec.ts b/apps/server/src/database/repos/page/page-history.repo.spec.ts new file mode 100644 index 00000000..c03f8a39 --- /dev/null +++ b/apps/server/src/database/repos/page/page-history.repo.spec.ts @@ -0,0 +1,107 @@ +import { PageHistoryRepo } from './page-history.repo'; + +/** + * Enrichment coverage for the page-history agent avatar stack (#300/#304). + * + * attachPageHistoryAgent maps a DIFFERENT column set than comments — + * `lastUpdatedSource` / `lastUpdatedAiChatId` / `lastUpdatedBy` instead of + * `createdSource` / `aiChatId` / `creator` — so it needs its own direct proof + * that the {agent,launcher} pair resolves for each provenance shape and that the + * internal `agentRole` join column is stripped. + * + * The mapping is exercised through findPageHistoryByPageId (the only page-history + * path that enriches). The Kysely db is a chainable recorder: query-builder + * methods return the builder and `.execute()` (called by + * executeWithCursorPagination) yields preset rows, so no real database is + * touched. The `.select((eb) => ...)` callbacks are recorded but never invoked, + * so the preset row stands in for what the DB would have returned. + * + * NON-VACUITY: against an identity mapping (raw row pass-through) the agent-case + * assertions fail — `agent`/`launcher` would be undefined and the internal + * `agentRole` column would leak. + */ +describe('PageHistoryRepo.findPageHistoryByPageId — agent avatar stack enrichment', () => { + function makeRepo(rows: unknown[]) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const builder: any = { + selectFrom: () => builder, + select: () => builder, + where: () => builder, + orderBy: () => builder, + limit: () => builder, + execute: async () => rows, + }; + const db = { selectFrom: () => builder }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return new PageHistoryRepo(db as any); + } + + // perPage high enough that a single preset row never triggers the extra-row + // "has next page" branch (which would call generateCursor). + const pagination = { limit: 50 } as any; + + const firstItem = async (row: Record) => { + const repo = makeRepo([row]); + const result = await repo.findPageHistoryByPageId('page-1', pagination); + return result.items[0] as any; + }; + + it('internal chat WITH role: agent = role (emoji, no avatar), launcher = human, agentRole stripped', async () => { + const item = await firstItem({ + id: 'ph-1', + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-1', + lastUpdatedBy: { name: 'Alice', avatarUrl: 'a.png' }, + agentRole: { name: 'Editor', emoji: '✏️' }, + }); + + expect(item.agent).toEqual({ name: 'Editor', emoji: '✏️', avatarUrl: null }); + expect(item.launcher).toEqual({ name: 'Alice', avatarUrl: 'a.png' }); + // The internal join column must never leak to the client. + expect(item).not.toHaveProperty('agentRole'); + }); + + it('internal chat WITHOUT role: agent = "AI agent" fallback, launcher = human', async () => { + const item = await firstItem({ + id: 'ph-2', + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: 'chat-1', + lastUpdatedBy: { name: 'Alice', avatarUrl: 'a.png' }, + agentRole: null, + }); + + expect(item.agent).toEqual({ name: 'AI agent', avatarUrl: null }); + expect(item.agent).not.toHaveProperty('emoji'); + expect(item.launcher).toEqual({ name: 'Alice', avatarUrl: 'a.png' }); + expect(item).not.toHaveProperty('agentRole'); + }); + + it('external MCP (lastUpdatedAiChatId null): agent = the account itself, launcher = null', async () => { + const item = await firstItem({ + id: 'ph-3', + lastUpdatedSource: 'agent', + lastUpdatedAiChatId: null, + lastUpdatedBy: { name: 'MCP Bot', avatarUrl: 'bot.png' }, + agentRole: null, + }); + + expect(item.agent).toEqual({ name: 'MCP Bot', avatarUrl: 'bot.png' }); + expect(item.launcher).toBeNull(); + expect(item).not.toHaveProperty('agentRole'); + }); + + it('non-agent (lastUpdatedSource !== "agent"): neither agent nor launcher, agentRole stripped', async () => { + const item = await firstItem({ + id: 'ph-4', + lastUpdatedSource: 'user', + lastUpdatedAiChatId: null, + lastUpdatedBy: { name: 'Bob', avatarUrl: null }, + agentRole: null, + }); + + expect(item).not.toHaveProperty('agent'); + expect(item).not.toHaveProperty('launcher'); + // A plain human row still strips the internal join column. + expect(item).not.toHaveProperty('agentRole'); + }); +}); diff --git a/packages/mcp/.gitignore b/packages/mcp/.gitignore new file mode 100644 index 00000000..fe6414e3 --- /dev/null +++ b/packages/mcp/.gitignore @@ -0,0 +1 @@ +node_modules/node_modules diff --git a/packages/mcp/node_modules/node_modules b/packages/mcp/node_modules/node_modules deleted file mode 120000 index b7c5fb79..00000000 --- a/packages/mcp/node_modules/node_modules +++ /dev/null @@ -1 +0,0 @@ -/home/claude/gitmost/packages/mcp/node_modules \ No newline at end of file