diff --git a/docs/report-phase-permissions.md b/docs/report-phase-permissions.md new file mode 100644 index 0000000..38b1e95 --- /dev/null +++ b/docs/report-phase-permissions.md @@ -0,0 +1,34 @@ +# Report Phase Permissions Design + +## Summary + +The report phase now uses permission mode as the primary control surface. +Call sites only provide resume metadata (for example, `maxTurns`), and tool compatibility details are isolated inside `OptionsBuilder`. + +## Problem + +Historically, report phase calls passed `allowedTools: []` directly from `phase-runner`. +This made phase control depend on a tool list setting that is treated as legacy in OpenCode. + +## Design + +1. `phase-runner` uses `buildResumeOptions(step, sessionId, { maxTurns })`. +2. `OptionsBuilder.buildResumeOptions` enforces: + - `permissionMode: 'readonly'` + - `allowedTools: []` (compatibility layer for SDK behavior differences) +3. OpenCode-specific execution is controlled by permission rules (`readonly` => deny). + +## Rationale + +- OpenCode permission rules are the stable and explicit control mechanism for report-phase safety. +- Centralizing compatibility behavior in `OptionsBuilder` prevents policy leakage into movement orchestration code. +- Resume-session behavior remains deterministic for both report and status phases. + +## Test Coverage + +- `src/__tests__/options-builder.test.ts` + - verifies report/status resume options force `readonly` and empty tools. +- `src/__tests__/phase-runner-report-history.test.ts` + - verifies report phase passes only `{ maxTurns: 3 }` override. +- `src/__tests__/opencode-types.test.ts` + - verifies readonly maps to deny in OpenCode permission config. diff --git a/src/__tests__/options-builder.test.ts b/src/__tests__/options-builder.test.ts new file mode 100644 index 0000000..e1db23e --- /dev/null +++ b/src/__tests__/options-builder.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; +import { OptionsBuilder } from '../core/piece/engine/OptionsBuilder.js'; +import type { PieceMovement } from '../core/models/types.js'; +import type { PieceEngineOptions } from '../core/piece/types.js'; + +function createMovement(): PieceMovement { + return { + name: 'reviewers', + personaDisplayName: 'Reviewers', + instructionTemplate: 'review', + passPreviousResponse: false, + permissionMode: 'full', + }; +} + +function createBuilder(step: PieceMovement): OptionsBuilder { + const engineOptions: PieceEngineOptions = { + projectCwd: '/project', + }; + + return new OptionsBuilder( + engineOptions, + () => '/project', + () => '/project', + () => undefined, + () => '.takt/runs/sample/reports', + () => 'ja', + () => [{ name: step.name }], + () => 'default', + () => 'test piece', + ); +} + +describe('OptionsBuilder.buildResumeOptions', () => { + it('should enforce readonly permission and empty allowedTools for report/status phases', () => { + // Given + const step = createMovement(); + const builder = createBuilder(step); + + // When + const options = builder.buildResumeOptions(step, 'session-123', { maxTurns: 3 }); + + // Then + expect(options.permissionMode).toBe('readonly'); + expect(options.allowedTools).toEqual([]); + expect(options.maxTurns).toBe(3); + expect(options.sessionId).toBe('session-123'); + }); +}); diff --git a/src/__tests__/phase-runner-report-history.test.ts b/src/__tests__/phase-runner-report-history.test.ts index 6bd607b..6017616 100644 --- a/src/__tests__/phase-runner-report-history.test.ts +++ b/src/__tests__/phase-runner-report-history.test.ts @@ -4,6 +4,7 @@ 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'; +import type { RunAgentOptions } from '../agents/runner.js'; vi.mock('../agents/runner.js', () => ({ runAgent: vi.fn(), @@ -21,7 +22,10 @@ function createStep(fileName: string): PieceMovement { }; } -function createContext(reportDir: string): PhaseRunnerContext { +function createContext( + reportDir: string, + onBuildResumeOptions?: (overrides: Pick) => void, +): PhaseRunnerContext { let currentSessionId = 'session-1'; return { cwd: reportDir, @@ -30,8 +34,11 @@ function createContext(reportDir: string): PhaseRunnerContext { buildResumeOptions: ( _step, _sessionId, - _overrides, - ) => ({ cwd: reportDir }), + overrides, + ) => { + onBuildResumeOptions?.(overrides); + return { cwd: reportDir }; + }, buildNewSessionReportOptions: ( _step, _overrides, @@ -144,4 +151,28 @@ describe('runReportPhase report history behavior', () => { '06-qa-review.20260210T061143Z.md', ]); }); + + it('should build report resume options with maxTurns override only', async () => { + // Given + const reportDir = join(tmpRoot, '.takt', 'runs', 'sample-run', 'reports'); + const step = createStep('07-permissions-check.md'); + const capturedOverrides: Array> = []; + const ctx = createContext(reportDir, (overrides) => { + capturedOverrides.push(overrides); + }); + const runAgentMock = vi.mocked(runAgent); + runAgentMock.mockResolvedValueOnce({ + persona: 'reviewers', + status: 'done', + content: 'Permission-based report execution', + timestamp: new Date('2026-02-10T06:21:17Z'), + sessionId: 'session-2', + }); + + // When + await runReportPhase(step, 1, ctx); + + // Then + expect(capturedOverrides).toEqual([{ maxTurns: 3 }]); + }); }); diff --git a/src/core/piece/engine/OptionsBuilder.ts b/src/core/piece/engine/OptionsBuilder.ts index cdfdcee..b0711bd 100644 --- a/src/core/piece/engine/OptionsBuilder.ts +++ b/src/core/piece/engine/OptionsBuilder.ts @@ -85,14 +85,14 @@ export class OptionsBuilder { buildResumeOptions( step: PieceMovement, sessionId: string, - overrides: Pick, + overrides: Pick, ): RunAgentOptions { return { ...this.buildBaseOptions(step), // Report/status phases are read-only regardless of movement settings. permissionMode: 'readonly', sessionId, - allowedTools: overrides.allowedTools, + allowedTools: [], maxTurns: overrides.maxTurns, }; } diff --git a/src/core/piece/phase-runner.ts b/src/core/piece/phase-runner.ts index b1daf35..0b61b93 100644 --- a/src/core/piece/phase-runner.ts +++ b/src/core/piece/phase-runner.ts @@ -35,7 +35,7 @@ export interface PhaseRunnerContext { /** Get persona session ID */ getSessionId: (persona: string) => string | undefined; /** Build resume options for a movement */ - buildResumeOptions: (step: PieceMovement, sessionId: string, overrides: Pick) => RunAgentOptions; + buildResumeOptions: (step: PieceMovement, sessionId: string, overrides: Pick) => RunAgentOptions; /** Build options for report phase retry in a new session */ buildNewSessionReportOptions: (step: PieceMovement, overrides: Pick) => RunAgentOptions; /** Update persona session after a phase run */ @@ -143,7 +143,6 @@ export async function runReportPhase( }).build(); const reportOptions = ctx.buildResumeOptions(step, currentSessionId, { - allowedTools: [], maxTurns: 3, }); const firstAttempt = await runSingleReportAttempt(step, reportInstruction, reportOptions, ctx);