takt/#209/update review history logs (#213)

* fix: callAiJudgeをプロバイダーシステム経由に変更(Codex対応)

callAiJudgeがinfra/claude/にハードコードされており、Codexプロバイダー使用時に
judge評価が動作しなかった。agents/ai-judge.tsに移動し、runAgent経由で
プロバイダーを正しく解決するように修正。

* takt: github-issue-209
This commit is contained in:
nrs 2026-02-10 19:58:38 +09:00 committed by GitHub
parent f08c66cb63
commit 194610018a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 278 additions and 33 deletions

View File

@ -406,7 +406,7 @@ Key constraints:
- **Ephemeral lifecycle**: Clone is created → task runs → auto-commit + push → clone is deleted. Branches are the single source of truth. - **Ephemeral lifecycle**: Clone is created → task runs → auto-commit + push → clone is deleted. Branches are the single source of truth.
- **Session isolation**: Claude Code sessions are stored per-cwd in `~/.claude/projects/{encoded-path}/`. Sessions from the main project cannot be resumed in a clone. The engine skips session resume when `cwd !== projectCwd`. - **Session isolation**: Claude Code sessions are stored per-cwd in `~/.claude/projects/{encoded-path}/`. Sessions from the main project cannot be resumed in a clone. The engine skips session resume when `cwd !== projectCwd`.
- **No node_modules**: Clones only contain tracked files. `node_modules/` is absent. - **No node_modules**: Clones only contain tracked files. `node_modules/` is absent.
- **Dual cwd**: `cwd` = clone path (where agents run), `projectCwd` = project root. Reports write to `cwd/.takt/reports/` (clone) to prevent agents from discovering the main repository. Logs and session data write to `projectCwd`. - **Dual cwd**: `cwd` = clone path (where agents run), `projectCwd` = project root. Reports write to `cwd/.takt/runs/{slug}/reports/` (clone) to prevent agents from discovering the main repository. Logs and session data write to `projectCwd`.
- **List**: Use `takt list` to list branches. Instruct action creates a temporary clone for the branch, executes, pushes, then removes the clone. - **List**: Use `takt list` to list branches. Instruct action creates a temporary clone for the branch, executes, pushes, then removes the clone.
## Error Propagation ## Error Propagation
@ -455,10 +455,10 @@ Debug logs are written to `.takt/logs/debug.log` (ndjson format). Log levels: `d
- If persona file doesn't exist, the persona string is used as inline system prompt - If persona file doesn't exist, the persona string is used as inline system prompt
**Report directory structure:** **Report directory structure:**
- Report dirs are created at `.takt/reports/{timestamp}-{slug}/` - Report dirs are created at `.takt/runs/{timestamp}-{slug}/reports/`
- Report files specified in `step.report` are written relative to report dir - Report files specified in `step.report` are written relative to report dir
- Report dir path is available as `{report_dir}` variable in instruction templates - Report dir path is available as `{report_dir}` variable in instruction templates
- When `cwd !== projectCwd` (worktree execution), reports write to `cwd/.takt/reports/` (clone dir) to prevent agents from discovering the main repository path - When `cwd !== projectCwd` (worktree execution), reports write to `cwd/.takt/runs/{slug}/reports/` (clone dir) to prevent agents from discovering the main repository path
**Session continuity across phases:** **Session continuity across phases:**
- Agent sessions persist across Phase 1 → Phase 2 → Phase 3 for context continuity - Agent sessions persist across Phase 1 → Phase 2 → Phase 3 for context continuity
@ -470,7 +470,7 @@ Debug logs are written to `.takt/logs/debug.log` (ndjson format). Log levels: `d
- `git clone --shared` creates independent `.git` directory (not `git worktree`) - `git clone --shared` creates independent `.git` directory (not `git worktree`)
- Clone cwd ≠ project cwd: agents work in clone, reports write to clone, logs write to project - Clone cwd ≠ project cwd: agents work in clone, reports write to clone, logs write to project
- Session resume is skipped when `cwd !== projectCwd` to avoid cross-directory contamination - Session resume is skipped when `cwd !== projectCwd` to avoid cross-directory contamination
- Reports write to `cwd/.takt/reports/` (clone) to prevent agents from discovering the main repository path via instruction - Reports write to `cwd/.takt/runs/{slug}/reports/` (clone) to prevent agents from discovering the main repository path via instruction
- Clones are ephemeral: created → task runs → auto-commit + push → deleted - Clones are ephemeral: created → task runs → auto-commit + push → deleted
- Use `takt list` to manage task branches after clone deletion - Use `takt list` to manage task branches after clone deletion

