diff --git a/packages/pulse/src/e2e/council-v2-live.ts b/packages/pulse/src/e2e/council-v2-live.ts index e29225e..ad99dd9 100644 --- a/packages/pulse/src/e2e/council-v2-live.ts +++ b/packages/pulse/src/e2e/council-v2-live.ts @@ -69,17 +69,32 @@ async function architectFn(chain: WorkflowMessage[]) { }, ], }); - let parsed: { analysis?: string; targetFiles?: string[] }; + let parsed: { + analysis?: string; + targetFiles?: string[]; + changes?: Record; + verification?: string; + }; try { parsed = JSON.parse(resp.content ?? '{}'); } catch { - parsed = { analysis: resp.content ?? 'No analysis', targetFiles: [] }; + parsed = { analysis: resp.content ?? 'No analysis' }; } console.log(` 🏗️ analysis: ${(parsed.analysis ?? '').slice(0, 120)}...`); + const targetFiles = parsed.targetFiles ?? []; + const changes: Record = parsed.changes ?? {}; + if (Object.keys(changes).length === 0) { + for (const f of targetFiles) changes[f] = 'Update this file'; + } + return { content: parsed.analysis ?? 'No analysis', - meta: { targetFiles: parsed.targetFiles ?? [] }, + meta: { + targetFiles, + changes, + verification: parsed.verification ?? 'Run tests and verify manually', + }, }; } diff --git a/packages/pulse/src/workflows/coding.test.ts b/packages/pulse/src/workflows/coding.test.ts index b42f09b..af184d6 100644 --- a/packages/pulse/src/workflows/coding.test.ts +++ b/packages/pulse/src/workflows/coding.test.ts @@ -76,18 +76,23 @@ describe('CodingTask WorkflowType', () => { expect(r5.executed).toEqual([]); }); - it('rejection triggers re-coding', async () => { + it('rejection triggers re-coding with rejectionReason', async () => { setup(); let reviewCount = 0; const codingTask = createCodingWorkflow({ - reviewerFn: async () => { + reviewerFn: async (chain) => { + const prevReviews = chain.filter((m) => m.role === 'reviewer'); reviewCount++; const verdict = reviewCount === 1 ? ('rejected' as const) : ('approved' as const); return { content: reviewCount === 1 ? 'Needs fixes' : 'Looks good', - meta: { verdict }, + meta: { + verdict, + rejectionReason: reviewCount === 1 ? 'Missing error handling' : '', + retryCount: prevReviews.length, + }, }; }, }); @@ -97,12 +102,19 @@ describe('CodingTask WorkflowType', () => { await rule.tick(); // architect await rule.tick(); // coder - await rule.tick(); // reviewer (rejects) + await rule.tick(); // reviewer (rejects, retryCount=0) + + const reviewerEvents = store.queryByKind('coding.reviewer'); + expect(reviewerEvents.length).toBe(1); + const meta = JSON.parse(reviewerEvents[0].meta!); + expect(meta.verdict).toBe('rejected'); + expect(meta.rejectionReason).toBe('Missing error handling'); + expect(meta.retryCount).toBe(0); const r4 = await rule.tick(); expect(r4.executed).toMatchObject([{ topicId: 'task-2', role: 'coder' }]); - await rule.tick(); // reviewer (approves) + await rule.tick(); // reviewer (approves, retryCount=1) const r6 = await rule.tick(); expect(r6.executed).toMatchObject([{ topicId: 'task-2', role: 'closer' }]); @@ -110,6 +122,46 @@ describe('CodingTask WorkflowType', () => { expect(r7.executed).toEqual([]); }); + it('retryCount >= 3 forces transition to closer even if rejected', async () => { + setup(); + + let reviewRound = 0; + const codingTask = createCodingWorkflow({ + reviewerFn: async (chain) => { + const prevReviews = chain.filter((m) => m.role === 'reviewer'); + reviewRound++; + return { + content: `[mock] Rejection round ${reviewRound}`, + meta: { + verdict: 'rejected' as const, + rejectionReason: `Issue #${reviewRound}`, + retryCount: prevReviews.length, + }, + }; + }, + }); + const rule = createWorkflowRule(codingTask, store); + + triggerCoding('task-retry', 'Retry limit test', 'Test max 3 retries', '/tmp/repo'); + + await rule.tick(); // architect + await rule.tick(); // coder (round 1) + await rule.tick(); // reviewer rejects, retryCount=0 + await rule.tick(); // coder (round 2) + await rule.tick(); // reviewer rejects, retryCount=1 + await rule.tick(); // coder (round 3) + await rule.tick(); // reviewer rejects, retryCount=2 + + await rule.tick(); // coder (round 4 — last retry) + await rule.tick(); // reviewer rejects, retryCount=3 → forces closer + + const r = await rule.tick(); + expect(r.executed).toMatchObject([{ topicId: 'task-retry', role: 'closer' }]); + + const rEnd = await rule.tick(); + expect(rEnd.executed).toEqual([]); + }); + it('adapter writes events, not roles (CAS content is string)', async () => { setup(); const codingTask = createCodingWorkflow(); diff --git a/packages/pulse/src/workflows/coding.ts b/packages/pulse/src/workflows/coding.ts index b1138d5..78967fb 100644 --- a/packages/pulse/src/workflows/coding.ts +++ b/packages/pulse/src/workflows/coding.ts @@ -18,9 +18,17 @@ import { // ── Role Meta types ──────────────────────────────────────────── -export type ArchitectMeta = { targetFiles: string[] }; +export type ArchitectMeta = { + targetFiles: string[]; + changes: Record; + verification: string; +}; export type CoderMeta = { filesChanged: string[]; testsPassed: boolean }; -export type ReviewerMeta = { verdict: 'approved' | 'rejected' }; +export type ReviewerMeta = { + verdict: 'approved' | 'rejected'; + rejectionReason: string; + retryCount: number; +}; export type CloserMeta = null; // ── Roles record ─────────────────────────────────────────────── @@ -40,7 +48,14 @@ const defaultArchitect: Role = async (chain) => { const targetFiles = ['src/main.ts', 'src/utils.ts']; return { content: `[mock] Analysis for "${title}": ${startMsg?.content ?? ''}`, - meta: { targetFiles }, + meta: { + targetFiles, + changes: { + 'src/main.ts': 'Update main entry point', + 'src/utils.ts': 'Add utility helpers', + }, + verification: 'Run `bun test` and verify all tests pass', + }, }; }; @@ -55,10 +70,12 @@ const defaultCoder: Role = async (chain) => { }; }; -const defaultReviewer: Role = async () => { +const defaultReviewer: Role = async (chain) => { + const reviewerMsgs = chain.filter((m) => m.role === 'reviewer'); + const retryCount = reviewerMsgs.length; return { content: '[mock] Code looks good', - meta: { verdict: 'approved' }, + meta: { verdict: 'approved', rejectionReason: '', retryCount }, }; }; @@ -85,8 +102,11 @@ function codingModerator( return 'coder'; case 'coder': return 'reviewer'; - case 'reviewer': - return output.meta?.verdict === 'rejected' ? 'coder' : 'closer'; + case 'reviewer': { + const rejected = output.meta?.verdict === 'rejected'; + const retryCount = output.meta?.retryCount ?? 0; + return rejected && retryCount < 3 ? 'coder' : 'closer'; + } case 'closer': return END; } diff --git a/packages/pulse/src/workflows/roles/architect-llm.test.ts b/packages/pulse/src/workflows/roles/architect-llm.test.ts index bfca364..68c1c8e 100644 --- a/packages/pulse/src/workflows/roles/architect-llm.test.ts +++ b/packages/pulse/src/workflows/roles/architect-llm.test.ts @@ -25,18 +25,19 @@ describe('createArchitectRole', () => { ]; } - it('calls LLM and returns { content, meta } with targetFiles', async () => { + it('calls LLM and returns { content, meta } with targetFiles, changes, verification', async () => { const mockLlm: LlmClient = { chat: async () => ({ content: JSON.stringify({ analysis: 'Need to update main.ts', targetFiles: ['src/main.ts'], + changes: { 'src/main.ts': 'Fix the entry point' }, + verification: 'Run bun test', }), }), }; const role = createArchitectRole(mockLlm); - // store param is still in signature but pure role shouldn't need it const result = await role( makeChain('Fix the bug', 'There is a bug in main.ts', '/tmp/repo'), 'test-1', @@ -44,7 +45,11 @@ describe('createArchitectRole', () => { ); expect(result.content).toBe('Need to update main.ts'); - expect(result.meta).toEqual({ targetFiles: ['src/main.ts'] }); + expect(result.meta).toEqual({ + targetFiles: ['src/main.ts'], + changes: { 'src/main.ts': 'Fix the entry point' }, + verification: 'Run bun test', + }); }); it('handles non-JSON LLM response gracefully', async () => { @@ -60,6 +65,10 @@ describe('createArchitectRole', () => { ); expect(result.content).toBe('Just do it'); - expect(result.meta).toEqual({ targetFiles: [] }); + expect(result.meta).toEqual({ + targetFiles: [], + changes: {}, + verification: 'Run tests and verify manually', + }); }); }); diff --git a/packages/pulse/src/workflows/roles/architect-llm.ts b/packages/pulse/src/workflows/roles/architect-llm.ts index 32907b8..16780d4 100644 --- a/packages/pulse/src/workflows/roles/architect-llm.ts +++ b/packages/pulse/src/workflows/roles/architect-llm.ts @@ -1,7 +1,9 @@ /** - * Architect role — LLM analyzes coding task and suggests approach. + * Architect role — LLM analyzes coding task and outputs structured design. * Built on createLlmRole factory (shared sandwich pattern). * + * Outputs: targetFiles, changes (file → description), verification criteria. + * * 小橘 🍊 (NEKO Team) */ @@ -12,8 +14,14 @@ import { createLlmRole } from './llm-role-factory.js'; export function createArchitectRole(llmClient: LlmClient): Role { return createLlmRole(llmClient, { - systemPrompt: - 'You are a software architect. Analyze the task and suggest target files and approach. Reply in JSON: {"analysis": "...", "targetFiles": ["..."]}', + systemPrompt: `You are a software architect. Analyze the task and produce a structured design. +Reply in JSON with these exact fields: +{ + "analysis": "high-level summary", + "targetFiles": ["file1.ts", "file2.ts"], + "changes": { "file1.ts": "description of what to change", "file2.ts": "..." }, + "verification": "how to verify the changes are correct (e.g. test commands, manual checks)" +}`, buildUserMessage: (chain) => { const startMsg = chain.find((m) => m.role === '__start__'); const title = startMsg?.meta?.title ?? 'unknown'; @@ -22,15 +30,29 @@ export function createArchitectRole(llmClient: LlmClient): Role { return `Task: ${title}\nDescription: ${description}\nRepo: ${repoDir}`; }, parseResponse: (resp) => { - let parsed: { analysis?: string; targetFiles?: string[] }; + let parsed: { + analysis?: string; + targetFiles?: string[]; + changes?: Record; + verification?: string; + }; try { parsed = JSON.parse(resp.content ?? '{}'); } catch { - parsed = { analysis: resp.content ?? 'No analysis', targetFiles: [] }; + parsed = { analysis: resp.content ?? 'No analysis' }; + } + const targetFiles = parsed.targetFiles ?? []; + const changes: Record = parsed.changes ?? {}; + if (Object.keys(changes).length === 0) { + for (const f of targetFiles) changes[f] = 'Update this file'; } return { content: parsed.analysis ?? 'No analysis', - meta: { targetFiles: parsed.targetFiles ?? [] }, + meta: { + targetFiles, + changes, + verification: parsed.verification ?? 'Run tests and verify manually', + }, }; }, }); diff --git a/packages/pulse/src/workflows/roles/coder-cursor.ts b/packages/pulse/src/workflows/roles/coder-cursor.ts index 0f87f39..8821e5c 100644 --- a/packages/pulse/src/workflows/roles/coder-cursor.ts +++ b/packages/pulse/src/workflows/roles/coder-cursor.ts @@ -31,6 +31,21 @@ export function createCoderRole(opts: { const description = startMsg?.content ?? ''; const repoDir = (startMsg?.meta?.repoDir as string) ?? '/tmp'; const architectMsg = chain.find((m) => m.role === 'architect'); + const lastReviewerMsg = [...chain].reverse().find((m) => m.role === 'reviewer'); + + const changes = (architectMsg?.meta?.changes as Record) ?? {}; + const verification = (architectMsg?.meta?.verification as string) ?? ''; + const targetFiles = (architectMsg?.meta?.targetFiles as string[]) ?? []; + + const changesSection = Object.entries(changes) + .map(([file, desc]) => `- **${file}**: ${desc}`) + .join('\n'); + + let rejectionSection = ''; + if (lastReviewerMsg?.meta?.verdict === 'rejected') { + const reason = (lastReviewerMsg.meta.rejectionReason as string) ?? 'No reason given'; + rejectionSection = `\n## Previous Review Rejection\n${reason}\n\nFix the issues above before proceeding.\n`; + } const prompt = `## Task: ${title} @@ -40,11 +55,17 @@ ${description} ${architectMsg?.content ?? 'None'} ## Target Files -${((architectMsg?.meta?.targetFiles as string[]) ?? []).join(', ') || 'Not specified'} +${targetFiles.join(', ') || 'Not specified'} +## Per-File Changes +${changesSection || 'Not specified'} + +## Verification Criteria +${verification || 'Not specified'} +${rejectionSection} ## Instructions -Implement the changes. Do NOT modify any existing test files. -Run tests if applicable. Commit your changes.`; +Implement the changes as described above. Do NOT modify any existing test files. +Run the verification criteria to confirm correctness. Commit your changes.`; return { prompt, cwd: repoDir }; }, diff --git a/packages/pulse/src/workflows/roles/reviewer-cursor.ts b/packages/pulse/src/workflows/roles/reviewer-cursor.ts index 493d028..c1af955 100644 --- a/packages/pulse/src/workflows/roles/reviewer-cursor.ts +++ b/packages/pulse/src/workflows/roles/reviewer-cursor.ts @@ -1,6 +1,9 @@ /** * Reviewer role — LLM-Agent-LLM sandwich via agent executor. * + * Compares coder's actual changes against architect's design (changes + verification). + * Outputs verdict, rejectionReason (when rejected), and monotonic retryCount. + * * 小橘 🍊 (NEKO Team) */ @@ -24,32 +27,52 @@ export function createReviewerRole(opts: { agentBin: opts.agentBin ?? `${process.env.HOME}/.local/bin/agent`, }); - return createAgentExecutorRole(agent, opts.llm, { + const innerRole = createAgentExecutorRole(agent, opts.llm, { prepPrompt: (chain, topicId) => { const startMsg = chain.find((m) => m.role === '__start__'); const title = startMsg?.meta?.title ?? topicId; const repoDir = (startMsg?.meta?.repoDir as string) ?? '/tmp'; + const architectMsg = chain.find((m) => m.role === 'architect'); const coderMsg = [...chain].reverse().find((m) => m.role === 'coder'); + const reviewerMsgs = chain.filter((m) => m.role === 'reviewer'); + const retryCount = reviewerMsgs.length; - const prompt = `## Code Review: ${title} + const architectChanges = (architectMsg?.meta?.changes as Record) ?? {}; + const architectVerification = (architectMsg?.meta?.verification as string) ?? ''; + const changesSpec = Object.entries(architectChanges) + .map(([file, desc]) => `- **${file}**: ${desc}`) + .join('\n'); -## What was done + const prompt = `## Code Review: ${title} (review round ${retryCount}) + +## Architect's Design Specification +### Expected Changes +${changesSpec || 'Not specified'} + +### Verification Criteria +${architectVerification || 'Not specified'} + +## What the Coder Actually Did ${coderMsg?.content ?? 'Unknown'} -## Files changed +## Files Changed by Coder ${((coderMsg?.meta?.filesChanged as string[]) ?? []).join(', ')} ## Instructions -Review the recent changes for correctness, security, and code quality. +1. Compare the coder's actual changes against the architect's expected changes above. +2. Verify the coder addressed each file and change described in the spec. +3. Check if the verification criteria can be satisfied. +4. Review for correctness, security, and code quality. Do NOT modify any files. Only output your review. -End with a clear verdict: APPROVED or REJECTED with reasons.`; +End with a clear verdict: APPROVED or REJECTED. +If REJECTED, explain specifically what is wrong or missing.`; return { prompt, cwd: repoDir }; }, parseMeta: { system: - 'Extract the review verdict from this code review report. Call the extract_review_verdict tool.', + 'Extract the review verdict and rejection reason from this code review report. Call the extract_review_verdict tool.', tool: { type: 'function', function: { @@ -63,8 +86,12 @@ End with a clear verdict: APPROVED or REJECTED with reasons.`; enum: ['approved', 'rejected'], description: 'Final review verdict', }, + rejectionReason: { + type: 'string', + description: 'Reason for rejection (empty string if approved)', + }, }, - required: ['verdict'], + required: ['verdict', 'rejectionReason'], }, }, }, @@ -72,17 +99,29 @@ End with a clear verdict: APPROVED or REJECTED with reasons.`; const parsed = JSON.parse(args); return { verdict: parsed.verdict === 'rejected' ? 'rejected' : 'approved', + rejectionReason: parsed.rejectionReason ?? '', + retryCount: 0, }; }, defaultMeta: (output) => { - // Last resort: keyword matching on the last 200 chars const tail = output.toLowerCase().slice(-200); return { verdict: tail.includes('reject') ? ('rejected' as const) : ('approved' as const), + rejectionReason: tail.includes('reject') ? output.slice(-500) : '', + retryCount: 0, }; }, }, }); + + return async (chain, topicId, store) => { + const result = await innerRole(chain, topicId, store); + const retryCount = chain.filter((m) => m.role === 'reviewer').length; + return { + ...result, + meta: result.meta ? { ...result.meta, retryCount } : result.meta, + }; + }; }