takt: github-issue-246-opencode-report-permission-deprecated-tools (#252)
This commit is contained in:
parent
b54fbe32b2
commit
c7f2670562
34
docs/report-phase-permissions.md
Normal file
34
docs/report-phase-permissions.md
Normal file
@ -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.
|
||||||
49
src/__tests__/options-builder.test.ts
Normal file
49
src/__tests__/options-builder.test.ts
Normal file
@ -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');
|
||||||
|
});
|
||||||
|
});
|
||||||
@ -4,6 +4,7 @@ import { join } from 'node:path';
|
|||||||
import { tmpdir } from 'node:os';
|
import { tmpdir } from 'node:os';
|
||||||
import { runReportPhase, type PhaseRunnerContext } from '../core/piece/phase-runner.js';
|
import { runReportPhase, type PhaseRunnerContext } from '../core/piece/phase-runner.js';
|
||||||
import type { PieceMovement } from '../core/models/types.js';
|
import type { PieceMovement } from '../core/models/types.js';
|
||||||
|
import type { RunAgentOptions } from '../agents/runner.js';
|
||||||
|
|
||||||
vi.mock('../agents/runner.js', () => ({
|
vi.mock('../agents/runner.js', () => ({
|
||||||
runAgent: vi.fn(),
|
runAgent: vi.fn(),
|
||||||
@ -21,7 +22,10 @@ function createStep(fileName: string): PieceMovement {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
function createContext(reportDir: string): PhaseRunnerContext {
|
function createContext(
|
||||||
|
reportDir: string,
|
||||||
|
onBuildResumeOptions?: (overrides: Pick<RunAgentOptions, 'maxTurns'>) => void,
|
||||||
|
): PhaseRunnerContext {
|
||||||
let currentSessionId = 'session-1';
|
let currentSessionId = 'session-1';
|
||||||
return {
|
return {
|
||||||
cwd: reportDir,
|
cwd: reportDir,
|
||||||
@ -30,8 +34,11 @@ function createContext(reportDir: string): PhaseRunnerContext {
|
|||||||
buildResumeOptions: (
|
buildResumeOptions: (
|
||||||
_step,
|
_step,
|
||||||
_sessionId,
|
_sessionId,
|
||||||
_overrides,
|
overrides,
|
||||||
) => ({ cwd: reportDir }),
|
) => {
|
||||||
|
onBuildResumeOptions?.(overrides);
|
||||||
|
return { cwd: reportDir };
|
||||||
|
},
|
||||||
buildNewSessionReportOptions: (
|
buildNewSessionReportOptions: (
|
||||||
_step,
|
_step,
|
||||||
_overrides,
|
_overrides,
|
||||||
@ -144,4 +151,28 @@ describe('runReportPhase report history behavior', () => {
|
|||||||
'06-qa-review.20260210T061143Z.md',
|
'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<Pick<RunAgentOptions, 'maxTurns'>> = [];
|
||||||
|
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 }]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@ -85,14 +85,14 @@ export class OptionsBuilder {
|
|||||||
buildResumeOptions(
|
buildResumeOptions(
|
||||||
step: PieceMovement,
|
step: PieceMovement,
|
||||||
sessionId: string,
|
sessionId: string,
|
||||||
overrides: Pick<RunAgentOptions, 'allowedTools' | 'maxTurns'>,
|
overrides: Pick<RunAgentOptions, 'maxTurns'>,
|
||||||
): RunAgentOptions {
|
): RunAgentOptions {
|
||||||
return {
|
return {
|
||||||
...this.buildBaseOptions(step),
|
...this.buildBaseOptions(step),
|
||||||
// Report/status phases are read-only regardless of movement settings.
|
// Report/status phases are read-only regardless of movement settings.
|
||||||
permissionMode: 'readonly',
|
permissionMode: 'readonly',
|
||||||
sessionId,
|
sessionId,
|
||||||
allowedTools: overrides.allowedTools,
|
allowedTools: [],
|
||||||
maxTurns: overrides.maxTurns,
|
maxTurns: overrides.maxTurns,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@ -35,7 +35,7 @@ export interface PhaseRunnerContext {
|
|||||||
/** Get persona session ID */
|
/** Get persona session ID */
|
||||||
getSessionId: (persona: string) => string | undefined;
|
getSessionId: (persona: string) => string | undefined;
|
||||||
/** Build resume options for a movement */
|
/** Build resume options for a movement */
|
||||||
buildResumeOptions: (step: PieceMovement, sessionId: string, overrides: Pick<RunAgentOptions, 'allowedTools' | 'maxTurns'>) => RunAgentOptions;
|
buildResumeOptions: (step: PieceMovement, sessionId: string, overrides: Pick<RunAgentOptions, 'maxTurns'>) => RunAgentOptions;
|
||||||
/** Build options for report phase retry in a new session */
|
/** Build options for report phase retry in a new session */
|
||||||
buildNewSessionReportOptions: (step: PieceMovement, overrides: Pick<RunAgentOptions, 'allowedTools' | 'maxTurns'>) => RunAgentOptions;
|
buildNewSessionReportOptions: (step: PieceMovement, overrides: Pick<RunAgentOptions, 'allowedTools' | 'maxTurns'>) => RunAgentOptions;
|
||||||
/** Update persona session after a phase run */
|
/** Update persona session after a phase run */
|
||||||
@ -143,7 +143,6 @@ export async function runReportPhase(
|
|||||||
}).build();
|
}).build();
|
||||||
|
|
||||||
const reportOptions = ctx.buildResumeOptions(step, currentSessionId, {
|
const reportOptions = ctx.buildResumeOptions(step, currentSessionId, {
|
||||||
allowedTools: [],
|
|
||||||
maxTurns: 3,
|
maxTurns: 3,
|
||||||
});
|
});
|
||||||
const firstAttempt = await runSingleReportAttempt(step, reportInstruction, reportOptions, ctx);
|
const firstAttempt = await runSingleReportAttempt(step, reportInstruction, reportOptions, ctx);
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user