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
This commit is contained in:
@@ -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**
|
- 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
|
- 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
|
- 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
|
- 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
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import { describe, expect, it } from 'vitest';
|
import { describe, expect, it } from 'vitest';
|
||||||
|
|
||||||
import {
|
import {
|
||||||
|
buildFinalizePendingPrompt,
|
||||||
buildOwnerPendingPrompt,
|
buildOwnerPendingPrompt,
|
||||||
buildPairedTurnPrompt,
|
buildPairedTurnPrompt,
|
||||||
buildReviewerPendingPrompt,
|
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 {
|
return {
|
||||||
id: 1,
|
id: 1,
|
||||||
task_id: 'task-1',
|
task_id: 'task-1',
|
||||||
turn_number: 0,
|
turn_number: 0,
|
||||||
role: 'owner',
|
role,
|
||||||
output_text: outputText,
|
output_text: outputText,
|
||||||
created_at: '2026-04-20T00:59:00.000Z',
|
created_at: '2026-04-20T00:59:00.000Z',
|
||||||
};
|
};
|
||||||
@@ -111,7 +115,7 @@ describe('message-runtime-prompts carry-forward guidance', () => {
|
|||||||
).toBe(false);
|
).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({
|
const prompt = buildReviewerPendingPrompt({
|
||||||
chatJid: 'group@test',
|
chatJid: 'group@test',
|
||||||
timezone: 'UTC',
|
timezone: 'UTC',
|
||||||
@@ -129,11 +133,11 @@ describe('message-runtime-prompts carry-forward guidance', () => {
|
|||||||
);
|
);
|
||||||
expect(prompt).toContain('Previous task owner final:');
|
expect(prompt).toContain('Previous task owner final:');
|
||||||
expect(prompt).toContain('이전 owner 결론');
|
expect(prompt).toContain('이전 owner 결론');
|
||||||
expect(prompt).toContain('Previous task reviewer final:');
|
expect(prompt).not.toContain('Previous task reviewer final:');
|
||||||
expect(prompt).toContain('이전 reviewer 결론');
|
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({
|
const prompt = buildOwnerPendingPrompt({
|
||||||
chatJid: 'group@test',
|
chatJid: 'group@test',
|
||||||
timezone: 'UTC',
|
timezone: 'UTC',
|
||||||
@@ -150,7 +154,67 @@ describe('message-runtime-prompts carry-forward guidance', () => {
|
|||||||
'Background from the previous completed paired task:',
|
'Background from the previous completed paired task:',
|
||||||
);
|
);
|
||||||
expect(prompt).toContain('Previous task owner final:');
|
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 질문');
|
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');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -77,11 +77,6 @@ function formatPriorTaskPromptContext(
|
|||||||
`Previous task owner final:\n---\n${truncatePriorTaskFinal(priorTaskContext.ownerFinal)}\n---`,
|
`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) {
|
if (sections.length === 0) {
|
||||||
return '';
|
return '';
|
||||||
@@ -202,18 +197,11 @@ export function buildArbiterPromptForTask(args: {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
export function buildFinalizePendingPrompt(args: {
|
export function buildFinalizePendingPrompt(_args: {
|
||||||
turnOutputs: PairedTurnOutput[];
|
turnOutputs: PairedTurnOutput[];
|
||||||
}): string {
|
}): 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.
|
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 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 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.`;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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 chatJid = 'group@test';
|
||||||
const group = makeGroup('codex');
|
const group = makeGroup('codex');
|
||||||
const channel = makeChannel(chatJid);
|
const channel = makeChannel(chatJid);
|
||||||
const longReviewerOutput = `검토 요약 ${'a'.repeat(2105)} 끝`;
|
const longReviewerOutput = `검토 요약 ${'a'.repeat(2105)} 끝`;
|
||||||
const truncatedReviewerOutput = longReviewerOutput.slice(0, 2000);
|
|
||||||
|
|
||||||
vi.mocked(serviceRouting.hasReviewerLease).mockReturnValue(true);
|
vi.mocked(serviceRouting.hasReviewerLease).mockReturnValue(true);
|
||||||
vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({
|
vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue({
|
||||||
@@ -1701,11 +1700,12 @@ describe('createMessageRuntime', () => {
|
|||||||
vi.mocked(agentRunner.runAgentProcess).mockImplementation(
|
vi.mocked(agentRunner.runAgentProcess).mockImplementation(
|
||||||
async (_group, input, _onProcess, onOutput) => {
|
async (_group, input, _onProcess, onOutput) => {
|
||||||
expect(input.prompt).toBe(
|
expect(input.prompt).toBe(
|
||||||
`The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.
|
'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.
|
'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.
|
'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.\n\nReviewer's final assessment:\n${truncatedReviewerOutput}`,
|
'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(longReviewerOutput);
|
||||||
expect(input.prompt).not.toContain('이전 reviewer 요약');
|
expect(input.prompt).not.toContain('이전 reviewer 요약');
|
||||||
await onOutput?.({
|
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(
|
vi.mocked(agentRunner.runAgentProcess).mockImplementation(
|
||||||
async (_group, input, _onProcess, onOutput) => {
|
async (_group, input, _onProcess, onOutput) => {
|
||||||
expect(input.prompt).toBe(
|
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승인합니다.');
|
expect(input.prompt).not.toContain('DONE\n승인합니다.');
|
||||||
await onOutput?.({
|
await onOutput?.({
|
||||||
status: 'success',
|
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 chatJid = 'group@test';
|
||||||
const group = makeGroup('claude-code');
|
const group = makeGroup('claude-code');
|
||||||
const currentTask = {
|
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('Previous task owner final:');
|
||||||
expect(pending?.prompt).toContain('이전 owner 답변');
|
expect(pending?.prompt).toContain('이전 owner 답변');
|
||||||
expect(pending?.prompt).toContain('Previous task reviewer final:');
|
expect(pending?.prompt).not.toContain('Previous task reviewer final:');
|
||||||
expect(pending?.prompt).toContain('이전 reviewer 피드백');
|
expect(pending?.prompt).not.toContain('이전 reviewer 피드백');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => {
|
it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => {
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ import {
|
|||||||
runVerificationRequest,
|
runVerificationRequest,
|
||||||
} from './verification.js';
|
} from './verification.js';
|
||||||
import {
|
import {
|
||||||
|
buildWorkspaceCommandEnvironment,
|
||||||
ensureWorkspaceDependenciesInstalled,
|
ensureWorkspaceDependenciesInstalled,
|
||||||
hasInstalledNodeModules,
|
hasInstalledNodeModules,
|
||||||
} from './workspace-package-manager.js';
|
} 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(
|
const parentDir = fs.mkdtempSync(
|
||||||
path.join(os.tmpdir(), 'ejclaw-verification-corepack-parent-'),
|
path.join(os.tmpdir(), 'ejclaw-verification-corepack-parent-'),
|
||||||
);
|
);
|
||||||
@@ -122,48 +123,31 @@ describe('verification helpers', () => {
|
|||||||
JSON.stringify({
|
JSON.stringify({
|
||||||
name: 'nested-pnpm-workspace',
|
name: 'nested-pnpm-workspace',
|
||||||
scripts: {
|
scripts: {
|
||||||
typecheck:
|
typecheck: 'tsc --noEmit',
|
||||||
'node -e "process.stdout.write(process.env.COREPACK_ENABLE_PROJECT_SPEC || \'missing\')"',
|
|
||||||
},
|
},
|
||||||
}),
|
}),
|
||||||
);
|
);
|
||||||
fs.writeFileSync(
|
fs.writeFileSync(
|
||||||
path.join(repoDir, 'pnpm-lock.yaml'),
|
path.join(repoDir, 'pnpm-lock.yaml'),
|
||||||
[
|
'lockfileVersion: 9.0\n',
|
||||||
"lockfileVersion: '6.0'",
|
|
||||||
'settings:',
|
|
||||||
' autoInstallPeers: true',
|
|
||||||
' excludeLinksFromLockfile: false',
|
|
||||||
'importers:',
|
|
||||||
' .: {}',
|
|
||||||
'',
|
|
||||||
].join('\n'),
|
|
||||||
);
|
);
|
||||||
fs.mkdirSync(path.join(repoDir, 'node_modules', '.bin'), {
|
const command = buildVerificationCommand('typecheck', repoDir);
|
||||||
recursive: true,
|
|
||||||
|
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', () => {
|
it('computes a stable snapshot over the readable workspace inputs', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user