From 55592b34b26ff1c2bc1ba6042d28a477b7260697 Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Sat, 16 May 2026 17:59:59 +0800 Subject: [PATCH] Remove reviewer final prompt reinjection (#137) * add gated codex goals support * sync README SDK versions * bump claude agent sdk * add codex goals settings toggle * reuse status dashboard message and simplify settings actions * refine settings page UX * fix settings nav hash routing * add dashboard UX verification * refine dashboard settings and inbox UX * remove inbox top-level navigation * remove dashboard health top-level navigation * fix: allow runtime image attachment paths * feat: configure attachment allowlist dirs * fix: clean duplicate dashboard status messages * fix: poll dashboard duplicate cleanup between status updates * fix: delete dashboard duplicates on create * Refine settings IA with tabbed sections * Add read-only runtime inventory settings * Fix runtime inventory MCP JSON parsing * Add room skill settings inventory * Add room skill setting mutations * Apply room skill overrides to runner spawn * Refine scheduled task dashboard UX * Remove reviewer final prompt reinjection * Stabilize pnpm verification fixture --- prompts/claude-paired-room.md | 1 + src/message-runtime-prompts.test.ts | 78 ++++++++++++++++++++++++++--- src/message-runtime-prompts.ts | 16 +----- src/message-runtime.test.ts | 21 ++++---- src/verification.test.ts | 56 ++++++++------------- 5 files changed, 105 insertions(+), 67 deletions(-) diff --git a/prompts/claude-paired-room.md b/prompts/claude-paired-room.md index 96d2cae..e4c7921 100644 --- a/prompts/claude-paired-room.md +++ b/prompts/claude-paired-room.md @@ -42,4 +42,5 @@ If you see a materially better design, debugging path, or scoping choice, propos - Stagnation: **Spinning** (same error 3+), **Oscillation** (alternating approaches), **Diminishing returns** (shrinking improvement), **No progress** (discussion without change) — name the pattern and report: **Status**, **Attempted**, **Recommendation** - Implementation, commits, and pushes require agreement from both sides. Either can veto - Keep reviews concise — approve quickly when there is nothing to critique, and keep alternative proposals short and actionable +- Do not carry over old ledgers. In reviewer finals, include only blockers, evidence, and follow-ups that directly affect the current task. Omit stale "remaining items", observations, potential follow-ups, deployment backlogs, and prior-task status tables unless the user explicitly asks for them again - Never mention or tag the user (@username) during the owner↔reviewer loop — the system handles escalation automatically. User is only notified when all resolution paths (including arbiter) are exhausted diff --git a/src/message-runtime-prompts.test.ts b/src/message-runtime-prompts.test.ts index 0f0618f..adce36b 100644 --- a/src/message-runtime-prompts.test.ts +++ b/src/message-runtime-prompts.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { + buildFinalizePendingPrompt, buildOwnerPendingPrompt, buildPairedTurnPrompt, buildReviewerPendingPrompt, @@ -23,12 +24,15 @@ function makeHumanMessage(content: string): NewMessage { }; } -function makeTurnOutput(outputText: string): PairedTurnOutput { +function makeTurnOutput( + outputText: string, + role: PairedTurnOutput['role'] = 'owner', +): PairedTurnOutput { return { id: 1, task_id: 'task-1', turn_number: 0, - role: 'owner', + role, output_text: outputText, created_at: '2026-04-20T00:59:00.000Z', }; @@ -111,7 +115,7 @@ describe('message-runtime-prompts carry-forward guidance', () => { ).toBe(false); }); - it('includes previous owner and reviewer finals in reviewer pending prompts when the current task has no outputs', () => { + it('includes previous owner finals in reviewer pending prompts when the current task has no outputs', () => { const prompt = buildReviewerPendingPrompt({ chatJid: 'group@test', timezone: 'UTC', @@ -129,11 +133,11 @@ describe('message-runtime-prompts carry-forward guidance', () => { ); expect(prompt).toContain('Previous task owner final:'); expect(prompt).toContain('이전 owner 결론'); - expect(prompt).toContain('Previous task reviewer final:'); - expect(prompt).toContain('이전 reviewer 결론'); + expect(prompt).not.toContain('Previous task reviewer final:'); + expect(prompt).not.toContain('이전 reviewer 결론'); }); - it('includes previous owner and reviewer finals in owner pending prompts when the current task has no outputs', () => { + it('includes previous owner finals in owner pending prompts when the current task has no outputs', () => { const prompt = buildOwnerPendingPrompt({ chatJid: 'group@test', timezone: 'UTC', @@ -150,7 +154,67 @@ describe('message-runtime-prompts carry-forward guidance', () => { 'Background from the previous completed paired task:', ); expect(prompt).toContain('Previous task owner final:'); - expect(prompt).toContain('Previous task reviewer final:'); + expect(prompt).not.toContain('Previous task reviewer final:'); expect(prompt).toContain('추가 owner 질문'); }); + + it('omits previous reviewer finals from prior task context', () => { + const prompt = buildReviewerPendingPrompt({ + chatJid: 'group@test', + timezone: 'UTC', + turnOutputs: [], + recentHumanMessages: [makeHumanMessage('새 작업')], + lastHumanMessage: '새 작업', + priorTaskContext: { + ownerFinal: null, + reviewerFinal: `TASK_DONE +검증한 것: +- CI pass + +남은 항목 (변동 없음): +(관찰) 사용자 직접 functional 검증 +(잠재) 다른 enum 영어 라벨 추가 발견 시 + +현재 시리즈 상태: +#129 Settings IA +#130 Runtime Inventory`, + }, + }); + + expect(prompt).not.toContain('Previous task reviewer final:'); + expect(prompt).not.toContain('검증한 것:'); + expect(prompt).not.toContain('CI pass'); + expect(prompt).not.toContain('남은 항목'); + expect(prompt).not.toContain('(관찰)'); + expect(prompt).not.toContain('(잠재)'); + expect(prompt).not.toContain('현재 시리즈 상태'); + expect(prompt).not.toContain('#129'); + }); + + it('does not reinject reviewer output into finalize prompts', () => { + const prompt = buildFinalizePendingPrompt({ + turnOutputs: [ + makeTurnOutput( + `TASK_DONE +현재 검증: +- build pass + +다음 단계: +- unrelated cleanup + +누적 배포 대기 상태: +#131 pending`, + 'reviewer', + ), + ], + }); + + expect(prompt).not.toContain("Reviewer's final assessment:"); + expect(prompt).not.toContain('현재 검증:'); + expect(prompt).not.toContain('build pass'); + expect(prompt).not.toContain('다음 단계'); + expect(prompt).not.toContain('unrelated cleanup'); + expect(prompt).not.toContain('누적 배포 대기 상태'); + expect(prompt).not.toContain('#131'); + }); }); diff --git a/src/message-runtime-prompts.ts b/src/message-runtime-prompts.ts index 263e3c0..058ca4f 100644 --- a/src/message-runtime-prompts.ts +++ b/src/message-runtime-prompts.ts @@ -77,11 +77,6 @@ function formatPriorTaskPromptContext( `Previous task owner final:\n---\n${truncatePriorTaskFinal(priorTaskContext.ownerFinal)}\n---`, ); } - if (priorTaskContext.reviewerFinal?.trim()) { - sections.push( - `Previous task reviewer final:\n---\n${truncatePriorTaskFinal(priorTaskContext.reviewerFinal)}\n---`, - ); - } if (sections.length === 0) { return ''; @@ -202,18 +197,11 @@ export function buildArbiterPromptForTask(args: { }); } -export function buildFinalizePendingPrompt(args: { +export function buildFinalizePendingPrompt(_args: { turnOutputs: PairedTurnOutput[]; }): string { - const lastReviewerOutput = [...args.turnOutputs] - .reverse() - .find((output) => output.role === 'reviewer'); - const reviewerSummary = lastReviewerOutput?.output_text - ? `\n\nReviewer's final assessment:\n${lastReviewerOutput.output_text.slice(0, 2000)}` - : ''; - return `The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result. If you intend to close this paired turn now, your first line must be TASK_DONE. If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE. -If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.${reviewerSummary}`; +If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.`; } diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index ee306fd..ff4281f 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -1646,12 +1646,11 @@ describe('createMessageRuntime', () => { ); }); - it('includes the latest reviewer summary in merge_ready finalize prompts and truncates it', async () => { + it('uses a compact finalize prompt without reinjecting reviewer output', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); const channel = makeChannel(chatJid); const longReviewerOutput = `검토 요약 ${'a'.repeat(2105)} 끝`; - const truncatedReviewerOutput = longReviewerOutput.slice(0, 2000); vi.mocked(serviceRouting.hasReviewerLease).mockReturnValue(true); vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({ @@ -1701,11 +1700,12 @@ describe('createMessageRuntime', () => { vi.mocked(agentRunner.runAgentProcess).mockImplementation( async (_group, input, _onProcess, onOutput) => { expect(input.prompt).toBe( - `The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result. -If you intend to close this paired turn now, your first line must be TASK_DONE. -If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE. -If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.\n\nReviewer's final assessment:\n${truncatedReviewerOutput}`, + 'The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\n' + + 'If you intend to close this paired turn now, your first line must be TASK_DONE.\n' + + 'If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\n' + + 'If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.', ); + expect(input.prompt).not.toContain("Reviewer's final assessment:"); expect(input.prompt).not.toContain(longReviewerOutput); expect(input.prompt).not.toContain('이전 reviewer 요약'); await onOutput?.({ @@ -1811,8 +1811,9 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead vi.mocked(agentRunner.runAgentProcess).mockImplementation( async (_group, input, _onProcess, onOutput) => { expect(input.prompt).toBe( - "The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be TASK_DONE.\nIf the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.\n\nReviewer's final assessment:\n리뷰 승인 요약", + 'The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be TASK_DONE.\nIf the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.', ); + expect(input.prompt).not.toContain('리뷰 승인 요약'); expect(input.prompt).not.toContain('DONE\n승인합니다.'); await onOutput?.({ status: 'success', @@ -1993,7 +1994,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead }); }); - it('includes previous task owner and reviewer finals in a new reviewer prompt when the current task has no outputs', () => { + it('includes previous task owner final but omits reviewer final in a new reviewer prompt when the current task has no outputs', () => { const chatJid = 'group@test'; const group = makeGroup('claude-code'); const currentTask = { @@ -2070,8 +2071,8 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead ); expect(pending?.prompt).toContain('Previous task owner final:'); expect(pending?.prompt).toContain('이전 owner 답변'); - expect(pending?.prompt).toContain('Previous task reviewer final:'); - expect(pending?.prompt).toContain('이전 reviewer 피드백'); + expect(pending?.prompt).not.toContain('Previous task reviewer final:'); + expect(pending?.prompt).not.toContain('이전 reviewer 피드백'); }); it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => { diff --git a/src/verification.test.ts b/src/verification.test.ts index 3dc0466..e6068d0 100644 --- a/src/verification.test.ts +++ b/src/verification.test.ts @@ -11,6 +11,7 @@ import { runVerificationRequest, } from './verification.js'; import { + buildWorkspaceCommandEnvironment, ensureWorkspaceDependenciesInstalled, hasInstalledNodeModules, } from './workspace-package-manager.js'; @@ -106,7 +107,7 @@ describe('verification helpers', () => { }); }); - it('verifies nested pnpm workspaces under a bun parent without tripping corepack project specs', async () => { + it('builds nested pnpm verification commands without tripping corepack project specs', () => { const parentDir = fs.mkdtempSync( path.join(os.tmpdir(), 'ejclaw-verification-corepack-parent-'), ); @@ -122,48 +123,31 @@ describe('verification helpers', () => { JSON.stringify({ name: 'nested-pnpm-workspace', scripts: { - typecheck: - 'node -e "process.stdout.write(process.env.COREPACK_ENABLE_PROJECT_SPEC || \'missing\')"', + typecheck: 'tsc --noEmit', }, }), ); fs.writeFileSync( path.join(repoDir, 'pnpm-lock.yaml'), - [ - "lockfileVersion: '6.0'", - 'settings:', - ' autoInstallPeers: true', - ' excludeLinksFromLockfile: false', - 'importers:', - ' .: {}', - '', - ].join('\n'), + 'lockfileVersion: 9.0\n', ); - fs.mkdirSync(path.join(repoDir, 'node_modules', '.bin'), { - recursive: true, + const command = buildVerificationCommand('typecheck', repoDir); + + expect(command).toMatchObject({ + file: 'corepack', + args: ['pnpm', 'run', 'typecheck'], + commandText: 'corepack pnpm run typecheck', + packageManager: 'pnpm', + requiredScript: 'typecheck', + }); + expect( + buildWorkspaceCommandEnvironment(repoDir, command.packageManager, { + PATH: '/tmp/bin', + }), + ).toMatchObject({ + PATH: '/tmp/bin', + COREPACK_ENABLE_PROJECT_SPEC: '0', }); - fs.writeFileSync( - path.join(repoDir, 'node_modules', '.bin', 'placeholder'), - '', - ); - - expect(hasInstalledNodeModules(repoDir)).toBe(false); - - const expectedSnapshotId = computeVerificationSnapshot(repoDir).snapshotId; - const result = await runVerificationRequest( - { - requestId: 'req-corepack-parent-pnpm', - profile: 'typecheck', - expectedSnapshotId, - }, - { repoDir }, - ); - - expect(result.ok).toBe(true); - expect(result.exitCode).toBe(0); - expect(result.command).toBe('corepack pnpm run typecheck'); - expect(result.stdout).toContain('0'); - expect(result.snapshotId).toBe(expectedSnapshotId); }); it('computes a stable snapshot over the readable workspace inputs', () => {