View File

@ -0,0 +1,21 @@
import { describe, it, expect, vi } from 'vitest';
import { cleanupPieceEngine } from './engine-test-helpers.js';
describe('cleanupPieceEngine', () => {
it('should remove all listeners when engine has removeAllListeners function', () => {
const removeAllListeners = vi.fn();
const engine = { removeAllListeners };
cleanupPieceEngine(engine);
expect(removeAllListeners).toHaveBeenCalledOnce();
});
it('should not throw when engine does not have removeAllListeners function', () => {
expect(() => cleanupPieceEngine({})).not.toThrow();
expect(() => cleanupPieceEngine(null)).not.toThrow();
expect(() => cleanupPieceEngine(undefined)).not.toThrow();
expect(() => cleanupPieceEngine({ removeAllListeners: 'no-op' })).not.toThrow();
});
});

View File

@ -178,25 +178,27 @@ export function applyDefaultMocks(): void {
vi.mocked(generateReportDir).mockReturnValue('test-report-dir'); vi.mocked(generateReportDir).mockReturnValue('test-report-dir');
} }
type RemovableListeners = {
removeAllListeners: () => void;
};
function hasRemovableListeners(value: unknown): value is RemovableListeners {
if (!value || typeof value !== 'object') {
return false;
}
if (!('removeAllListeners' in value)) {
return false;
}
const candidate = value as { removeAllListeners: unknown };
return typeof candidate.removeAllListeners === 'function';
}
/** /**
* Clean up PieceEngine instances to prevent EventEmitter memory leaks. * Clean up PieceEngine instances to prevent EventEmitter memory leaks.
* Call this in afterEach to ensure all event listeners are removed. * Call this in afterEach to ensure all event listeners are removed.
*/ */
type ListenerCleanupTarget = {
removeAllListeners: () => void;
};
function isListenerCleanupTarget(value: unknown): value is ListenerCleanupTarget {
return (
typeof value === 'object' &&
value !== null &&
'removeAllListeners' in value &&
typeof value.removeAllListeners === 'function'
);
}
export function cleanupPieceEngine(engine: unknown): void { export function cleanupPieceEngine(engine: unknown): void {
if (isListenerCleanupTarget(engine)) { if (hasRemovableListeners(engine)) {
engine.removeAllListeners(); engine.removeAllListeners();
} }
} }

View File

