From 30860dd8ea2507e60da15ae339e0da2e7bdf95d1 Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 19:45:15 +0200 Subject: [PATCH] fix(update_idea_plan_reviewed): nooit stilzwijgend goedkeuren (IDEA-066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit De status-logica sprak z'n eigen tool-beschrijving tegen. De code deed: approved -> PLAN_REVIEWED rejected -> PLAN_REVIEW_FAILED else -> PLAN_REVIEWED // "Default to approved if not specified" Een review die 'pending' (needs manual approval) of helemaal geen approval_status teruggaf, markeerde het idee dus als PLAN_REVIEWED (goedgekeurd) — precies omgekeerd aan wat de beschrijving belooft. Fix: alleen een expliciete approval_status='approved' brengt het idee naar PLAN_REVIEWED; 'rejected', 'pending' én een weggelaten approval_status gaan allemaal naar PLAN_REVIEW_FAILED (mens beslist). Nooit stilzwijgend goedkeuren. Verder: - Handler geextraheerd naar handleUpdateIdeaPlanReviewed + inputSchema geexporteerd, conform het create-sprint/update-sprint-patroon, zodat de logica zonder McpServer-wrapper testbaar is. - Tool-beschrijving + header-comment aangescherpt zodat code en docs niet meer divergeren. - Nieuw test-bestand: 6 tests (approved/rejected/pending/omitted status-transitie, not-found, log-persistentie). Build groen, 379 tests groen. Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/update-idea-plan-reviewed.test.ts | 140 ++++++++++++++++++++ src/tools/update-idea-plan-reviewed.ts | 118 +++++++++-------- 2 files changed, 204 insertions(+), 54 deletions(-) create mode 100644 __tests__/update-idea-plan-reviewed.test.ts diff --git a/__tests__/update-idea-plan-reviewed.test.ts b/__tests__/update-idea-plan-reviewed.test.ts new file mode 100644 index 0000000..257fce4 --- /dev/null +++ b/__tests__/update-idea-plan-reviewed.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../src/prisma.js', () => ({ + prisma: { + idea: { update: vi.fn() }, + ideaLog: { create: vi.fn() }, + $transaction: vi.fn(), + }, +})) + +vi.mock('../src/auth.js', () => ({ + requireWriteAccess: vi.fn(), + PermissionDeniedError: class PermissionDeniedError extends Error { + constructor(message = 'Demo accounts cannot perform write operations') { + super(message) + this.name = 'PermissionDeniedError' + } + }, +})) + +vi.mock('../src/access.js', () => ({ + userOwnsIdea: vi.fn(), +})) + +import { prisma } from '../src/prisma.js' +import { requireWriteAccess } from '../src/auth.js' +import { userOwnsIdea } from '../src/access.js' +import { handleUpdateIdeaPlanReviewed } from '../src/tools/update-idea-plan-reviewed.js' + +const mockPrisma = prisma as unknown as { + idea: { update: ReturnType } + ideaLog: { create: ReturnType } + $transaction: ReturnType +} +const mockRequireWriteAccess = requireWriteAccess as ReturnType +const mockUserOwnsIdea = userOwnsIdea as ReturnType + +const IDEA_ID = 'idea-1' +const USER_ID = 'user-1' +const REVIEW_LOG = { + rounds: [{ score: 88 }], + convergence: { stable_at_round: 2 }, + approval: { status: 'approved' }, +} + +beforeEach(() => { + vi.clearAllMocks() + mockRequireWriteAccess.mockResolvedValue({ + userId: USER_ID, + tokenId: 'tok-1', + username: 'alice', + isDemo: false, + }) + mockUserOwnsIdea.mockResolvedValue(true) + // $transaction returns the array of its two operations' results; the handler + // only reads result[0] (the idea.update result). + mockPrisma.$transaction.mockImplementation(async () => [ + { id: IDEA_ID, status: 'PLACEHOLDER', code: 'IDEA-1' }, + {}, + ]) +}) + +function parseResult(result: Awaited>) { + const text = result.content?.[0]?.type === 'text' ? result.content[0].text : '' + try { + return JSON.parse(text) + } catch { + return text + } +} + +// The handler builds `data.status` inside the idea.update call passed to +// $transaction. We capture it by inspecting the prisma.idea.update mock args. +function statusPassedToUpdate(): string | undefined { + const call = mockPrisma.idea.update.mock.calls[0] + return call?.[0]?.data?.status +} + +describe('handleUpdateIdeaPlanReviewed — status transition', () => { + it('approval_status="approved" → PLAN_REVIEWED', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEWED') + }) + + it('approval_status="rejected" → PLAN_REVIEW_FAILED', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'rejected', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('approval_status="pending" → PLAN_REVIEW_FAILED (needs manual approval, never silently approved)', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'pending', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('omitted approval_status → PLAN_REVIEW_FAILED (safe default, not PLAN_REVIEWED)', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('returns "Idea not found" when the user does not own the idea', async () => { + mockUserOwnsIdea.mockResolvedValue(false) + const result = await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + expect(parseResult(result)).toContain('Idea not found') + expect(mockPrisma.idea.update).not.toHaveBeenCalled() + }) + + it('persists review_log + reviewed_at and logs a PLAN_REVIEW_RESULT entry', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + const updateArg = mockPrisma.idea.update.mock.calls[0]?.[0] + expect(updateArg?.data?.plan_review_log).toEqual(REVIEW_LOG) + expect(updateArg?.data?.reviewed_at).toBeInstanceOf(Date) + + const logArg = mockPrisma.ideaLog.create.mock.calls[0]?.[0] + expect(logArg?.data?.type).toBe('PLAN_REVIEW_RESULT') + expect(logArg?.data?.idea_id).toBe(IDEA_ID) + }) +}) diff --git a/src/tools/update-idea-plan-reviewed.ts b/src/tools/update-idea-plan-reviewed.ts index 0217c22..2e9f1ac 100644 --- a/src/tools/update-idea-plan-reviewed.ts +++ b/src/tools/update-idea-plan-reviewed.ts @@ -1,6 +1,8 @@ -// MCP-tool: writes the review-log result after a IDEA_REVIEW_PLAN grill-job -// and transitions the idea.status to PLAN_REVIEWED (on success) or -// PLAN_REVIEW_FAILED (on failure). +// MCP-tool: writes the review-log result after an IDEA_REVIEW_PLAN job and +// transitions idea.status. Only an explicit approval_status='approved' moves +// the idea to PLAN_REVIEWED; anything else (rejected, pending, or omitted) +// goes to PLAN_REVIEW_FAILED — a human must then decide. The tool never +// silently approves. // // Called by the worker as the final step of a review-plan session. @@ -12,7 +14,7 @@ import { requireWriteAccess } from '../auth.js' import { userOwnsIdea } from '../access.js' import { toolError, toolJson, withToolErrors } from '../errors.js' -const inputSchema = z.object({ +export const inputSchema = z.object({ idea_id: z.string().min(1), review_log: z.object({}).passthrough(), // Full ReviewLog from orchestrator (JSON object) approval_status: z @@ -20,64 +22,72 @@ const inputSchema = z.object({ .optional(), }) +export async function handleUpdateIdeaPlanReviewed( + { idea_id, review_log, approval_status }: z.infer, +) { + return withToolErrors(async () => { + const auth = await requireWriteAccess() + if (!(await userOwnsIdea(idea_id, auth.userId))) { + return toolError('Idea not found') + } + + // Alleen een expliciete 'approved' brengt het idee naar PLAN_REVIEWED. + // 'rejected', 'pending' én een weggelaten approval_status betekenen + // allemaal "niet auto-goedgekeurd — mens moet beslissen" en gaan naar + // PLAN_REVIEW_FAILED. Nooit stilzwijgend goedkeuren (de vorige + // `: 'PLAN_REVIEWED'`-default deed dat wel bij pending/undefined). + const nextStatus = + approval_status === 'approved' ? 'PLAN_REVIEWED' : 'PLAN_REVIEW_FAILED' + + // Log summary metrics from review_log + const logSummary = buildReviewLogSummary(review_log) + + const result = await prisma.$transaction([ + prisma.idea.update({ + where: { id: idea_id }, + data: { + plan_review_log: review_log as any, + reviewed_at: new Date(), + status: nextStatus, + }, + select: { id: true, status: true, code: true }, + }), + prisma.ideaLog.create({ + data: { + idea_id, + type: 'PLAN_REVIEW_RESULT', + content: logSummary.summary, + metadata: { + approval_status, + convergence_status: logSummary.convergence_status, + final_score: logSummary.final_score, + rounds_completed: logSummary.rounds_completed, + }, + }, + }), + ]) + + return toolJson({ + ok: true, + idea: result[0], + review_log_summary: logSummary, + }) + }) +} + export function registerUpdateIdeaPlanReviewedTool(server: McpServer) { server.registerTool( 'update_idea_plan_reviewed', { title: 'Mark plan as reviewed', description: - 'Save review-log after plan review cycle and transition idea.status to PLAN_REVIEWED (if approved) or PLAN_REVIEW_FAILED (if rejected/pending requires manual approval). Forbidden for demo accounts.', + 'Save review-log after a plan review cycle and transition idea.status. ' + + 'Only approval_status="approved" → PLAN_REVIEWED; "rejected", "pending", ' + + 'or an omitted approval_status → PLAN_REVIEW_FAILED (needs manual ' + + 'approval — never silently approved). Forbidden for demo accounts.', inputSchema, }, - async ({ idea_id, review_log, approval_status }) => - withToolErrors(async () => { - const auth = await requireWriteAccess() - if (!(await userOwnsIdea(idea_id, auth.userId))) { - return toolError('Idea not found') - } - - // Determine target status based on approval - const nextStatus = - approval_status === 'approved' - ? 'PLAN_REVIEWED' - : approval_status === 'rejected' - ? 'PLAN_REVIEW_FAILED' - : 'PLAN_REVIEWED' // Default to approved if not specified - - // Log summary metrics from review_log - const logSummary = buildReviewLogSummary(review_log) - - const result = await prisma.$transaction([ - prisma.idea.update({ - where: { id: idea_id }, - data: { - plan_review_log: review_log as any, - reviewed_at: new Date(), - status: nextStatus, - }, - select: { id: true, status: true, code: true }, - }), - prisma.ideaLog.create({ - data: { - idea_id, - type: 'PLAN_REVIEW_RESULT', - content: logSummary.summary, - metadata: { - approval_status, - convergence_status: logSummary.convergence_status, - final_score: logSummary.final_score, - rounds_completed: logSummary.rounds_completed, - }, - }, - }), - ]) - - return toolJson({ - ok: true, - idea: result[0], - review_log_summary: logSummary, - }) - }), + handleUpdateIdeaPlanReviewed, ) }