@ -101,7 +101,7 @@ describe('renderReportOutputInstruction', () => {
const result = renderReportOutputInstruction(step, ctx, 'en'); const result = renderReportOutputInstruction(step, ctx, 'en');
expect(result).toContain('Report output'); expect(result).toContain('Report output');
expect(result).toContain('Report File'); expect(result).toContain('Report File');
expect(result).toContain('Iteration 2'); expect(result).toContain('Move current content to `logs/reports-history/`');
}); });
it('should render English multi-file instruction', () => { it('should render English multi-file instruction', () => {
@ -121,6 +121,7 @@ describe('renderReportOutputInstruction', () => {
const result = renderReportOutputInstruction(step, ctx, 'ja'); const result = renderReportOutputInstruction(step, ctx, 'ja');
expect(result).toContain('レポート出力'); expect(result).toContain('レポート出力');
expect(result).toContain('Report File'); expect(result).toContain('Report File');
expect(result).toContain('`logs/reports-history/`');
}); });
it('should render Japanese multi-file instruction', () => { it('should render Japanese multi-file instruction', () => {

View File

@ -802,7 +802,7 @@ describe('instruction-builder', () => {
expect(result).toContain('**Report output:** Output to the `Report File` specified above.'); expect(result).toContain('**Report output:** Output to the `Report File` specified above.');
expect(result).toContain('- If file does not exist: Create new file'); expect(result).toContain('- If file does not exist: Create new file');
expect(result).toContain('Append with `## Iteration 1` section'); expect(result).toContain('- If file exists: Move current content to `logs/reports-history/` and overwrite with latest report');
}); });
it('should include explicit order instead of auto-generated', () => { it('should include explicit order instead of auto-generated', () => {
@ -833,14 +833,14 @@ describe('instruction-builder', () => {
expect(result).toContain('# Plan'); expect(result).toContain('# Plan');
}); });
it('should replace {movement_iteration} in report output instruction', () => { it('should include overwrite-and-archive rule in report output instruction', () => {
const step = createMinimalStep('Do work'); const step = createMinimalStep('Do work');
step.outputContracts = [{ name: '00-plan.md' }]; step.outputContracts = [{ name: '00-plan.md' }];
const ctx = createReportContext({ movementIteration: 5 }); const ctx = createReportContext({ movementIteration: 5 });
const result = buildReportInstruction(step, ctx); const result = buildReportInstruction(step, ctx);
expect(result).toContain('Append with `## Iteration 5` section'); expect(result).toContain('Move current content to `logs/reports-history/` and overwrite with latest report');
}); });
it('should include instruction body text', () => { it('should include instruction body text', () => {

View File

@ -3,8 +3,12 @@
*/ */
import { describe, it, expect, vi, beforeEach } from 'vitest'; import { describe, it, expect, vi, beforeEach } from 'vitest';
import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import type { PieceMovement } from '../core/models/types.js'; import type { PieceMovement } from '../core/models/types.js';
import type { JudgmentContext } from '../core/piece/judgment/FallbackStrategy.js'; import type { JudgmentContext } from '../core/piece/judgment/FallbackStrategy.js';
import { runAgent } from '../agents/runner.js';
import { import {
AutoSelectStrategy, AutoSelectStrategy,
ReportBasedStrategy, ReportBasedStrategy,
@ -88,6 +92,48 @@ describe('JudgmentStrategies', () => {
// mockStep has no outputContracts field → getReportFiles returns [] // mockStep has no outputContracts field → getReportFiles returns []
expect(strategy.canApply(mockContext)).toBe(false); expect(strategy.canApply(mockContext)).toBe(false);
}); });
it('should use only latest report file from reports directory', async () => {
const tmpRoot = mkdtempSync(join(tmpdir(), 'takt-judgment-report-'));
try {
const reportDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'reports');
const historyDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'logs', 'reports-history');
mkdirSync(reportDir, { recursive: true });
mkdirSync(historyDir, { recursive: true });
const latestFile = '05-architect-review.md';
writeFileSync(join(reportDir, latestFile), 'LATEST-ONLY-CONTENT');
writeFileSync(join(historyDir, '05-architect-review.20260210T061143Z.md'), 'OLD-HISTORY-CONTENT');
const stepWithOutputContracts: PieceMovement = {
...mockStep,
outputContracts: [{ name: latestFile }],
};
const runAgentMock = vi.mocked(runAgent);
runAgentMock.mockResolvedValue({
persona: 'conductor',
status: 'done',
content: '[TEST-MOVEMENT:1]',
timestamp: new Date('2026-02-10T07:11:43Z'),
});
const strategy = new ReportBasedStrategy();
const result = await strategy.execute({
...mockContext,
step: stepWithOutputContracts,
reportDir,
});
expect(result.success).toBe(true);
expect(runAgentMock).toHaveBeenCalledTimes(1);
const instruction = runAgentMock.mock.calls[0]?.[1];
expect(instruction).toContain('LATEST-ONLY-CONTENT');
expect(instruction).not.toContain('OLD-HISTORY-CONTENT');
} finally {
rmSync(tmpRoot, { recursive: true, force: true });
}
});
}); });
describe('ResponseBasedStrategy', () => { describe('ResponseBasedStrategy', () => {

View File

@ -0,0 +1,143 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import { runReportPhase, type PhaseRunnerContext } from '../core/piece/phase-runner.js';
import type { PieceMovement } from '../core/models/types.js';
vi.mock('../agents/runner.js', () => ({
runAgent: vi.fn(),
}));
import { runAgent } from '../agents/runner.js';
function createStep(fileName: string): PieceMovement {
return {
name: 'reviewers',
personaDisplayName: 'Reviewers',
instructionTemplate: 'review',
passPreviousResponse: false,
outputContracts: [{ name: fileName }],
};
}
function createContext(reportDir: string): PhaseRunnerContext {
let currentSessionId = 'session-1';
return {
cwd: reportDir,
reportDir,
getSessionId: (_persona: string) => currentSessionId,
buildResumeOptions: (
_step,
_sessionId,
_overrides,
) => ({ cwd: reportDir }),
updatePersonaSession: (_persona, sessionId) => {
if (sessionId) {
currentSessionId = sessionId;
}
},
};
}
describe('runReportPhase report history behavior', () => {
let tmpRoot: string;
beforeEach(() => {
tmpRoot = mkdtempSync(join(tmpdir(), 'takt-report-history-'));
vi.resetAllMocks();
});
afterEach(() => {
vi.useRealTimers();
if (existsSync(tmpRoot)) {
rmSync(tmpRoot, { recursive: true, force: true });
}
});
it('should overwrite report file and archive previous content to reports-history', async () => {
// Given
const reportDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'reports');
const step = createStep('05-architect-review.md');
const ctx = createContext(reportDir);
const runAgentMock = vi.mocked(runAgent);
runAgentMock
.mockResolvedValueOnce({
persona: 'reviewers',
status: 'done',
content: 'First review result',
timestamp: new Date('2026-02-10T06:11:43Z'),
sessionId: 'session-2',
})
.mockResolvedValueOnce({
persona: 'reviewers',
status: 'done',
content: 'Second review result',
timestamp: new Date('2026-02-10T06:14:37Z'),
sessionId: 'session-3',
});
// When
await runReportPhase(step, 1, ctx);
await runReportPhase(step, 2, ctx);
// Then
const latestPath = join(reportDir, '05-architect-review.md');
const latestContent = readFileSync(latestPath, 'utf-8');
expect(latestContent).toBe('Second review result');
const historyDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'logs', 'reports-history');
const historyFiles = readdirSync(historyDir);
expect(historyFiles).toHaveLength(1);
expect(historyFiles[0]).toMatch(/^05-architect-review\.\d{8}T\d{6}Z\.md$/);
const archivedContent = readFileSync(join(historyDir, historyFiles[0]!), 'utf-8');
expect(archivedContent).toBe('First review result');
});
it('should add sequence suffix when history file name collides in the same second', async () => {
// Given
vi.useFakeTimers();
vi.setSystemTime(new Date('2026-02-10T06:11:43Z'));
const reportDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'reports');
const step = createStep('06-qa-review.md');
const ctx = createContext(reportDir);
const runAgentMock = vi.mocked(runAgent);
runAgentMock
.mockResolvedValueOnce({
persona: 'reviewers',
status: 'done',
content: 'v1',
timestamp: new Date('2026-02-10T06:11:43Z'),
sessionId: 'session-2',
})
.mockResolvedValueOnce({
persona: 'reviewers',
status: 'done',
content: 'v2',
timestamp: new Date('2026-02-10T06:11:43Z'),
sessionId: 'session-3',
})
.mockResolvedValueOnce({
persona: 'reviewers',
status: 'done',
content: 'v3',
timestamp: new Date('2026-02-10T06:11:43Z'),
sessionId: 'session-4',
});
// When
await runReportPhase(step, 1, ctx);
await runReportPhase(step, 2, ctx);
await runReportPhase(step, 3, ctx);
// Then
const historyDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'logs', 'reports-history');
const historyFiles = readdirSync(historyDir).sort();
expect(historyFiles).toEqual([
'06-qa-review.20260210T061143Z.1.md',
'06-qa-review.20260210T061143Z.md',
]);
});
});

View File

@ -298,21 +298,21 @@ export function renderReportOutputInstruction(
let heading: string; let heading: string;
let createRule: string; let createRule: string;
let appendRule: string; let overwriteRule: string;
if (language === 'ja') { if (language === 'ja') {
heading = isMulti heading = isMulti
? '**レポート出力:** Report Files に出力してください。' ? '**レポート出力:** Report Files に出力してください。'
: '**レポート出力:** `Report File` に出力してください。'; : '**レポート出力:** `Report File` に出力してください。';
createRule = '- ファイルが存在しない場合: 新規作成'; createRule = '- ファイルが存在しない場合: 新規作成';
appendRule = `- ファイルが存在する場合: \`## Iteration ${context.movementIteration}\` セクションを追記`; overwriteRule = '- ファイルが存在する場合: 既存内容を `logs/reports-history/` に退避し、最新内容で上書き';
} else { } else {
heading = isMulti heading = isMulti
? '**Report output:** Output to the `Report Files` specified above.' ? '**Report output:** Output to the `Report Files` specified above.'
: '**Report output:** Output to the `Report File` specified above.'; : '**Report output:** Output to the `Report File` specified above.';
createRule = '- If file does not exist: Create new file'; createRule = '- If file does not exist: Create new file';
appendRule = `- If file exists: Append with \`## Iteration ${context.movementIteration}\` section`; overwriteRule = '- If file exists: Move current content to `logs/reports-history/` and overwrite with latest report';
} }
return `${heading}\n${createRule}\n${appendRule}`; return `${heading}\n${createRule}\n${overwriteRule}`;
} }

View File

@ -5,8 +5,8 @@
* as session-resume operations. * as session-resume operations.
*/ */
import { appendFileSync, existsSync, mkdirSync, writeFileSync } from 'node:fs'; import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
import { dirname, resolve, sep } from 'node:path'; import { dirname, parse, resolve, sep } from 'node:path';
import type { PieceMovement, Language } from '../models/types.js'; import type { PieceMovement, Language } from '../models/types.js';
import type { PhaseName } from './types.js'; import type { PhaseName } from './types.js';
import { runAgent, type RunAgentOptions } from '../../agents/runner.js'; import { runAgent, type RunAgentOptions } from '../../agents/runner.js';
@ -49,6 +49,41 @@ export function needsStatusJudgmentPhase(step: PieceMovement): boolean {
return hasTagBasedRules(step); return hasTagBasedRules(step);
} }
function formatHistoryTimestamp(date: Date): string {
const year = date.getUTCFullYear();
const month = String(date.getUTCMonth() + 1).padStart(2, '0');
const day = String(date.getUTCDate()).padStart(2, '0');
const hour = String(date.getUTCHours()).padStart(2, '0');
const minute = String(date.getUTCMinutes()).padStart(2, '0');
const second = String(date.getUTCSeconds()).padStart(2, '0');
return `${year}${month}${day}T${hour}${minute}${second}Z`;
}
function buildHistoryFileName(fileName: string, timestamp: string, sequence: number): string {
const parsed = parse(fileName);
const duplicateSuffix = sequence === 0 ? '' : `.${sequence}`;
return `${parsed.name}.${timestamp}${duplicateSuffix}${parsed.ext}`;
}
function backupExistingReport(reportDir: string, fileName: string, targetPath: string): void {
if (!existsSync(targetPath)) {
return;
}
const currentContent = readFileSync(targetPath, 'utf-8');
const historyDir = resolve(reportDir, '..', 'logs', 'reports-history');
mkdirSync(historyDir, { recursive: true });
const timestamp = formatHistoryTimestamp(new Date());
let sequence = 0;
let historyPath = resolve(historyDir, buildHistoryFileName(fileName, timestamp, sequence));
while (existsSync(historyPath)) {
sequence += 1;
historyPath = resolve(historyDir, buildHistoryFileName(fileName, timestamp, sequence));
}
writeFileSync(historyPath, currentContent);
}
function writeReportFile(reportDir: string, fileName: string, content: string): void { function writeReportFile(reportDir: string, fileName: string, content: string): void {
const baseDir = resolve(reportDir); const baseDir = resolve(reportDir);
@ -58,12 +93,9 @@ function writeReportFile(reportDir: string, fileName: string, content: string):
throw new Error(`Report file path escapes report directory: ${fileName}`); throw new Error(`Report file path escapes report directory: ${fileName}`);
} }
mkdirSync(dirname(targetPath), { recursive: true }); mkdirSync(dirname(targetPath), { recursive: true });
if (existsSync(targetPath)) { backupExistingReport(baseDir, fileName, targetPath);
appendFileSync(targetPath, `\n\n${content}`);
} else {
writeFileSync(targetPath, content); writeFileSync(targetPath, content);
} }
}
/** /**
* Phase 2: Report output. * Phase 2: Report output.