diff --git a/backend/src/db/change-walkthroughs.ts b/backend/src/db/change-walkthroughs.ts index 63baea3d8..8f4ee6728 100644 --- a/backend/src/db/change-walkthroughs.ts +++ b/backend/src/db/change-walkthroughs.ts @@ -1,8 +1,13 @@ import type { Database } from 'bun:sqlite' -import { ChangeWalkthroughSchema, type ChangeWalkthrough } from '@opencode-manager/shared/schemas' +import { + ChangeWalkthroughSchema, + walkthroughSourceKey, + type ChangeWalkthrough, +} from '@opencode-manager/shared/schemas' interface ChangeWalkthroughRow { session_id: string + source_key: string diff_hash: string payload: string created_at: number @@ -19,10 +24,16 @@ export function ensureChangeWalkthroughTable(db: Database): void { `) } -export function getChangeWalkthrough(db: Database, sessionId: string): ChangeWalkthrough | null { +export function getChangeWalkthrough( + db: Database, + sessionId: string, + sourceKey: string, +): ChangeWalkthrough | null { const row = db - .prepare('SELECT session_id, diff_hash, payload, created_at FROM change_walkthroughs WHERE session_id = ?') - .get(sessionId) as ChangeWalkthroughRow | undefined + .prepare( + 'SELECT session_id, source_key, diff_hash, payload, created_at FROM change_walkthroughs WHERE session_id = ? AND source_key = ?', + ) + .get(sessionId, sourceKey) as ChangeWalkthroughRow | undefined if (!row) { return null } @@ -33,17 +44,24 @@ export function getChangeWalkthrough(db: Database, sessionId: string): ChangeWal } } -export function deleteChangeWalkthrough(db: Database, sessionId: string): void { +export function deleteChangeWalkthroughs(db: Database, sessionId: string): void { db.prepare('DELETE FROM change_walkthroughs WHERE session_id = ?').run(sessionId) } export function saveChangeWalkthrough(db: Database, walkthrough: ChangeWalkthrough): void { + const sourceKey = walkthroughSourceKey(walkthrough.source) db.prepare(` - INSERT INTO change_walkthroughs(session_id, diff_hash, payload, created_at) - VALUES(?,?,?,?) - ON CONFLICT(session_id) DO UPDATE SET + INSERT INTO change_walkthroughs(session_id, source_key, diff_hash, payload, created_at) + VALUES(?,?,?,?,?) + ON CONFLICT(session_id, source_key) DO UPDATE SET diff_hash = excluded.diff_hash, payload = excluded.payload, created_at = excluded.created_at - `).run(walkthrough.sessionId, walkthrough.diffHash, JSON.stringify(walkthrough), walkthrough.createdAt) + `).run( + walkthrough.sessionId, + sourceKey, + walkthrough.diffHash, + JSON.stringify(walkthrough), + walkthrough.createdAt, + ) } diff --git a/backend/src/db/migrations/202610081200-change-walkthrough-sources.ts b/backend/src/db/migrations/202610081200-change-walkthrough-sources.ts new file mode 100644 index 000000000..8df875637 --- /dev/null +++ b/backend/src/db/migrations/202610081200-change-walkthrough-sources.ts @@ -0,0 +1,28 @@ +import type { Migration } from '../migration-runner' + +const migration: Migration = { + id: '202610081200-change-walkthrough-sources', + up(db) { + const columns = db.prepare('PRAGMA table_info(change_walkthroughs)').all() as Array<{ name: string }> + if (columns.some((column) => column.name === 'source_key')) { + return + } + + db.run('DROP TABLE IF EXISTS change_walkthroughs') + db.run(` + CREATE TABLE change_walkthroughs ( + session_id TEXT NOT NULL, + source_key TEXT NOT NULL, + diff_hash TEXT NOT NULL, + payload TEXT NOT NULL, + created_at INTEGER NOT NULL, + PRIMARY KEY (session_id, source_key) + ) + `) + }, + down(db) { + db.run('DROP TABLE IF EXISTS change_walkthroughs') + }, +} + +export default migration diff --git a/backend/src/db/migrations/index.ts b/backend/src/db/migrations/index.ts index e0f7ae43c..20084f541 100644 --- a/backend/src/db/migrations/index.ts +++ b/backend/src/db/migrations/index.ts @@ -29,6 +29,7 @@ import migration027 from './027-multi-runs' import migration202610061345 from './202610061345-schedule-workspace-mode' import migration202610061700 from './202610061700-multi-run-fusions' import migration202610061701 from './202610061701-change-walkthroughs' +import migration202610081200 from './202610081200-change-walkthrough-sources' export const allMigrations: Migration[] = [ migration001, @@ -61,4 +62,5 @@ export const allMigrations: Migration[] = [ migration202610061345, migration202610061700, migration202610061701, + migration202610081200, ] diff --git a/backend/src/index.ts b/backend/src/index.ts index cb0675a4b..b5c6d4ed5 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -244,7 +244,8 @@ const terminalService = new TerminalService( new CredentialProvider(db), () => getOpenCodeUpstreamBaseUrl(opencodeServerManager.getEffectiveServerHost()), ) -const projectConfigService = new ProjectConfigService(db, createGitService(gitAuthService), gitAuthService) +const gitService = createGitService(gitAuthService) +const projectConfigService = new ProjectConfigService(db, gitService, gitAuthService) const scheduleWorktreeManager = new ScheduleWorktreeManager(gitAuthService, db) const scheduleService = new ScheduleService(db, openCodeClient, scheduleWorktreeManager) scheduleService.setWorktreeRemovedHandler(async (directory) => { @@ -272,7 +273,7 @@ const sessionGoalService = new SessionGoalService(db, openCodeClient, sessionSet sessionGoalService.loadOpenGoals() const multiRunService = new MultiRunService(db, openCodeClient, repoWorkspaces, sessionPermissionModeService) -const changeWalkthroughService = new ChangeWalkthroughService(db, openCodeClient) +const changeWalkthroughService = new ChangeWalkthroughService(db, openCodeClient, sessionSettingsService, gitAuthService, gitService) sseAggregator.onEvent((directory, event) => { sessionPermissionModeService.handleEvent(directory, event).catch((err) => { diff --git a/backend/src/routes/change-walkthroughs.ts b/backend/src/routes/change-walkthroughs.ts index aabbee21e..1e02f576a 100644 --- a/backend/src/routes/change-walkthroughs.ts +++ b/backend/src/routes/change-walkthroughs.ts @@ -1,5 +1,9 @@ import { Hono } from 'hono' -import { GenerateChangeWalkthroughRequestSchema } from '@opencode-manager/shared/schemas' +import { + DEFAULT_WALKTHROUGH_SOURCE, + GenerateChangeWalkthroughRequestSchema, + parseWalkthroughSourceKey, +} from '@opencode-manager/shared/schemas' import type { ChangeWalkthroughService } from '../services/change-walkthroughs' import { handleServiceError, parseJsonBody } from '../utils/route-helpers' import { ServiceError } from '../utils/service-error' @@ -8,8 +12,15 @@ export function createChangeWalkthroughRoutes(service: ChangeWalkthroughService) const app = new Hono() app.get('/:sessionId', async (c) => { + const sourceParam = c.req.query('source') + const source = sourceParam === undefined ? DEFAULT_WALKTHROUGH_SOURCE : parseWalkthroughSourceKey(sourceParam) + if (!source) { + return c.json({ error: 'Invalid walkthrough source' }, 400) + } + try { - const state = await service.getState(c.req.param('sessionId')) + const hunksFor = c.req.query('hunksFor') + const state = await service.getState(c.req.param('sessionId'), source, hunksFor) return c.json(state) } catch (error) { return handleServiceError(c, error, 'Failed to read change walkthrough', ServiceError) diff --git a/backend/src/services/assistant-mode.ts b/backend/src/services/assistant-mode.ts index 8f44f38e3..de6691193 100644 --- a/backend/src/services/assistant-mode.ts +++ b/backend/src/services/assistant-mode.ts @@ -12,7 +12,7 @@ import { fileExists, ensureDirectoryExists, } from './file-operations' -import { ASSISTANT_NOTIFICATION_LIMITS, OpenCodeConfigSchema, DEFAULT_KEYBOARD_SHORTCUTS, DEFAULT_LEADER_KEY } from '@opencode-manager/shared/schemas' +import { ASSISTANT_NOTIFICATION_LIMITS, AssistantSettingsPatchSchema, OpenCodeConfigSchema, DEFAULT_KEYBOARD_SHORTCUTS, DEFAULT_LEADER_KEY } from '@opencode-manager/shared/schemas' import { MANAGER_COLOR_THEME_ID, OPENCODE_THEMES } from '@opencode-manager/shared/themes' import { ASSISTANT_REPO_ID, ASSISTANT_REPO_PATH, ASSISTANT_OPENCODE_DIR_NAME } from '@opencode-manager/shared/utils' import { getAssistantModePath, getReposPath } from '@opencode-manager/shared/config/env' @@ -581,6 +581,23 @@ Sending is rate limited to **10 notifications per minute**. Beyond that the tool ` } +type AssistantSettingsKey = keyof typeof AssistantSettingsPatchSchema.shape + +const ASSISTANT_SETTINGS_KEY_NOTES: Partial> = { + tts: 'Non-secret TTS preferences (`enabled`, `provider`, `autoPlay`, `voice`, `model`, `speed`). TTS must already be configured in the UI (the endpoint returns 400 otherwise).', + stt: 'Non-secret STT preferences (`enabled`, `provider`, `model`, `language`). STT must already be configured in the UI (the endpoint returns 400 otherwise).', +} + +function buildAllowedSettingsKeysList(): string { + const keys = Object.keys(AssistantSettingsPatchSchema.shape) as AssistantSettingsKey[] + return keys + .map((key) => { + const note = ASSISTANT_SETTINGS_KEY_NOTES[key] + return note === undefined ? `- \`${key}\`` : `- \`${key}\` — ${note}` + }) + .join('\n') +} + export function buildSettingsSkill(): string { return `--- name: manager-settings @@ -660,13 +677,7 @@ Update a subset of safe user preferences. **Allowed Keys:** The following preference keys can be modified: -- \`theme\`, \`colorTheme\`, \`mode\`, \`defaultModel\`, \`defaultAgent\` -- \`autoScroll\`, \`expandDiffs\`, \`expandToolCalls\`, \`showReasoning\` -- \`simpleChatMode\`, \`leaderKey\`, \`directShortcuts\` -- \`keyboardShortcuts\`, \`customCommands\`, \`notifications\` -- \`repoOrder\`, \`repoSortMode\` -- \`tts\` — Non-secret TTS preferences (\`enabled\`, \`provider\`, \`autoPlay\`, \`voice\`, \`model\`, \`speed\`). TTS must already be configured in the UI (the endpoint returns 400 otherwise). -- \`stt\` — Non-secret STT preferences (\`enabled\`, \`provider\`, \`model\`, \`language\`). STT must already be configured in the UI (the endpoint returns 400 otherwise). +${buildAllowedSettingsKeysList()} **DO NOT attempt to set:** - \`gitCredentials\` - Git credentials must be managed via the full UI diff --git a/backend/src/services/change-walkthrough-error.ts b/backend/src/services/change-walkthrough-error.ts new file mode 100644 index 000000000..89b5fc1d8 --- /dev/null +++ b/backend/src/services/change-walkthrough-error.ts @@ -0,0 +1,8 @@ +import { ServiceError } from '../utils/service-error' + +export class ChangeWalkthroughError extends ServiceError { + constructor(message: string, status: number, options?: { code?: string; details?: unknown }) { + super(message, status, options) + this.name = 'ChangeWalkthroughError' + } +} diff --git a/backend/src/services/change-walkthroughs.ts b/backend/src/services/change-walkthroughs.ts index 6ea59169b..470c1e952 100644 --- a/backend/src/services/change-walkthroughs.ts +++ b/backend/src/services/change-walkthroughs.ts @@ -2,39 +2,103 @@ import type { Database } from 'bun:sqlite' import { createHash } from 'node:crypto' import { z } from 'zod' import { + formatOpenCodeModelRef, isSessionNotFoundError, + parseOpenCodeModelRef, type FileDiffInfo, + type ModelRef, type SessionInfo, } from '@opencode-manager/shared/opencode' +import { getOpenCodeGlobalConfigPath } from '@opencode-manager/shared/config/env' import { + DEFAULT_WALKTHROUGH_SOURCE, + MECHANICAL_TEXT_BUDGET, WALKTHROUGH_DIFF_MAX_CHARS, WALKTHROUGH_HUNK_MAX_CHARS, WALKTHROUGH_MAX_STOPS, + WALKTHROUGH_OUTLINE_LINE_MAX_CHARS, + WALKTHROUGH_OUTLINE_PREVIEW_LINES, WALKTHROUGH_TEXT_MAX_CHARS, + describeWalkthroughSource, + walkthroughHunksIdentity, + walkthroughSourceKey, type ChangeWalkthrough, - type ChangeWalkthroughState, + type ChangeWalkthroughStateWire, + type ChangeWalkthroughWire, type GenerateChangeWalkthroughRequest, type WalkthroughGenerationError, type WalkthroughHunk, type WalkthroughOmittedFile, + type WalkthroughSource, type WalkthroughStop, } from '@opencode-manager/shared/schemas' -import { splitDiffHunks } from '@opencode-manager/shared/utils' -import { deleteChangeWalkthrough, getChangeWalkthrough, saveChangeWalkthrough } from '../db/change-walkthroughs' +import { countDiffLineChanges, parseDiffLines, splitDiffHunks, type DiffHunk } from '@opencode-manager/shared/utils' +import { deleteChangeWalkthroughs, getChangeWalkthrough, saveChangeWalkthrough } from '../db/change-walkthroughs' import { getErrorMessage } from '../utils/error-utils' import { ServiceError } from '../utils/service-error' import { truncateText } from '../utils/text-truncate' -import { extractFirstJsonObject } from '../utils/json-extract' +import { parseFirstJsonObject } from '../utils/json-extract' +import { mapWithConcurrency, createLimiter } from '../utils/concurrency' +import { logger } from '../utils/logger' import { GenerateTextTimeoutError, generateTextWithTimeout } from './opencode/generate-text' +import { resolveOpenCodeModel } from './opencode-models' import type { OpenCodeClient } from './opencode/client' -import { readSessionChanges } from './session-changes' +import { ChangeWalkthroughError } from './change-walkthrough-error' +import { readWalkthroughChanges, fetchPullRequestRef, deletePullRequestRef } from './walkthrough-sources' +import { resolveRepoForDirectory } from './repo' +import type { GitService } from './git/GitService' +import type { GitAuthService } from './git-auth' +import type { SettingsService } from './settings' import type { SSEEvent } from './sse-aggregator' const DEFAULT_TIMEOUT_MS = 120_000 +const WALKTHROUGH_CALL_ATTEMPTS = 2 +const WALKTHROUGH_EXPLAIN_CONCURRENCY = 4 +const WALKTHROUGH_SAVE_EVERY_STOPS = 4 +const WALKTHROUGH_PROMPT_VERSION = '2' const HUNK_TRUNCATION_MARKER = '\n[hunk truncated]' const TEXT_TRUNCATION_MARKER = '' const REMAINING_STOP_TITLE = 'Remaining changes' const REMAINING_STOP_EXPLANATION = 'These changes were not covered by the generated walkthrough.' +const MECHANICAL_STOP_TITLE = 'Mechanical changes' +const MECHANICAL_STOP_EXPLANATION = + 'Lock files, snapshots and generated files. Their contents were not sent to the model.' +const MECHANICAL_ONLY_SUMMARY = 'Only lock files, snapshots or generated files changed.' +const MECHANICAL_PARTIAL_SUMMARY = + 'Some changes were omitted. Only lock files, snapshots or generated files are shown.' +const OUTLINE_RETRY_DEPTHS = [3, 1, 0] +const WALKTHROUGH_MODEL_LIMITER = createLimiter(WALKTHROUGH_EXPLAIN_CONCURRENCY) + +const MECHANICAL_LOCKFILE_NAMES = new Set([ + 'pnpm-lock.yaml', + 'package-lock.json', + 'npm-shrinkwrap.json', + 'yarn.lock', + 'bun.lock', + 'bun.lockb', + 'Cargo.lock', + 'Gemfile.lock', + 'composer.lock', + 'poetry.lock', + 'uv.lock', + 'Pipfile.lock', + 'go.sum', + 'flake.lock', +]) + +const MECHANICAL_SUFFIXES = ['.snap', '.min.js', '.min.css', '.map'] + +export function isMechanicalChangePath(file: string): boolean { + const segments = file.split('/') + const basename = segments.at(-1) ?? file + if (MECHANICAL_LOCKFILE_NAMES.has(basename)) { + return true + } + if (MECHANICAL_SUFFIXES.some((suffix) => file.endsWith(suffix))) { + return true + } + return segments.includes('__snapshots__') +} const modelStopSchema = z.object({ title: z.string().catch(''), @@ -49,6 +113,7 @@ const modelResponseSchema = z.object({ export interface WalkthroughInput { hunks: WalkthroughHunk[] + mechanicalHunks: WalkthroughHunk[] omittedFiles: WalkthroughOmittedFile[] } @@ -61,13 +126,6 @@ export interface ChangeWalkthroughServiceOptions { timeoutMs?: number } -export class ChangeWalkthroughError extends ServiceError { - constructor(message: string, status: number, options?: { code?: string; details?: unknown }) { - super(message, status, options) - this.name = 'ChangeWalkthroughError' - } -} - export function computeChangesHash(changes: FileDiffInfo[]): string { const hash = createHash('sha256') for (const change of changes) { @@ -76,13 +134,94 @@ export function computeChangesHash(changes: FileDiffInfo[]): string { return hash.digest('hex') } -export function buildWalkthroughInstructions(title: string): string { +export function computeHunkId(file: string, status: FileDiffInfo['status'], hunkText: string): string { + const body = hunkText.split('\n').slice(1).join('\n') + const digest = createHash('sha256').update(`${file}\0${status}\0${body}`).digest('hex') + return `h_${digest.slice(0, 12)}` +} + +export function computeStopId(hunkIds: string[]): string { + const digest = createHash('sha256').update([...hunkIds].sort().join('\n')).digest('hex') + return `s_${digest.slice(0, 12)}` +} + +function computeExplanationKey(modelKey: string, hunkIds: string[]): string { + const digest = createHash('sha256') + .update(`${WALKTHROUGH_PROMPT_VERSION}\0${modelKey}\0${[...hunkIds].sort().join('\n')}`) + .digest('hex') + return digest +} + +function computeModelKey(model: ModelRef): string { + return formatOpenCodeModelRef(model) +} + +function truncateWalkthroughText(text: string): string { + return truncateText(text, WALKTHROUGH_TEXT_MAX_CHARS, TEXT_TRUNCATION_MARKER).text +} + +function entryKey(sessionId: string, source: WalkthroughSource): string { + return `${sessionId}\0${walkthroughSourceKey(source)}` +} + +function walkthroughTitle(session: SessionInfo, source: WalkthroughSource): string { + const base = session.title ?? session.id + return source.kind === 'session' ? base : `${base} — ${describeWalkthroughSource(source)}` +} + +function sourceChangesVerb(source: WalkthroughSource): string { + return source.kind === 'branch' || source.kind === 'pullRequest' ? 'contains' : 'contain' +} + +function noChangesMessage(source: WalkthroughSource): string { + if (source.kind === 'session') { + return 'This session has no changes to walk through' + } + return `The ${describeWalkthroughSource(source)} ${sourceChangesVerb(source)} no changes to walk through` +} + +function noTextChangesMessage(source: WalkthroughSource): string { + if (source.kind === 'session') { + return 'This session has no text changes to walk through' + } + return `The ${describeWalkthroughSource(source)} ${sourceChangesVerb(source)} no text changes` +} + +function applyReuse(stop: WalkthroughStop, reusable: Map, modelKey: string): WalkthroughStop { + const key = computeExplanationKey(modelKey, stop.hunkIds) + const explanation = reusable.get(key) + if (explanation !== undefined) { + return { ...stop, explanation, status: 'ready', explanationKey: key } + } + return stop.status === 'ready' ? { ...stop, explanationKey: key } : stop +} + +function isRemainingPlaceholder(stop: WalkthroughStop): boolean { + return stop.title === REMAINING_STOP_TITLE && stop.explanation === REMAINING_STOP_EXPLANATION +} + +function applySingleCallKeys(stop: WalkthroughStop, modelKey: string): WalkthroughStop { + if (stop.status !== 'ready' || stop.explanationKey !== null || isRemainingPlaceholder(stop)) { + return stop + } + return { ...stop, explanationKey: computeExplanationKey(modelKey, stop.hunkIds) } +} + +function buildWalkthroughPreamble(title: string): string { return [ `You are writing a change walkthrough for a reviewer who reads it top to bottom to understand the change to "${title}".`, '## How to write the walkthrough', [ '- Order the stops by the reading order that best explains the change: contracts and data model first, then core logic, then callers and UI, then tests and config.', '- Group related hunks, possibly across files.', + ].join('\n'), + ].join('\n') +} + +function buildWalkthroughInstructions(title: string): string { + return [ + buildWalkthroughPreamble(title), + [ '- Reference only the hunk ids given below, each at most once.', `- Use at most ${WALKTHROUGH_MAX_STOPS} stops.`, '- Explain intent and impact rather than restating the code.', @@ -92,32 +231,59 @@ export function buildWalkthroughInstructions(title: string): string { ].join('\n') } -export function formatHunkBlock(hunk: WalkthroughHunk): string { +export function formatHunkOutline( + hunk: WalkthroughHunk, + previewLines = WALKTHROUGH_OUTLINE_PREVIEW_LINES, +): string { + const { added, removed } = countDiffLineChanges(hunk.text) + const head = `### ${hunk.id} ${hunk.file} (${hunk.status}) ${hunk.header} +${added} -${removed}` + const preview = parseDiffLines(hunk.text) + .filter((line) => line.type === 'add' || line.type === 'remove') + .slice(0, previewLines) + .map((line) => + truncateText( + `${line.type === 'add' ? '+' : '-'}${line.content}`, + WALKTHROUGH_OUTLINE_LINE_MAX_CHARS, + TEXT_TRUNCATION_MARKER, + ).text, + ) + return [head, ...preview].join('\n') +} + +function formatHunkBlock(hunk: WalkthroughHunk): string { return `### ${hunk.id} ${hunk.file} (${hunk.status})\n\n\`\`\`diff\n${hunk.text}\n\`\`\`` } -export function buildWalkthroughInput(changes: FileDiffInfo[], title: string): WalkthroughInput { +function classifyOmittedFileReason(patch: string): WalkthroughOmittedFile['reason'] { + const binary = patch.includes('Binary files') || patch.includes('GIT binary patch') + if (!binary && patch.includes('rename to ')) { + return 'renamed' + } + if (!binary && (patch.includes('old mode ') || patch.includes('new mode '))) { + return 'modeChange' + } + return 'binary' +} + +export function buildWalkthroughInput(changes: FileDiffInfo[]): WalkthroughInput { const hunks: WalkthroughHunk[] = [] + const mechanicalHunks: WalkthroughHunk[] = [] const omittedFiles: WalkthroughOmittedFile[] = [] - let total = buildWalkthroughInstructions(title).length - let budgetExhausted = false + const hunkIdCounts = new Map() + let mechanicalTextLength = 0 - changes.forEach((change, fileIndex) => { - if (budgetExhausted) { - omittedFiles.push({ file: change.file, reason: 'budget' }) - return - } - - const fileHunks = splitDiffHunks(change.patch) - if (fileHunks.length === 0) { - omittedFiles.push({ file: change.file, reason: 'binary' }) - return - } + const nextHunkId = (file: string, status: FileDiffInfo['status'], text: string): string => { + const baseId = computeHunkId(file, status, text) + const count = (hunkIdCounts.get(baseId) ?? 0) + 1 + hunkIdCounts.set(baseId, count) + return count === 1 ? baseId : `${baseId}_${count}` + } - const prepared = fileHunks.map((hunk, hunkIndex) => { + const prepareHunks = (change: FileDiffInfo, fileHunks: DiffHunk[]): WalkthroughHunk[] => + fileHunks.map((hunk) => { const truncated = truncateText(hunk.text, WALKTHROUGH_HUNK_MAX_CHARS, HUNK_TRUNCATION_MARKER) return { - id: `f${fileIndex}h${hunkIndex}`, + id: nextHunkId(change.file, change.status, hunk.text), file: change.file, status: change.status, header: hunk.header, @@ -126,18 +292,84 @@ export function buildWalkthroughInput(changes: FileDiffInfo[], title: string): W } satisfies WalkthroughHunk }) - const fileTotal = prepared.reduce((sum, hunk) => sum + formatHunkBlock(hunk).length + 2, 0) - if (total + fileTotal > WALKTHROUGH_DIFF_MAX_CHARS) { - budgetExhausted = true - omittedFiles.push({ file: change.file, reason: 'budget' }) + const buildMechanicalHunk = (change: FileDiffInfo, fileHunks: DiffHunk[]): WalkthroughHunk => { + const text = fileHunks.map((hunk) => hunk.text).join('\n') + return { + id: nextHunkId(change.file, change.status, text), + file: change.file, + status: change.status, + header: fileHunks[0]!.header, + text, + truncated: false, + additions: change.additions, + deletions: change.deletions, + } satisfies WalkthroughHunk + } + + const buildMechanicalSummaryHunk = (change: FileDiffInfo): WalkthroughHunk => ({ + id: nextHunkId(change.file, change.status, ''), + file: change.file, + status: change.status, + header: '', + text: '', + truncated: true, + additions: change.additions, + deletions: change.deletions, + }) + + changes.forEach((change) => { + const mechanical = isMechanicalChangePath(change.file) + const fileHunks = splitDiffHunks(change.patch) + if (fileHunks.length === 0) { + omittedFiles.push({ file: change.file, reason: classifyOmittedFileReason(change.patch) }) + return + } + + if (mechanical) { + const text = fileHunks.map((hunk) => hunk.text).join('\n') + if (mechanicalTextLength + text.length <= MECHANICAL_TEXT_BUDGET) { + mechanicalHunks.push(buildMechanicalHunk(change, fileHunks)) + mechanicalTextLength += text.length + } else { + mechanicalHunks.push(buildMechanicalSummaryHunk(change)) + } return } - total += fileTotal - hunks.push(...prepared) + hunks.push(...prepareHunks(change, fileHunks)) }) - return { hunks, omittedFiles } + return { hunks, mechanicalHunks, omittedFiles } +} + +function createStop({ + hunkIds, + title, + explanation, + status, +}: { + hunkIds: string[] + title: string + explanation: string + status: WalkthroughStop['status'] +}): WalkthroughStop { + return { + id: computeStopId(hunkIds), + title, + explanation, + hunkIds, + status, + explanationKey: null, + } +} + +function buildMechanicalStop(hunks: WalkthroughHunk[]): WalkthroughStop { + return createStop({ + hunkIds: hunks.map((hunk) => hunk.id), + title: MECHANICAL_STOP_TITLE, + explanation: MECHANICAL_STOP_EXPLANATION, + status: 'ready', + }) } export function buildWalkthroughPrompt({ title, hunks }: { title: string; hunks: WalkthroughHunk[] }): string { @@ -147,29 +379,148 @@ export function buildWalkthroughPrompt({ title, hunks }: { title: string; hunks: return [instructions, ...blocks].join('\n\n') } -export function parseWalkthroughResponse(text: string, hunks: WalkthroughHunk[]): ParsedWalkthrough | null { - const extracted = extractFirstJsonObject(text) - if (!extracted) { - return null +export interface WalkthroughOutline { + hunks: WalkthroughHunk[] + omittedFiles: WalkthroughOmittedFile[] + outline: string +} + +function groupHunksByFile(hunks: WalkthroughHunk[]): WalkthroughHunk[][] { + const groups: WalkthroughHunk[][] = [] + for (const hunk of hunks) { + const last = groups.at(-1) + if (last && last[0]!.file === hunk.file) { + last.push(hunk) + } else { + groups.push([hunk]) + } } + return groups +} - let raw: unknown - try { - raw = JSON.parse(extracted) - } catch { - return null +function buildWalkthroughPlanRules(title: string): string { + return [ + buildWalkthroughPreamble(title), + [ + '- Assign every hunk id to exactly one stop.', + '- Reference each hunk id at most once.', + `- Use at most ${WALKTHROUGH_MAX_STOPS} stops.`, + '- Respond with only the JSON {"summary": string, "stops": [{"title": string, "hunkIds": string[]}]}.', + ].join('\n'), + ].join('\n') +} + +export function buildWalkthroughPlanPrompt({ + title, + outline, + previousStops, +}: { + title: string + outline: string + previousStops: Array<{ title: string; hunkIds: string[] }> +}): string { + const prompt = buildWalkthroughPlanSections(title, outline, previousStops).join('\n\n') + if (previousStops.length > 0 && prompt.length > WALKTHROUGH_DIFF_MAX_CHARS) { + return buildWalkthroughPlanSections(title, outline, []).join('\n\n') } + return prompt +} - const parsed = modelResponseSchema.safeParse(raw) - if (!parsed.success) { - return null +function buildPreviousStopsSection(previousStops: Array<{ title: string; hunkIds: string[] }>): string { + return [ + '## Previous stops (keep a grouping when it still explains the change)', + ...previousStops.map((stop) => `- ${stop.title}: ${stop.hunkIds.join(', ')}`), + ].join('\n') +} + +function buildWalkthroughPlanSections( + title: string, + outline: string, + previousStops: Array<{ title: string; hunkIds: string[] }>, +): string[] { + const sections = [buildWalkthroughPlanRules(title)] + if (previousStops.length > 0) { + sections.push(buildPreviousStopsSection(previousStops)) + } + sections.push(`## Outline\n${outline}`) + return sections +} + +export function buildWalkthroughOutline( + input: WalkthroughInput, + title: string, + previousStops: Array<{ title: string; hunkIds: string[] }> = [], +): WalkthroughOutline { + const groups = groupHunksByFile(input.hunks) + const reservedHint = + previousStops.length > 0 ? '\n\n'.length + buildPreviousStopsSection(previousStops).length : 0 + const prefixLength = buildWalkthroughPlanRules(title).length + '\n\n## Outline\n'.length + reservedHint + + const pack = (previewLines: number) => { + let outline = '' + const hunks: WalkthroughHunk[] = [] + const omittedFiles: WalkthroughOmittedFile[] = [] + let exhausted = false + for (const fileHunks of groups) { + if (exhausted) { + omittedFiles.push({ file: fileHunks[0]!.file, reason: 'budget' }) + continue + } + const block = fileHunks.map((hunk) => formatHunkOutline(hunk, previewLines)).join('\n') + const next = outline.length === 0 ? block : `${outline}\n\n${block}` + if (prefixLength + next.length > WALKTHROUGH_DIFF_MAX_CHARS) { + exhausted = true + omittedFiles.push({ file: fileHunks[0]!.file, reason: 'budget' }) + continue + } + outline = next + hunks.push(...fileHunks) + } + return { hunks, omittedFiles, outline } + } + + let packed = pack(WALKTHROUGH_OUTLINE_PREVIEW_LINES) + if (packed.omittedFiles.length > 0) { + for (const previewLines of OUTLINE_RETRY_DEPTHS) { + const candidate = pack(previewLines) + packed = candidate + if (candidate.omittedFiles.length === 0) { + break + } + } + } + + return { + hunks: packed.hunks, + omittedFiles: [...input.omittedFiles, ...packed.omittedFiles], + outline: packed.outline, } +} + +export function fitsSingleCall(input: WalkthroughInput, title: string): boolean { + return buildWalkthroughPrompt({ title, hunks: input.hunks }).length <= WALKTHROUGH_DIFF_MAX_CHARS +} + +interface ModelStop { + title: string + explanation: string + hunkIds: string[] +} +function parseModelResponse(text: string): { summary: string; stops: ModelStop[] } | null { + return parseFirstJsonObject(text, modelResponseSchema) +} + +function collectModelStops( + modelStops: ModelStop[], + hunks: WalkthroughHunk[], + buildStop: (modelStop: ModelStop | null, hunkIds: string[]) => WalkthroughStop, +): WalkthroughStop[] { const validIds = new Set(hunks.map((hunk) => hunk.id)) const referenced = new Set() const stops: WalkthroughStop[] = [] - for (const modelStop of parsed.data.stops) { + for (const modelStop of modelStops) { if (stops.length >= WALKTHROUGH_MAX_STOPS) { break } @@ -183,39 +534,152 @@ export function parseWalkthroughResponse(text: string, hunks: WalkthroughHunk[]) hunkIds.push(id) } - if (hunkIds.length === 0) { - continue + if (hunkIds.length > 0) { + stops.push(buildStop(modelStop, hunkIds)) } - - stops.push({ - title: truncateText(modelStop.title, WALKTHROUGH_TEXT_MAX_CHARS, TEXT_TRUNCATION_MARKER).text, - explanation: truncateText(modelStop.explanation, WALKTHROUGH_TEXT_MAX_CHARS, TEXT_TRUNCATION_MARKER).text, - hunkIds, - }) } if (stops.length === 0) { - return null + return stops } const unreferenced = hunks.filter((hunk) => !referenced.has(hunk.id)).map((hunk) => hunk.id) if (unreferenced.length > 0) { - stops.push({ - title: REMAINING_STOP_TITLE, - explanation: REMAINING_STOP_EXPLANATION, - hunkIds: unreferenced, - }) + stops.push(buildStop(null, unreferenced)) + } + + return stops +} + +export function parseWalkthroughResponse(text: string, hunks: WalkthroughHunk[]): ParsedWalkthrough | null { + const parsed = parseModelResponse(text) + if (!parsed) { + return null + } + + const stops = collectModelStops(parsed.stops, hunks, (modelStop, hunkIds) => + modelStop + ? createStop({ + hunkIds, + title: truncateWalkthroughText(modelStop.title), + explanation: truncateWalkthroughText(modelStop.explanation), + status: 'ready', + }) + : createStop({ + hunkIds, + title: REMAINING_STOP_TITLE, + explanation: REMAINING_STOP_EXPLANATION, + status: 'ready', + }), + ) + + if (stops.length === 0) { + return null + } + + return { + summary: truncateWalkthroughText(parsed.summary), + stops, + } +} + +export function parseWalkthroughPlan(text: string, hunks: WalkthroughHunk[]): ParsedWalkthrough | null { + const parsed = parseModelResponse(text) + if (!parsed) { + return null + } + + const stops = collectModelStops(parsed.stops, hunks, (modelStop, hunkIds) => + createStop({ + hunkIds, + title: modelStop ? truncateWalkthroughText(modelStop.title) : REMAINING_STOP_TITLE, + explanation: '', + status: 'pending', + }), + ) + + if (stops.length === 0) { + return null } return { - summary: truncateText(parsed.data.summary, WALKTHROUGH_TEXT_MAX_CHARS, TEXT_TRUNCATION_MARKER).text, + summary: truncateWalkthroughText(parsed.summary), stops, } } +const explanationSchema = z.object({ + explanation: z.string().catch(''), +}) + +function parseWalkthroughExplanation(text: string): string | null { + const parsed = parseFirstJsonObject(text, explanationSchema) + if (!parsed) { + return null + } + + const explanation = parsed.explanation.trim() + if (explanation.length === 0) { + return null + } + + return truncateWalkthroughText(explanation) +} + +function buildWalkthroughExplainPrompt({ + title, + summary, + stop, + hunks, +}: { + title: string + summary: string + stop: WalkthroughStop + hunks: WalkthroughHunk[] +}): string { + const rule = 'Respond with only the JSON {"explanation": string}.' + const stopHunks = hunks.filter((hunk) => stop.hunkIds.includes(hunk.id)) + const context = [ + buildWalkthroughPreamble(title), + `## Walkthrough summary\n${summary}`, + `## Stop: ${stop.title}`, + ].join('\n\n') + + const sections = [context] + let shown = 0 + + for (const hunk of stopHunks) { + const block = formatHunkBlock(hunk) + const omittedCount = stopHunks.length - shown - 1 + const candidate = [...sections, block, rule] + if (omittedCount > 0) { + candidate.push(`${omittedCount} more hunks in this stop are not shown.`) + } + if (candidate.join('\n\n').length > WALKTHROUGH_DIFF_MAX_CHARS) { + break + } + sections.push(block) + shown += 1 + } + + if (shown < stopHunks.length) { + sections.push(`${stopHunks.length - shown} more hunks in this stop are not shown.`) + } + sections.push(rule) + return sections.join('\n\n') +} + interface InFlightGeneration { modelStarted: Promise result: Promise<{ walkthrough: ChangeWalkthrough; created: boolean }> + walkthrough: ChangeWalkthrough | null +} + +class WalkthroughCallSkippedError extends Error { + constructor() { + super('Walkthrough model call skipped') + this.name = 'WalkthroughCallSkippedError' + } } function toGenerationError(error: unknown): WalkthroughGenerationError { @@ -234,68 +698,207 @@ export class ChangeWalkthroughService { private readonly inFlight = new Map() private readonly failures = new Map() private readonly deletedDuringGeneration = new Set() + private readonly currentHashes = new Map() + private readonly hashEpochs = new Map() + private readonly pendingHashReads = new Map() + private readonly pullRequestRefs = new Map>() private readonly timeoutMs: number constructor( private readonly db: Database, private readonly openCodeClient: OpenCodeClient, + private readonly settingsService: SettingsService, + private readonly gitAuthService: GitAuthService, + private readonly gitService: GitService, options: ChangeWalkthroughServiceOptions = {}, ) { this.timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS } - async getState(sessionId: string): Promise { - await this.readSession(sessionId) + async getState( + sessionId: string, + source: WalkthroughSource = DEFAULT_WALKTHROUGH_SOURCE, + hunksFor?: string, + ): Promise { + const session = await this.readSession(sessionId) + const key = entryKey(sessionId, source) + + const currentDiffHash = this.inFlight.has(key) + ? this.currentHashes.get(key) ?? null + : await this.readCurrentDiffHash(session, source, key) + + const entry = this.inFlight.get(key) + const walkthrough = + entry?.walkthrough ?? getChangeWalkthrough(this.db, sessionId, walkthroughSourceKey(source)) + return { + walkthrough: this.omitHunks(walkthrough, hunksFor), + currentDiffHash, + stale: walkthrough !== null && currentDiffHash !== null && walkthrough.diffHash !== currentDiffHash, + generating: this.inFlight.has(key), + error: this.failures.get(key) ?? null, + } + } + + private omitHunks( + walkthrough: ChangeWalkthrough | null, + hunksFor: string | undefined, + ): ChangeWalkthroughWire | null { + if (!walkthrough || hunksFor === undefined || walkthroughHunksIdentity(walkthrough) !== hunksFor) { + return walkthrough + } + return { ...walkthrough, hunks: undefined } + } + + private async readCurrentDiffHash( + session: SessionInfo, + source: WalkthroughSource, + key: string, + ): Promise { + if (source.kind === 'session') { + const cached = this.currentHashes.get(key) + if (cached !== undefined) { + return cached + } + } - let currentDiffHash: string | null = null + const epoch = this.hashEpochs.get(key) ?? 0 + this.pendingHashReads.set(key, (this.pendingHashReads.get(key) ?? 0) + 1) try { - const changes = await readSessionChanges(this.openCodeClient, sessionId) - currentDiffHash = computeChangesHash(changes) + const changes = await readWalkthroughChanges({ + client: this.openCodeClient, + session, + source, + gitEnv: this.gitAuthService.getGitEnvironment(true), + }) + const hash = computeChangesHash(changes) + if ((this.hashEpochs.get(key) ?? 0) === epoch) { + this.currentHashes.set(key, hash) + } + return hash } catch { - currentDiffHash = null + return null + } finally { + this.releaseHashRead(key) } + } - const walkthrough = getChangeWalkthrough(this.db, sessionId) - return { - walkthrough, - currentDiffHash, - stale: walkthrough !== null && currentDiffHash !== null && walkthrough.diffHash !== currentDiffHash, - generating: this.inFlight.has(sessionId), - error: this.failures.get(sessionId) ?? null, + private invalidateCurrentHash(key: string): void { + this.currentHashes.delete(key) + if ((this.pendingHashReads.get(key) ?? 0) > 0) { + this.hashEpochs.set(key, (this.hashEpochs.get(key) ?? 0) + 1) + return + } + this.hashEpochs.delete(key) + } + + private releaseHashRead(key: string): void { + const remaining = (this.pendingHashReads.get(key) ?? 1) - 1 + if (remaining > 0) { + this.pendingHashReads.set(key, remaining) + return + } + this.pendingHashReads.delete(key) + if (!this.currentHashes.has(key)) { + this.hashEpochs.delete(key) } } - generate(sessionId: string, request: GenerateChangeWalkthroughRequest): Promise<{ walkthrough: ChangeWalkthrough; created: boolean }> { + generate( + sessionId: string, + request: GenerateChangeWalkthroughRequest, + ): Promise<{ walkthrough: ChangeWalkthrough; created: boolean }> { return this.begin(sessionId, request).result } - async startGeneration(sessionId: string, request: GenerateChangeWalkthroughRequest): Promise { + async startGeneration(sessionId: string, request: GenerateChangeWalkthroughRequest): Promise { const entry = this.begin(sessionId, request) await Promise.race([entry.modelStarted, entry.result]) - return this.getState(sessionId) + return this.getState(sessionId, request.source ?? DEFAULT_WALKTHROUGH_SOURCE) } - /** Removes the stored walkthrough of a deleted session, including one still being generated. */ + /** Removes the stored walkthroughs of a deleted session, including one still being generated. */ handleEvent(event: SSEEvent): void { - if (event.type !== 'session.deleted') { + switch (event.type) { + case 'session.deleted': { + const { sessionID } = event.data + const prefix = `${sessionID}\0` + for (const key of this.inFlight.keys()) { + if (key.startsWith(prefix)) { + this.deletedDuringGeneration.add(key) + } + } + this.clearSessionState(sessionID) + this.deleteSessionPullRequestRefs(sessionID) + deleteChangeWalkthroughs(this.db, sessionID) + return + } + case 'session.execution.started': + case 'session.execution.succeeded': + case 'session.execution.failed': + case 'session.execution.interrupted': + case 'session.revert.staged': + case 'session.revert.committed': + case 'session.revert.cleared': + this.invalidateCurrentHash(entryKey(event.data.sessionID, DEFAULT_WALKTHROUGH_SOURCE)) + return + default: + return + } + } + + private clearSessionState(sessionId: string): void { + const prefix = `${sessionId}\0` + const keys = new Set([ + ...this.inFlight.keys(), + ...this.failures.keys(), + ...this.currentHashes.keys(), + ...this.hashEpochs.keys(), + ...this.pendingHashReads.keys(), + ]) + for (const key of keys) { + if (!key.startsWith(prefix)) { + continue + } + this.inFlight.delete(key) + this.failures.delete(key) + this.invalidateCurrentHash(key) + } + } + + private recordPullRequestRef(sessionId: string, directory: string, ref: string): void { + const entry = `${directory}\0${ref}` + const existing = this.pullRequestRefs.get(sessionId) + if (existing) { + existing.add(entry) return } + this.pullRequestRefs.set(sessionId, new Set([entry])) + } - const { sessionID } = event.data - if (this.inFlight.has(sessionID)) { - this.deletedDuringGeneration.add(sessionID) + private deleteSessionPullRequestRefs(sessionId: string): void { + const entries = this.pullRequestRefs.get(sessionId) + if (!entries) { + return + } + this.pullRequestRefs.delete(sessionId) + const gitEnv = this.gitAuthService.getGitEnvironment(true) + for (const entry of entries) { + const separator = entry.indexOf('\0') + const directory = entry.slice(0, separator) + const ref = entry.slice(separator + 1) + void deletePullRequestRef(directory, ref, gitEnv).catch(() => {}) } - this.failures.delete(sessionID) - deleteChangeWalkthrough(this.db, sessionID) } private begin(sessionId: string, request: GenerateChangeWalkthroughRequest): InFlightGeneration { - const existing = this.inFlight.get(sessionId) + const source = request.source ?? DEFAULT_WALKTHROUGH_SOURCE + const key = entryKey(sessionId, source) + const existing = this.inFlight.get(key) if (existing) { return existing } - this.failures.delete(sessionId) + this.failures.delete(key) let resolveModelStarted: () => void = () => {} const modelStarted = new Promise((resolve) => { @@ -307,110 +910,380 @@ export class ChangeWalkthroughService { resolveModelStarted() } - const result = this.runGenerate(sessionId, request, markModelStarted) + const entry = { modelStarted, walkthrough: null } as InFlightGeneration + this.inFlight.set(key, entry) + + entry.result = this.runGenerate(sessionId, source, request, markModelStarted, entry) .catch((error) => { if (modelCalled) { - this.failures.set(sessionId, toGenerationError(error)) + this.failures.set(key, toGenerationError(error)) } throw error }) .finally(() => { - this.inFlight.delete(sessionId) - this.deletedDuringGeneration.delete(sessionId) + this.inFlight.delete(key) + this.deletedDuringGeneration.delete(key) + this.invalidateCurrentHash(key) }) - result.catch(() => {}) + entry.result.catch(() => {}) - const entry: InFlightGeneration = { modelStarted, result } - this.inFlight.set(sessionId, entry) return entry } + private async fetchPullRequestChanges( + session: SessionInfo, + source: Extract, + ): Promise { + const directory = session.location.directory + const repo = await resolveRepoForDirectory(this.db, directory) + const target = repo + ? { id: repo.id, repoUrl: repo.repoUrl, fullPath: repo.fullPath } + : { fullPath: directory } + const refs = await fetchPullRequestRef( + directory, + session.id, + source, + this.gitAuthService.getGitEnvironment(true), + (remote, refspecs, timeoutMs) => + this.gitService.fetchRemoteRef(target, remote, refspecs, this.db, timeoutMs), + ) + for (const ref of refs) { + this.recordPullRequestRef(session.id, directory, ref) + } + } + private async runGenerate( sessionId: string, + source: WalkthroughSource, request: GenerateChangeWalkthroughRequest, onModelStart: () => void, + entry: InFlightGeneration, ): Promise<{ walkthrough: ChangeWalkthrough; created: boolean }> { const session = await this.readSession(sessionId) + const key = entryKey(sessionId, source) + + if (source.kind === 'pullRequest') { + await this.fetchPullRequestChanges(session, source) + } let changes: FileDiffInfo[] try { - changes = await readSessionChanges(this.openCodeClient, sessionId) + changes = await readWalkthroughChanges({ + client: this.openCodeClient, + session, + source, + gitEnv: this.gitAuthService.getGitEnvironment(true), + }) } catch (error) { + if (error instanceof ChangeWalkthroughError) { + throw error + } throw new ChangeWalkthroughError(getErrorMessage(error) || 'Failed to read session changes', 502, { code: 'WALKTHROUGH_CHANGES_UNAVAILABLE', }) } if (changes.length === 0) { - throw new ChangeWalkthroughError('This session has no changes to walk through', 409, { + throw new ChangeWalkthroughError(noChangesMessage(source), 409, { code: 'WALKTHROUGH_NO_CHANGES', }) } const diffHash = computeChangesHash(changes) - const stored = getChangeWalkthrough(this.db, sessionId) - if (stored && stored.diffHash === diffHash && !request.regenerate) { - return { walkthrough: stored, created: false } + this.currentHashes.set(key, diffHash) + + const previous = getChangeWalkthrough(this.db, sessionId, walkthroughSourceKey(source)) + const title = walkthroughTitle(session, source) + + if (previous && previous.diffHash === diffHash && !request.regenerate) { + if (previous.stops.every((stop) => stop.status === 'ready')) { + return { walkthrough: previous, created: false } + } + + const model = await this.resolveWalkthroughModel(session) + const storedModel = formatOpenCodeModelRef(model) + const resumingStops = previous.stops.map((stop) => + stop.status === 'ready' ? stop : { ...stop, status: 'pending' as const }, + ) + const resuming: ChangeWalkthrough = { ...previous, model: storedModel, stops: resumingStops } + if (this.deletedDuringGeneration.has(key)) { + throw new ChangeWalkthroughError('Session not found', 404) + } + entry.walkthrough = resuming + + const resumed = await this.explainStops( + model, + title, + previous.summary, + resumingStops.filter((stop) => stop.status !== 'ready'), + previous.hunks, + resuming, + onModelStart, + entry, + ) + if (this.deletedDuringGeneration.has(key)) { + throw new ChangeWalkthroughError('Session not found', 404) + } + return { walkthrough: resumed, created: true } + } + + const model = await this.resolveWalkthroughModel(session) + const modelKey = computeModelKey(model) + const storedModel = formatOpenCodeModelRef(model) + + const input = buildWalkthroughInput(changes) + const { hunks, mechanicalHunks, omittedFiles } = input + if (hunks.length === 0 && mechanicalHunks.length === 0) { + throw new ChangeWalkthroughError(noTextChangesMessage(source), 409, { + code: 'WALKTHROUGH_NO_TEXT_CHANGES', + }) + } + + const mechanicalStop = mechanicalHunks.length > 0 ? buildMechanicalStop(mechanicalHunks) : null + + const store = (fields: { + summary: string + stops: WalkthroughStop[] + hunks: WalkthroughHunk[] + omittedFiles: WalkthroughOmittedFile[] + }): ChangeWalkthrough => { + const walkthrough: ChangeWalkthrough = { + sessionId, + source, + diffHash, + model: storedModel, + summary: fields.summary, + stops: fields.stops, + hunks: fields.hunks, + omittedFiles: fields.omittedFiles, + createdAt: Date.now(), + } + + if (!this.saveIfSessionLive(walkthrough)) { + throw new ChangeWalkthroughError('Session not found', 404) + } + return walkthrough } - const title = session.title ?? sessionId - const { hunks, omittedFiles } = buildWalkthroughInput(changes, title) - if (hunks.length === 0) { - if (omittedFiles.some((file) => file.reason === 'budget')) { + const reusable = new Map() + if (!request.regenerate && previous) { + for (const stop of previous.stops) { + if (stop.status === 'ready' && stop.explanationKey) { + reusable.set(stop.explanationKey, stop.explanation) + } + } + } + + const inputHunkIds = new Set(hunks.map((hunk) => hunk.id)) + const previousStops = + !request.regenerate && previous + ? previous.stops + .filter((stop) => stop.hunkIds.length > 0 && stop.hunkIds.every((id) => inputHunkIds.has(id))) + .map((stop) => ({ title: stop.title, hunkIds: stop.hunkIds })) + : [] + + const outline = buildWalkthroughOutline(input, title, previousStops) + if (hunks.length === 0 || outline.hunks.length === 0) { + if (!mechanicalStop) { throw new ChangeWalkthroughError('These changes are too large to walk through', 413, { code: 'WALKTHROUGH_CONTEXT_LIMIT', - details: { omittedFiles }, + details: { omittedFiles: outline.omittedFiles }, }) } - throw new ChangeWalkthroughError('This session has no text changes to walk through', 409, { - code: 'WALKTHROUGH_NO_TEXT_CHANGES', - }) + + return { + walkthrough: store({ + summary: + hunks.length === 0 && outline.omittedFiles.every((file) => isMechanicalChangePath(file.file)) + ? MECHANICAL_ONLY_SUMMARY + : MECHANICAL_PARTIAL_SUMMARY, + stops: [mechanicalStop], + hunks: mechanicalHunks, + omittedFiles: outline.omittedFiles, + }), + created: true, + } + } + + if (fitsSingleCall(input, title)) { + const prompt = buildWalkthroughPrompt({ title, hunks }) + onModelStart() + + const parsed = await this.callModelParsed(prompt, model, (text) => + parseWalkthroughResponse(text, hunks), + ) + + const stops = parsed.stops.map((stop) => applySingleCallKeys(stop, modelKey)) + return { + walkthrough: store({ + summary: parsed.summary, + stops: mechanicalStop ? [...stops, mechanicalStop] : stops, + hunks: [...hunks, ...mechanicalHunks], + omittedFiles, + }), + created: true, + } } - const prompt = buildWalkthroughPrompt({ title, hunks }) + const outlineIds = new Set(outline.hunks.map((hunk) => hunk.id)) + const planPreviousStops = previousStops.filter((stop) => + stop.hunkIds.every((id) => outlineIds.has(id)), + ) + const planPrompt = buildWalkthroughPlanPrompt({ + title, + outline: outline.outline, + previousStops: planPreviousStops, + }) onModelStart() - let responseText: string - try { - responseText = await generateTextWithTimeout( - this.openCodeClient, - { prompt, model: session.model }, - this.timeoutMs, + const planned = await this.callModelParsed(planPrompt, model, (text) => + parseWalkthroughPlan(text, outline.hunks), + ) + + const stops = planned.stops.map((stop) => applyReuse(stop, reusable, modelKey)) + let walkthrough: ChangeWalkthrough = store({ + summary: planned.summary, + stops: mechanicalStop ? [...stops, mechanicalStop] : stops, + hunks: [...outline.hunks, ...mechanicalHunks], + omittedFiles: outline.omittedFiles, + }) + entry.walkthrough = walkthrough + + const pending = stops.filter((stop) => stop.status !== 'ready') + if (pending.length > 0) { + walkthrough = await this.explainStops( + model, + title, + planned.summary, + pending, + outline.hunks, + walkthrough, + onModelStart, + entry, ) - } catch (error) { - if (error instanceof GenerateTextTimeoutError) { - throw new ChangeWalkthroughError('Generating the change walkthrough timed out', 504, { - code: 'WALKTHROUGH_TIMEOUT', + } + + if (this.deletedDuringGeneration.has(key)) { + throw new ChangeWalkthroughError('Session not found', 404) + } + + return { walkthrough, created: true } + } + + private async explainStops( + model: ModelRef, + title: string, + summary: string, + stops: WalkthroughStop[], + hunks: WalkthroughHunk[], + walkthrough: ChangeWalkthrough, + onModelStart: () => void, + entry: InFlightGeneration, + ): Promise { + const modelKey = computeModelKey(model) + const key = entryKey(walkthrough.sessionId, walkthrough.source) + const total = stops.length + let completed = 0 + let current = walkthrough + + await mapWithConcurrency(stops, WALKTHROUGH_EXPLAIN_CONCURRENCY, async (stop) => { + const explainPrompt = buildWalkthroughExplainPrompt({ title, summary, stop, hunks }) + onModelStart() + + let explained: WalkthroughStop + try { + const explanation = await this.callModelParsed( + explainPrompt, + model, + parseWalkthroughExplanation, + () => this.deletedDuringGeneration.has(key), + ) + explained = { + ...stop, + explanation, + status: 'ready', + explanationKey: computeExplanationKey(modelKey, stop.hunkIds), + } + } catch (error) { + if (error instanceof WalkthroughCallSkippedError) { + return + } + logger.warn('Failed to explain a change walkthrough stop', getErrorMessage(error)) + explained = { ...stop, status: 'failed' } + } + + current = this.replaceStop(current, explained) + entry.walkthrough = current + completed += 1 + if (completed % WALKTHROUGH_SAVE_EVERY_STOPS === 0 || completed === total) { + this.saveIfSessionLive(current) + } + }) + + return current + } + + private async callModelParsed( + prompt: string, + model: ModelRef, + parse: (text: string) => T | null, + shouldSkip?: () => boolean, + ): Promise { + let lastError = new ChangeWalkthroughError('Failed to generate the change walkthrough', 502) + + for (let attempt = 0; attempt < WALKTHROUGH_CALL_ATTEMPTS; attempt += 1) { + let text: string + try { + text = await WALKTHROUGH_MODEL_LIMITER.run(() => { + if (shouldSkip?.()) { + throw new WalkthroughCallSkippedError() + } + return generateTextWithTimeout(this.openCodeClient, { prompt, model }, this.timeoutMs) }) + } catch (error) { + if (error instanceof WalkthroughCallSkippedError) { + throw error + } + lastError = + error instanceof GenerateTextTimeoutError + ? new ChangeWalkthroughError('Generating the change walkthrough timed out', 504, { + code: 'WALKTHROUGH_TIMEOUT', + }) + : new ChangeWalkthroughError( + getErrorMessage(error) || 'Failed to generate the change walkthrough', + 502, + ) + continue } - throw new ChangeWalkthroughError(getErrorMessage(error) || 'Failed to generate the change walkthrough', 502) - } - const parsed = parseWalkthroughResponse(responseText, hunks) - if (!parsed) { - throw new ChangeWalkthroughError('The model did not return a usable change walkthrough', 502, { + const parsed = parse(text) + if (parsed !== null) { + return parsed + } + + lastError = new ChangeWalkthroughError('The model did not return a usable change walkthrough', 502, { code: 'WALKTHROUGH_UNPARSEABLE', }) } - const walkthrough: ChangeWalkthrough = { - sessionId, - diffHash, - summary: parsed.summary, - stops: parsed.stops, - hunks, - omittedFiles, - createdAt: Date.now(), - } + throw lastError + } - if (this.deletedDuringGeneration.has(sessionId)) { - throw new ChangeWalkthroughError('Session not found', 404) + private replaceStop(walkthrough: ChangeWalkthrough, stop: WalkthroughStop): ChangeWalkthrough { + return { + ...walkthrough, + stops: walkthrough.stops.map((existing) => (existing.id === stop.id ? stop : existing)), } + } + private saveIfSessionLive(walkthrough: ChangeWalkthrough): boolean { + if (this.deletedDuringGeneration.has(entryKey(walkthrough.sessionId, walkthrough.source))) { + return false + } saveChangeWalkthrough(this.db, walkthrough) - return { walkthrough, created: true } + return true } private async readSession(sessionId: string): Promise { @@ -423,4 +1296,28 @@ export class ChangeWalkthroughService { throw new ChangeWalkthroughError(getErrorMessage(error) || 'Failed to read session', 502) } } + + private async resolveWalkthroughModel(session: SessionInfo): Promise { + const configured = this.settingsService.getSettings().preferences.walkthroughModel?.trim() + const parsed = configured ? parseOpenCodeModelRef(configured) : undefined + const preferred = parsed ?? session.model + if (preferred) { + return preferred + } + + let resolved + try { + resolved = await resolveOpenCodeModel(this.openCodeClient, getOpenCodeGlobalConfigPath()) + } catch (error) { + throw new ChangeWalkthroughError( + getErrorMessage(error) || 'Failed to resolve the walkthrough model', + 502, + { code: 'WALKTHROUGH_MODEL_UNAVAILABLE' }, + ) + } + + return resolved.variant + ? { providerID: resolved.providerID, id: resolved.id, variant: resolved.variant } + : { providerID: resolved.providerID, id: resolved.id } + } } diff --git a/backend/src/services/git/GitService.ts b/backend/src/services/git/GitService.ts index 907ff8878..9d2ff5749 100644 --- a/backend/src/services/git/GitService.ts +++ b/backend/src/services/git/GitService.ts @@ -738,6 +738,26 @@ export class GitService { ) } + async fetchRemoteRef( + repo: { id?: number; repoUrl?: string; fullPath: string }, + remote: string, + refspecs: string[], + database: Database, + timeoutMs?: number, + ): Promise { + await this.withRemoteAuth( + repo, + async (env) => { + await executeCommand(['git', '-C', repo.fullPath, 'fetch', '--no-tags', remote, ...refspecs], { + env, + silent: true, + timeout: timeoutMs, + }) + }, + { database, silent: true } + ) + } + async pull(repoId: number, database: Database): Promise { const repo = getRepoById(database, repoId) if (!repo) { diff --git a/backend/src/services/repo.ts b/backend/src/services/repo.ts index 4b8ba817f..c15d869d1 100644 --- a/backend/src/services/repo.ts +++ b/backend/src/services/repo.ts @@ -1019,6 +1019,18 @@ export async function resolveDefaultBranch(repoPath: string, env: Record 'main') } +export async function resolveBaseRef(repoPath: string, base: string, env: Record): Promise { + for (const candidate of [`refs/remotes/origin/${base}`, `refs/heads/${base}`]) { + try { + await executeCommand(['git', '-C', repoPath, 'rev-parse', '--verify', candidate], { env, silent: true }) + return candidate.startsWith('refs/remotes/') ? `origin/${base}` : base + } catch { + continue + } + } + return null +} + export async function removeWorktree( baseRepoPath: string, worktreePath: string, diff --git a/backend/src/services/schedule-worktree.ts b/backend/src/services/schedule-worktree.ts index 118d50add..8a5ca4fab 100644 --- a/backend/src/services/schedule-worktree.ts +++ b/backend/src/services/schedule-worktree.ts @@ -8,7 +8,7 @@ import type { Repo } from '../types/repo' import type { GitAuthService } from './git-auth' import { isSSHUrl } from '@opencode-manager/shared/utils' import { executeCommand } from '../utils/process' -import { resolveDefaultBranch, createWorktreeSafely, listGitWorktrees, removeWorktree } from './repo' +import { resolveDefaultBranch, resolveBaseRef, createWorktreeSafely, listGitWorktrees, removeWorktree } from './repo' import { logger } from '../utils/logger' import { canonicalPathSync, mkdirSyncSafe } from '../utils/fs-safe' import { @@ -95,7 +95,7 @@ export class ScheduleWorktreeManager { await executeCommand(['git', '-C', repo.fullPath, 'fetch', '--prune', 'origin'], { env }).catch(() => {}) const base = job.branch?.trim() || (await resolveDefaultBranch(repo.fullPath, env)) - const baseRef = await this.resolveBaseRef(repo.fullPath, base, env) + const baseRef = await resolveBaseRef(repo.fullPath, base, env) if (!baseRef) { throw new Error(`Base branch "${base}" was not found in this repository. Choose an existing branch in the schedule settings.`) } @@ -319,24 +319,6 @@ export class ScheduleWorktreeManager { } } - /** - * Resolves a user-supplied base branch name to a verified git ref, preferring - * the remote-tracking branch for freshness. Returns null when neither the - * remote nor local ref exists, allowing the caller to fail with a clear error - * instead of a cryptic git "not a valid object name" failure. - */ - private async resolveBaseRef(repoPath: string, base: string, env: Record): Promise { - for (const candidate of [`refs/remotes/origin/${base}`, `refs/heads/${base}`]) { - try { - await executeCommand(['git', '-C', repoPath, 'rev-parse', '--verify', candidate], { env, silent: true }) - return candidate.startsWith('refs/remotes/') ? `origin/${base}` : base - } catch { - continue - } - } - return null - } - private async buildGitEnv(repo: Repo, sshSetup: boolean, silent: boolean): Promise> { const baseEnv = this.gitAuthService.getGitEnvironment(silent) const sshEnv = sshSetup ? this.gitAuthService.getSSHEnvironment() : {} diff --git a/backend/src/services/session-goal-audit.ts b/backend/src/services/session-goal-audit.ts index 92f31a56b..824b03008 100644 --- a/backend/src/services/session-goal-audit.ts +++ b/backend/src/services/session-goal-audit.ts @@ -1,5 +1,6 @@ +import { z } from 'zod' import { SessionGoalVerdictSchema, type SessionGoalVerdict } from '@opencode-manager/shared/schemas' -import { extractFirstJsonObject } from '../utils/json-extract' +import { parseFirstJsonObject } from '../utils/json-extract' import { truncateSessionReply } from './session-reply' export const GOAL_AUDIT_REASON_MAX_CHARS = 500 @@ -51,39 +52,23 @@ export function buildGoalContinuationPrompt({ objective, reason }: GoalContinuat ].join('\n') } -export function parseGoalVerdict(text: string): GoalVerdict | null { - const block = extractFirstJsonObject(text) - if (!block) { - return null - } +const goalVerdictSchema = z.object({ + verdict: SessionGoalVerdictSchema, + reason: z.string(), +}) - let parsed: unknown - try { - parsed = JSON.parse(block) - } catch { - return null - } - - if (typeof parsed !== 'object' || parsed === null) { - return null - } - - const candidate = parsed as { verdict?: unknown; reason?: unknown } - const verdict = SessionGoalVerdictSchema.safeParse(candidate.verdict) - if (!verdict.success) { - return null - } - - if (typeof candidate.reason !== 'string') { +export function parseGoalVerdict(text: string): GoalVerdict | null { + const parsed = parseFirstJsonObject(text, goalVerdictSchema) + if (!parsed) { return null } - const reason = candidate.reason.trim() + const reason = parsed.reason.trim() if (!reason) { return null } - return { verdict: verdict.data, reason: reason.slice(0, GOAL_AUDIT_REASON_MAX_CHARS) } + return { verdict: parsed.verdict, reason: reason.slice(0, GOAL_AUDIT_REASON_MAX_CHARS) } } function truncateReply(reply: string | null): string { diff --git a/backend/src/services/walkthrough-sources.ts b/backend/src/services/walkthrough-sources.ts new file mode 100644 index 000000000..a27cef8f5 --- /dev/null +++ b/backend/src/services/walkthrough-sources.ts @@ -0,0 +1,309 @@ +import { copyFileSync, existsSync, mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import type { FileDiffInfo, SessionInfo } from '@opencode-manager/shared/opencode' +import type { WalkthroughSource } from '@opencode-manager/shared/schemas' +import { splitUnifiedDiffByFile } from '@opencode-manager/shared/utils' +import { getErrorMessage } from '../utils/error-utils' +import { executeCommand } from '../utils/process' +import { ChangeWalkthroughError } from './change-walkthrough-error' +import type { OpenCodeClient } from './opencode/client' +import { resolveBaseRef, resolveDefaultBranch } from './repo' +import { readSessionChanges } from './session-changes' + +export interface ReadWalkthroughChangesOptions { + client: OpenCodeClient + session: SessionInfo + source: WalkthroughSource + gitEnv?: Record +} + +type GitWalkthroughSource = Exclude + +const GIT_DIFF_TIMEOUT_MS = 30_000 +const GIT_FETCH_TIMEOUT_MS = 60_000 +const GIT_DIFF_MAX_OUTPUT_CHARS = 5_000_000 +const EMPTY_TREE = '4b825dc642cb6eb9a060e54bf8d69288fbee4904' +const DIFF_ARGS = ['--no-color', '--src-prefix=a/', '--dst-prefix=b/', '--no-ext-diff', '--no-textconv', '-U3'] +const PULL_REQUEST_REMOTE_PREFERENCE = ['upstream', 'origin'] + +type PullRequestSource = Extract + +export interface PullRequestRefs { + remote: string + baseBranch: string + headRef: string + baseRef: string +} + +export interface PullRequestFetchPlan { + remote: string + refspecs: string[] + headRef: string + baseRef: string +} + +export type PullRequestFetchRunner = ( + remote: string, + refspecs: string[], + timeoutMs: number, +) => Promise + +export async function resolvePullRequestRefs( + directory: string, + sessionId: string, + source: PullRequestSource, + gitEnv?: Record, +): Promise { + const remotes = (await tryGit(directory, ['remote'], gitEnv))?.split('\n').map((line) => line.trim()) ?? [] + const remote = PULL_REQUEST_REMOTE_PREFERENCE.find((candidate) => remotes.includes(candidate)) ?? 'origin' + const baseBranch = source.base ?? await resolveRemoteDefaultBranch(directory, remote, gitEnv) + return { + remote, + baseBranch, + headRef: `refs/ocm-walkthrough/${sessionId}/pr/${source.number}/head`, + baseRef: `refs/ocm-walkthrough/${sessionId}/pr/${source.number}/base/${baseBranch}`, + } +} + +export async function planPullRequestFetch( + directory: string, + sessionId: string, + source: PullRequestSource, + gitEnv?: Record, +): Promise { + const { remote, baseBranch, headRef, baseRef } = await resolvePullRequestRefs(directory, sessionId, source, gitEnv) + return { + remote, + refspecs: [ + `+refs/pull/${source.number}/head:${headRef}`, + `+refs/heads/${baseBranch}:${baseRef}`, + ], + headRef, + baseRef, + } +} + +export async function fetchPullRequestRef( + directory: string, + sessionId: string, + source: PullRequestSource, + gitEnv: Record | undefined, + runFetch: PullRequestFetchRunner, +): Promise { + const plan = await planPullRequestFetch(directory, sessionId, source, gitEnv) + await mapGitError( + () => runFetch(plan.remote, plan.refspecs, GIT_FETCH_TIMEOUT_MS), + 'Failed to fetch the pull request', + ) + return [plan.headRef, plan.baseRef] +} + +async function resolveRemoteDefaultBranch( + directory: string, + remote: string, + gitEnv?: Record, +): Promise { + const prefix = `${remote}/` + const ref = (await tryGit(directory, ['rev-parse', '--abbrev-ref', `${remote}/HEAD`], gitEnv))?.trim() + if (ref && ref.startsWith(prefix)) { + return ref.slice(prefix.length) + } + return resolveDefaultBranch(directory, gitEnv ?? {}) +} + +export async function deletePullRequestRef( + directory: string, + ref: string, + gitEnv?: Record, +): Promise { + try { + await executeCommand(['git', '-C', directory, 'update-ref', '-d', ref], { + env: gitEnv, + silent: true, + ignoreExitCode: true, + timeout: GIT_DIFF_TIMEOUT_MS, + }) + } catch { + return + } +} + +export async function readWalkthroughChanges({ + client, + session, + source, + gitEnv, +}: ReadWalkthroughChangesOptions): Promise { + if (source.kind === 'session') { + return readSessionChanges(client, session.id) + } + return readGitChanges(session.location.directory, session.id, source, gitEnv) +} + +async function readGitChanges( + directory: string, + sessionId: string, + source: GitWalkthroughSource, + gitEnv?: Record, +): Promise { + await assertGitRepository(directory, gitEnv) + const repoRoot = await resolveRepoRoot(directory, gitEnv) + + switch (source.kind) { + case 'uncommitted': + return collectChanges(repoRoot, await uncommittedRevision(repoRoot, gitEnv), true, gitEnv) + case 'staged': + return collectChanges(repoRoot, ['--cached'], false, gitEnv) + case 'unstaged': + return collectChanges(repoRoot, [], true, gitEnv) + case 'branch': { + const base = await resolveBase(repoRoot, source.base, gitEnv) + return collectChanges(repoRoot, [`${base}...HEAD`], false, gitEnv) + } + case 'pullRequest': { + const { headRef, baseRef } = await resolvePullRequestRefs(repoRoot, sessionId, source, gitEnv) + return collectChanges(repoRoot, [`${baseRef}...${headRef}`], false, gitEnv) + } + } +} + +async function resolveBase(directory: string, base: string | undefined, gitEnv?: Record): Promise { + const resolved = await resolveBaseRef( + directory, + base ?? await resolveDefaultBranch(directory, gitEnv ?? {}), + gitEnv ?? {}, + ) + if (!resolved) { + throw new ChangeWalkthroughError('No base branch found', 409, { code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + } + return resolved +} + +async function uncommittedRevision(directory: string, gitEnv?: Record): Promise { + return (await headExists(directory, gitEnv)) ? ['HEAD'] : [EMPTY_TREE] +} + +async function collectChanges( + directory: string, + revisionArgs: string[], + includeUntracked: boolean, + gitEnv?: Record, +): Promise { + const tracked = await runDiff(directory, revisionArgs, gitEnv) + if (!includeUntracked) { + return splitUnifiedDiffByFile(tracked) + } + const untracked = await readUntrackedDiff(directory, gitEnv) + return splitUnifiedDiffByFile(`${tracked}${untracked}`) +} + +async function readUntrackedDiff(directory: string, gitEnv?: Record): Promise { + const untracked = await runGit(directory, ['ls-files', '--others', '--exclude-standard', '-z'], gitEnv) + if (untracked.length === 0) { + return '' + } + + const tempDir = mkdtempSync(path.join(tmpdir(), 'ocm-walkthrough-index-')) + const tempIndex = path.join(tempDir, 'index') + const env = { ...gitEnv, GIT_INDEX_FILE: tempIndex } + + try { + const indexPath = (await tryGit(directory, ['rev-parse', '--git-path', 'index'], gitEnv))?.trim() + if (indexPath) { + const realIndex = path.resolve(directory, indexPath) + if (existsSync(realIndex)) { + copyFileSync(realIndex, tempIndex) + } + } + await runGit(directory, ['add', '-N', '-A'], env) + return await runDiff(directory, ['--diff-filter=A', '--no-renames'], env) + } finally { + rmSync(tempDir, { recursive: true, force: true }) + } +} + +async function runDiff(directory: string, revisionArgs: string[], gitEnv?: Record): Promise { + const result = await mapGitError( + () => executeCommand( + ['git', '-C', directory, 'diff', ...DIFF_ARGS, ...revisionArgs], + { + env: gitEnv, + silent: true, + ignoreExitCode: true, + timeout: GIT_DIFF_TIMEOUT_MS, + maxOutputChars: GIT_DIFF_MAX_OUTPUT_CHARS, + }, + ), + 'git diff failed', + ) + if (result.truncated) { + throw new ChangeWalkthroughError('The diff is too large to display', 413, { code: 'WALKTHROUGH_DIFF_TOO_LARGE' }) + } + if (result.exitCode !== 0) { + throw new ChangeWalkthroughError(result.stderr.trim() || 'git diff failed', 502, { code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + } + return result.stdout +} + +async function assertGitRepository(directory: string, gitEnv?: Record): Promise { + let insideWorkTree = false + try { + insideWorkTree = (await runGit(directory, ['rev-parse', '--is-inside-work-tree'], gitEnv)).trim() === 'true' + } catch { + insideWorkTree = false + } + if (!insideWorkTree) { + throw new ChangeWalkthroughError('Not a git repository', 409, { code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + } +} + +async function resolveRepoRoot(directory: string, gitEnv?: Record): Promise { + const root = await tryGit(directory, ['rev-parse', '--show-toplevel'], gitEnv) + return root ? root.trim() : directory +} + +async function headExists(directory: string, gitEnv?: Record): Promise { + return (await tryGit(directory, ['rev-parse', '--verify', '--quiet', 'HEAD'], gitEnv)) !== null +} + +async function tryGit(directory: string, args: string[], gitEnv?: Record): Promise { + const result = await executeCommand(['git', '-C', directory, ...args], { + env: gitEnv, + silent: true, + ignoreExitCode: true, + timeout: GIT_DIFF_TIMEOUT_MS, + maxOutputChars: GIT_DIFF_MAX_OUTPUT_CHARS, + }) + return result.exitCode === 0 ? result.stdout : null +} + +async function runGit( + directory: string, + args: string[], + gitEnv?: Record, + timeout = GIT_DIFF_TIMEOUT_MS, + fallback = 'Git command failed', +): Promise { + return mapGitError( + () => executeCommand(['git', '-C', directory, ...args], { + env: gitEnv, + silent: true, + timeout, + maxOutputChars: GIT_DIFF_MAX_OUTPUT_CHARS, + }), + fallback, + ) +} + +async function mapGitError(run: () => Promise, fallback: string): Promise { + try { + return await run() + } catch (error) { + const message = getErrorMessage(error) + if (message.includes('timed out')) { + throw new ChangeWalkthroughError('git timed out', 504, { code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + } + throw new ChangeWalkthroughError(message || fallback, 502, { code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + } +} diff --git a/backend/src/utils/concurrency.ts b/backend/src/utils/concurrency.ts new file mode 100644 index 000000000..e071dbfc1 --- /dev/null +++ b/backend/src/utils/concurrency.ts @@ -0,0 +1,67 @@ +export async function mapWithConcurrency( + items: T[], + limit: number, + fn: (item: T, index: number) => Promise, +): Promise { + const results = new Array(items.length) + const workerCount = Math.max(1, Math.min(limit, items.length)) + let nextIndex = 0 + + const worker = async (): Promise => { + while (true) { + const index = nextIndex + nextIndex += 1 + if (index >= items.length) { + return + } + results[index] = await fn(items[index]!, index) + } + } + + await Promise.all(Array.from({ length: workerCount }, () => worker())) + return results +} + +export interface Limiter { + readonly activeCount: number + run(task: () => Promise): Promise +} + +export function createLimiter(limit: number): Limiter { + const max = Math.max(1, limit) + let active = 0 + const waiters: Array<() => void> = [] + + const acquire = (): Promise => { + if (active < max) { + active += 1 + return Promise.resolve() + } + return new Promise((resolve) => { + waiters.push(resolve) + }) + } + + const release = (): void => { + const next = waiters.shift() + if (next) { + next() + return + } + active -= 1 + } + + return { + get activeCount(): number { + return active + }, + async run(task: () => Promise): Promise { + await acquire() + try { + return await task() + } finally { + release() + } + }, + } +} diff --git a/backend/src/utils/json-extract.ts b/backend/src/utils/json-extract.ts index dec69113e..18772e0ba 100644 --- a/backend/src/utils/json-extract.ts +++ b/backend/src/utils/json-extract.ts @@ -1,3 +1,5 @@ +import type { ZodType } from 'zod' + export function extractFirstJsonObject(text: string): string | null { const start = text.indexOf('{') if (start === -1) { @@ -39,3 +41,24 @@ export function extractFirstJsonObject(text: string): string | null { return null } + +export function parseFirstJsonObject(text: string, schema: ZodType): T | null { + const extracted = extractFirstJsonObject(text) + if (!extracted) { + return null + } + + let raw: unknown + try { + raw = JSON.parse(extracted) + } catch { + return null + } + + const parsed = schema.safeParse(raw) + if (!parsed.success) { + return null + } + + return parsed.data +} diff --git a/backend/src/utils/process.ts b/backend/src/utils/process.ts index 0d2b4e6e1..d6f84173a 100644 --- a/backend/src/utils/process.ts +++ b/backend/src/utils/process.ts @@ -10,23 +10,24 @@ interface ExecuteCommandOptions { maxOutputChars?: number } -export async function executeCommand( - args: string[], - cwdOrOptions?: string | ExecuteCommandOptions -): Promise -export async function executeCommand( - args: string[], - cwdOrOptions: string | (ExecuteCommandOptions & { ignoreExitCode: true }) -): Promise -export async function executeCommand( +export interface ExecuteCommandResult { + exitCode: number + stdout: string + stderr: string + truncated?: boolean +} + +type ExecuteCommandReturn = O extends { ignoreExitCode: true } ? ExecuteCommandResult : string + +export async function executeCommand( args: string[], - cwdOrOptions?: string | ExecuteCommandOptions -): Promise { + cwdOrOptions?: string | O +): Promise> { const options: ExecuteCommandOptions = typeof cwdOrOptions === 'string' ? { cwd: cwdOrOptions } : cwdOrOptions || {} - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { const [command, ...cmdArgs] = args const effectiveEnv = { ...process.env, ...options.env } @@ -103,6 +104,7 @@ export async function executeCommand( exitCode, stdout, stderr: terminatedBySignal ? `${stderr}Command terminated by signal ${signal}` : stderr, + truncated: true, } : stdout) } else if (options.ignoreExitCode) { @@ -121,5 +123,5 @@ export async function executeCommand( reject(error) } }) - }) + }) as Promise> } diff --git a/backend/test/db/change-walkthroughs.test.ts b/backend/test/db/change-walkthroughs.test.ts index 25db50a9b..5214160ab 100644 --- a/backend/test/db/change-walkthroughs.test.ts +++ b/backend/test/db/change-walkthroughs.test.ts @@ -2,25 +2,32 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { Database } from 'bun:sqlite' import type { ChangeWalkthrough } from '@opencode-manager/shared/schemas' import { - deleteChangeWalkthrough, + deleteChangeWalkthroughs, ensureChangeWalkthroughTable, getChangeWalkthrough, saveChangeWalkthrough, } from '../../src/db/change-walkthroughs' import { migrate } from '../../src/db/migration-runner' import { allMigrations } from '../../src/db/migrations' +import migration202610081200 from '../../src/db/migrations/202610081200-change-walkthrough-sources' const SESSION_ID = 'ses_walkthrough' +const SESSION_SOURCE = 'session' +const STAGED_SOURCE = 'staged' function walkthrough(overrides: Partial = {}): ChangeWalkthrough { return { sessionId: SESSION_ID, + source: { kind: 'session' }, diffHash: 'hash-1', + model: null, summary: 'A short summary', - stops: [{ title: 'First stop', explanation: 'Why it matters', hunkIds: ['f0h0'] }], + stops: [ + { id: 's_1', title: 'First stop', explanation: 'Why it matters', hunkIds: ['h_1'], status: 'ready', explanationKey: null }, + ], hunks: [ { - id: 'f0h0', + id: 'h_1', file: 'src/app.ts', status: 'modified', header: '@@ -1,2 +1,2 @@', @@ -40,6 +47,12 @@ function walkthroughTableExists(db: Database): boolean { ) } +function tableColumns(db: Database): string[] { + return (db.prepare('PRAGMA table_info(change_walkthroughs)').all() as Array<{ name: string }>).map( + (column) => column.name, + ) +} + describe('change walkthroughs', () => { let db: Database @@ -52,27 +65,82 @@ describe('change walkthroughs', () => { db.close() }) - it('creates the change_walkthroughs table in migration 29', () => { + it('creates the change_walkthroughs table with per-source storage', () => { expect(walkthroughTableExists(db)).toBe(true) + expect(tableColumns(db)).toContain('source_key') + }) + + it('upgrades an existing single-source table and stays idempotent', () => { + const legacy = new Database(':memory:') + try { + ensureChangeWalkthroughTable(legacy) + + migrate(legacy, allMigrations) + expect(tableColumns(legacy)).toContain('source_key') + + expect(() => migrate(legacy, allMigrations)).not.toThrow() + expect(() => migration202610081200.up(legacy)).not.toThrow() + expect(tableColumns(legacy)).toContain('source_key') + } finally { + legacy.close() + } }) - it('creates the table idempotently', () => { + it('creates the old table idempotently', () => { expect(() => ensureChangeWalkthroughTable(db)).not.toThrow() expect(walkthroughTableExists(db)).toBe(true) }) it('returns null for a session without a stored walkthrough', () => { - expect(getChangeWalkthrough(db, 'ses_missing')).toBeNull() + expect(getChangeWalkthrough(db, 'ses_missing', SESSION_SOURCE)).toBeNull() }) it('round-trips a stored walkthrough', () => { const stored = walkthrough() saveChangeWalkthrough(db, stored) - expect(getChangeWalkthrough(db, SESSION_ID)).toEqual(stored) + expect(getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE)).toEqual(stored) }) - it('keeps a single row holding the latest payload on repeated saves', () => { + it('reads a walkthrough whose mechanical hunks predate hunk counts', () => { + const legacy = walkthrough({ + stops: [ + { + id: 's_mech', + title: 'Mechanical changes', + explanation: 'Lock files', + hunkIds: ['h_lock'], + status: 'ready', + explanationKey: null, + }, + ], + hunks: [ + { + id: 'h_lock', + file: 'pnpm-lock.yaml', + status: 'modified', + header: '@@ -1 +1 @@', + text: '@@ -1 +1 @@\n-old-lock\n+new-lock', + truncated: false, + }, + ], + }) + saveChangeWalkthrough(db, legacy) + + const stored = getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE) + expect(stored).toEqual(legacy) + expect(stored!.hunks[0]!.additions).toBeUndefined() + }) + + it('stores walkthroughs for two sources of one session side by side', () => { + saveChangeWalkthrough(db, walkthrough({ diffHash: 'hash-session' })) + saveChangeWalkthrough(db, walkthrough({ source: { kind: 'staged' }, diffHash: 'hash-staged', summary: 'Staged' })) + + expect(getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE)?.diffHash).toBe('hash-session') + expect(getChangeWalkthrough(db, SESSION_ID, STAGED_SOURCE)?.diffHash).toBe('hash-staged') + }) + + it('keeps a single row holding the latest payload on repeated saves of one source', () => { saveChangeWalkthrough(db, walkthrough()) saveChangeWalkthrough(db, walkthrough({ diffHash: 'hash-2', summary: 'Updated', createdAt: 2_000 })) @@ -81,43 +149,39 @@ describe('change walkthroughs', () => { .get(SESSION_ID) as { count: number } expect(count.count).toBe(1) - const stored = getChangeWalkthrough(db, SESSION_ID) + const stored = getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE) expect(stored).toMatchObject({ diffHash: 'hash-2', summary: 'Updated', createdAt: 2_000 }) }) - it('deletes only the walkthrough of the given session', () => { + it('deletes every source of the given session', () => { saveChangeWalkthrough(db, walkthrough()) + saveChangeWalkthrough(db, walkthrough({ source: { kind: 'staged' } })) saveChangeWalkthrough(db, walkthrough({ sessionId: 'ses_other' })) - deleteChangeWalkthrough(db, SESSION_ID) + deleteChangeWalkthroughs(db, SESSION_ID) - expect(getChangeWalkthrough(db, SESSION_ID)).toBeNull() - expect(getChangeWalkthrough(db, 'ses_other')).not.toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE)).toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, STAGED_SOURCE)).toBeNull() + expect(getChangeWalkthrough(db, 'ses_other', SESSION_SOURCE)).not.toBeNull() }) it('ignores deleting a session without a stored walkthrough', () => { - expect(() => deleteChangeWalkthrough(db, 'ses_missing')).not.toThrow() + expect(() => deleteChangeWalkthroughs(db, 'ses_missing')).not.toThrow() }) it('treats a corrupt payload as missing', () => { - db.prepare('INSERT INTO change_walkthroughs(session_id, diff_hash, payload, created_at) VALUES(?,?,?,?)').run( - SESSION_ID, - 'hash-1', - '{not json', - 1_000, - ) - - expect(getChangeWalkthrough(db, SESSION_ID)).toBeNull() + db.prepare( + 'INSERT INTO change_walkthroughs(session_id, source_key, diff_hash, payload, created_at) VALUES(?,?,?,?,?)', + ).run(SESSION_ID, SESSION_SOURCE, 'hash-1', '{not json', 1_000) + + expect(getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE)).toBeNull() }) it('treats a payload that fails schema validation as missing', () => { - db.prepare('INSERT INTO change_walkthroughs(session_id, diff_hash, payload, created_at) VALUES(?,?,?,?)').run( - SESSION_ID, - 'hash-1', - JSON.stringify({ sessionId: SESSION_ID }), - 1_000, - ) - - expect(getChangeWalkthrough(db, SESSION_ID)).toBeNull() + db.prepare( + 'INSERT INTO change_walkthroughs(session_id, source_key, diff_hash, payload, created_at) VALUES(?,?,?,?,?)', + ).run(SESSION_ID, SESSION_SOURCE, 'hash-1', JSON.stringify({ sessionId: SESSION_ID }), 1_000) + + expect(getChangeWalkthrough(db, SESSION_ID, SESSION_SOURCE)).toBeNull() }) }) diff --git a/backend/test/routes/change-walkthroughs.test.ts b/backend/test/routes/change-walkthroughs.test.ts index 6b6ac8272..415ba44c6 100644 --- a/backend/test/routes/change-walkthroughs.test.ts +++ b/backend/test/routes/change-walkthroughs.test.ts @@ -1,13 +1,22 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { Hono } from 'hono' import { Database } from 'bun:sqlite' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' import type { FileDiffInfo, SessionInfo } from '@opencode-manager/shared/opencode' +import { walkthroughHunksIdentity } from '@opencode-manager/shared/schemas' +import type { ChangeWalkthrough } from '@opencode-manager/shared/schemas' import { migrate } from '../../src/db/migration-runner' import { allMigrations } from '../../src/db/migrations' -import { ChangeWalkthroughService } from '../../src/services/change-walkthroughs' +import { saveChangeWalkthrough } from '../../src/db/change-walkthroughs' +import { ChangeWalkthroughService, computeChangesHash, computeHunkId } from '../../src/services/change-walkthroughs' +import { SettingsService } from '../../src/services/settings' import { createChangeWalkthroughRoutes } from '../../src/routes/change-walkthroughs' import type { OpenCodeClient } from '../../src/services/opencode/client' +import type { GitService } from '../../src/services/git/GitService' import { stubLoadedModelCatalog } from '../helpers/stub-opencode-client' +import { createCommittedRepo, createGitAuthService, git, uniqueName } from '../helpers/git-fixtures' const SESSION_ID = 'ses_walkthrough' @@ -17,9 +26,11 @@ function change(file: string, patch: string, status: FileDiffInfo['status'] = 'm const CHANGES: FileDiffInfo[] = [change('src/a.ts', '@@ -1,2 +1,2 @@\n-const a = 1;\n+const a = 2;')] +const HUNK_ID = computeHunkId(CHANGES[0]!.file, CHANGES[0]!.status, CHANGES[0]!.patch) + const MODEL_REPLY = JSON.stringify({ summary: 'A summary', - stops: [{ title: 'First', explanation: 'Why', hunkIds: ['f0h0'] }], + stops: [{ title: 'First', explanation: 'Why', hunkIds: [HUNK_ID] }], }) interface FakeSession { @@ -29,7 +40,7 @@ interface FakeSession { function createFakeClient(sessions: Record) { const generateCalls: string[] = [] - let generateImpl: () => Promise = async () => MODEL_REPLY + let generateImpl: (prompt: string) => Promise = async () => MODEL_REPLY const client = { api: { @@ -64,7 +75,7 @@ function createFakeClient(sessions: Record) { generate: { text: vi.fn(async (input: { prompt: string }) => { generateCalls.push(input.prompt) - return { text: await generateImpl() } + return { text: await generateImpl(input.prompt) } }), }, }, @@ -74,12 +85,34 @@ function createFakeClient(sessions: Record) { return { client, generateCalls, - setGenerateImpl: (impl: () => Promise) => { + setGenerateImpl: (impl: (prompt: string) => Promise) => { generateImpl = impl }, } } +const routeRepoRoot = mkdtempSync(path.join(tmpdir(), 'walkthrough-routes-')) + +afterAll(() => { + rmSync(routeRepoRoot, { recursive: true, force: true }) +}) + +function createRepoWithStagedChange(): string { + const repo = path.join(routeRepoRoot, uniqueName('repo')) + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'a.txt'), 'one\n') + git(['add', 'a.txt'], repo) + git(['commit', '-m', 'add a'], repo) + writeFileSync(path.join(repo, 'a.txt'), 'two\n') + git(['add', 'a.txt'], repo) + return repo +} + +function replyForPrompt(prompt: string, summary = 'A summary'): string { + const ids = [...prompt.matchAll(/^### (h_\S+) /gm)].map((match) => match[1]!) + return JSON.stringify({ summary, stops: [{ title: 'A', explanation: 'Why', hunkIds: ids }] }) +} + describe('change walkthrough routes', () => { let db: Database let sessions: Record @@ -91,7 +124,13 @@ describe('change walkthrough routes', () => { migrate(db, allMigrations) sessions = { [SESSION_ID]: { changes: CHANGES } } fake = createFakeClient(sessions) - const service = new ChangeWalkthroughService(db, fake.client) + const service = new ChangeWalkthroughService( + db, + fake.client, + new SettingsService(db), + createGitAuthService(), + { fetchRemoteRef: vi.fn(async () => {}) } as unknown as GitService, + ) app = new Hono() app.route('/change-walkthroughs', createChangeWalkthroughRoutes(service)) }) @@ -125,6 +164,88 @@ describe('change walkthrough routes', () => { await expect(res.json()).resolves.toMatchObject({ error: 'Session not found' }) }) + it('GET reads the diff of the requested source', async () => { + const repo = createRepoWithStagedChange() + sessions[SESSION_ID]!.info = { id: SESSION_ID, title: 'Title', location: { directory: repo } } as SessionInfo + + const sessionRes = await app.request(`/change-walkthroughs/${SESSION_ID}`) + const sessionBody = (await sessionRes.json()) as { currentDiffHash: string | null } + const stagedRes = await app.request(`/change-walkthroughs/${SESSION_ID}?source=staged`) + const stagedBody = (await stagedRes.json()) as { currentDiffHash: string | null } + + expect(stagedRes.status).toBe(200) + expect(typeof stagedBody.currentDiffHash).toBe('string') + expect(stagedBody.currentDiffHash).not.toBe(sessionBody.currentDiffHash) + }) + + it('GET rejects an invalid source with 400', async () => { + const res = await app.request(`/change-walkthroughs/${SESSION_ID}?source=branch:-x`) + + expect(res.status).toBe(400) + await expect(res.json()).resolves.toMatchObject({ error: 'Invalid walkthrough source' }) + }) + + it('GET omits hunks when hunksFor matches the stored identity and returns them otherwise', async () => { + const stored: ChangeWalkthrough = { + sessionId: SESSION_ID, + source: { kind: 'session' }, + diffHash: computeChangesHash(CHANGES), + model: null, + summary: 'A summary', + stops: [ + { id: 's_1', title: 'First', explanation: 'Why', hunkIds: [HUNK_ID], status: 'ready', explanationKey: null }, + ], + hunks: [ + { + id: HUNK_ID, + file: CHANGES[0]!.file, + status: CHANGES[0]!.status, + header: '@@ -1,2 +1,2 @@', + text: CHANGES[0]!.patch, + truncated: false, + }, + ], + omittedFiles: [], + createdAt: 42, + } + saveChangeWalkthrough(db, stored) + const identity = walkthroughHunksIdentity(stored) + + const fullRes = await app.request(`/change-walkthroughs/${SESSION_ID}`) + const compactRes = await app.request( + `/change-walkthroughs/${SESSION_ID}?hunksFor=${encodeURIComponent(identity)}`, + ) + const mismatchRes = await app.request(`/change-walkthroughs/${SESSION_ID}?hunksFor=other`) + + const full = (await fullRes.json()) as { walkthrough: { hunks?: unknown[] } } + const compact = (await compactRes.json()) as { walkthrough: { hunks?: unknown[] } } + const mismatch = (await mismatchRes.json()) as { walkthrough: { hunks?: unknown[] } } + + expect(full.walkthrough.hunks).toHaveLength(1) + expect(compact.walkthrough.hunks).toBeUndefined() + expect(mismatch.walkthrough.hunks).toHaveLength(1) + }) + + it('POST stores the walkthrough under the requested source', async () => { + const repo = createRepoWithStagedChange() + sessions[SESSION_ID]!.info = { id: SESSION_ID, title: 'Title', location: { directory: repo } } as SessionInfo + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt, 'Staged summary')) + + const res = await app.request(`/change-walkthroughs/${SESSION_ID}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ source: { kind: 'staged' } }), + }) + + expect(res.status).toBe(202) + + await vi.waitFor(async () => { + const getRes = await app.request(`/change-walkthroughs/${SESSION_ID}?source=staged`) + const state = (await getRes.json()) as { walkthrough: { summary: string } | null } + expect(state.walkthrough?.summary).toBe('Staged summary') + }) + }) + it('POST returns 202 while generating, then GET reads the walkthrough back', async () => { let resolveGenerate: (text: string) => void = () => {} fake.setGenerateImpl(() => new Promise((resolve) => { @@ -248,16 +369,11 @@ describe('change walkthrough routes', () => { }) it('POST records an unparseable model reply that GET surfaces', async () => { - let resolveGenerate: (text: string) => void = () => {} - fake.setGenerateImpl(() => new Promise((resolve) => { - resolveGenerate = resolve - })) + fake.setGenerateImpl(async () => 'not json') const res = await app.request(`/change-walkthroughs/${SESSION_ID}`, { method: 'POST' }) expect(res.status).toBe(202) - resolveGenerate('not json') - await vi.waitFor(async () => { const getRes = await app.request(`/change-walkthroughs/${SESSION_ID}`) const state = (await getRes.json()) as { generating: boolean; error: { code?: string } | null } diff --git a/backend/test/routes/internal-settings.test.ts b/backend/test/routes/internal-settings.test.ts index a9e87b75a..75f5bf90e 100644 --- a/backend/test/routes/internal-settings.test.ts +++ b/backend/test/routes/internal-settings.test.ts @@ -353,4 +353,19 @@ describe('internal/settings routes', () => { expect(body.preferences.theme).toBe('light') expect(body.preferences.mode).toBe('plan') }) + + it('PATCH /api/internal/settings with { walkthroughModel } persists it', async () => { + const res = await app.request('/api/internal/settings', { + method: 'PATCH', + body: JSON.stringify({ walkthroughModel: 'openai/gpt-5-mini' }), + headers: { + 'content-type': 'application/json', + authorization: `Bearer ${token}`, + }, + }) + expect(res.status).toBe(200) + const body = await res.json() as { preferences: { walkthroughModel?: string } } + expect(body.preferences.walkthroughModel).toBe('openai/gpt-5-mini') + expect(settingsService.getSettings().preferences.walkthroughModel).toBe('openai/gpt-5-mini') + }) }) diff --git a/backend/test/services/assistant-mode.test.ts b/backend/test/services/assistant-mode.test.ts index 0bee4910c..0054cd755 100644 --- a/backend/test/services/assistant-mode.test.ts +++ b/backend/test/services/assistant-mode.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, beforeEach, afterEach } from 'bun:test' import path from 'path' import { access, readFile, writeFile } from 'fs/promises' import { Hono } from 'hono' +import { AssistantSettingsPatchSchema } from '@opencode-manager/shared/schemas' import { ensureAssistantMode, getAssistantModeStatus, buildSchedulesSkill, buildReposSkill, buildSettingsSkill, buildSessionsSkill, buildAssistantDefaultAgentMd, buildAssistantOpenCodeConfig, buildAssistantRepo, installAssistantWorkspace } from '../../src/services/assistant-mode' import { createTempAssistantWorkspace, createTestDb, mockRepo } from '../helpers/assistant-workspace' import { ScheduleService } from '../../src/services/schedules' @@ -162,6 +163,15 @@ describe('buildSettingsSkill', () => { expect(skill).toContain('stt.endpoint') expect(skill).toContain('DO NOT attempt to set') }) + + it('lists every AssistantSettingsPatchSchema key in the allowed keys section', () => { + const skill = buildSettingsSkill() + const allowedSection = skill.split('**Allowed Keys:**')[1]?.split('**DO NOT attempt to set:**')[0] + if (allowedSection === undefined) throw new Error('Allowed Keys section is missing') + for (const key of Object.keys(AssistantSettingsPatchSchema.shape)) { + expect(allowedSection).toContain(`\`${key}\``) + } + }) }) describe('buildAssistantDefaultAgentMd', () => { diff --git a/backend/test/services/change-walkthroughs.test.ts b/backend/test/services/change-walkthroughs.test.ts index 7da63d8f2..c24ad1693 100644 --- a/backend/test/services/change-walkthroughs.test.ts +++ b/backend/test/services/change-walkthroughs.test.ts @@ -1,27 +1,44 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' import { Database } from 'bun:sqlite' import type { FileDiffInfo, ModelRef, SessionInfo, SessionMessageInfo } from '@opencode-manager/shared/opencode' import { + MECHANICAL_TEXT_BUDGET, WALKTHROUGH_DIFF_MAX_CHARS, WALKTHROUGH_HUNK_MAX_CHARS, WALKTHROUGH_MAX_STOPS, + WALKTHROUGH_OUTLINE_LINE_MAX_CHARS, + WALKTHROUGH_OUTLINE_PREVIEW_LINES, WALKTHROUGH_TEXT_MAX_CHARS, type WalkthroughHunk, } from '@opencode-manager/shared/schemas' -import { getChangeWalkthrough } from '../../src/db/change-walkthroughs' +import { getChangeWalkthrough, saveChangeWalkthrough, deleteChangeWalkthroughs } from '../../src/db/change-walkthroughs' import { migrate } from '../../src/db/migration-runner' import { allMigrations } from '../../src/db/migrations' import type { OpenCodeClient } from '../../src/services/opencode/client' +import { SettingsService } from '../../src/services/settings' +import type { GitService } from '../../src/services/git/GitService' import type { SSEEvent } from '../../src/services/sse-aggregator' import { stubLoadedModelCatalog } from '../helpers/stub-opencode-client' +import { createCommittedRepo, createGitAuthService, createOrigin, cloneOrigin, git, uniqueName } from '../helpers/git-fixtures' import { - ChangeWalkthroughError, ChangeWalkthroughService, buildWalkthroughInput, + buildWalkthroughOutline, + buildWalkthroughPlanPrompt, buildWalkthroughPrompt, computeChangesHash, + computeHunkId, + computeStopId, + fitsSingleCall, + formatHunkOutline, + isMechanicalChangePath, + parseWalkthroughPlan, parseWalkthroughResponse, } from '../../src/services/change-walkthroughs' +import { ChangeWalkthroughError } from '../../src/services/change-walkthrough-error' const SESSION_ID = 'ses_walkthrough' @@ -38,7 +55,19 @@ function hunkPatch(line: number): string { } function bigHunk(index: number, size = WALKTHROUGH_HUNK_MAX_CHARS): string { - return `@@ -${index},1 +${index},1 @@\n+${'x'.repeat(size)}` + return `@@ -${index},1 +${index},1 @@\n+${'x'.repeat(size)}_${index}` +} + +function longHunk(index: number): string { + const lines = Array.from( + { length: 6 }, + (_, line) => `+${'x'.repeat(200)}_${index}_${line}`, + ) + return `@@ -${index + 1},6 +${index + 1},6 @@\n${lines.join('\n')}` +} + +function longPatch(count: number, offset = 0): string { + return Array.from({ length: count }, (_, index) => longHunk(offset + index)).join('\n') } function modelReply( @@ -48,6 +77,69 @@ function modelReply( return JSON.stringify({ summary, stops }) } +function planReply( + stops: Array<{ title: string; hunkIds: string[] }>, + summary = 'Summary', +): string { + return JSON.stringify({ summary, stops }) +} + +function hunkFixture(file: string, text: string, status: FileDiffInfo['status'] = 'modified'): WalkthroughHunk { + return { + id: computeHunkId(file, status, text), + file, + status, + header: text.split('\n')[0] ?? '', + text, + truncated: false, + } +} + +const THREE_HUNKS: FileDiffInfo[] = [ + change('src/a.ts', `${hunkPatch(1)}\n${hunkPatch(10)}`), + change('src/b.ts', hunkPatch(1)), +] + +const THREE_HUNK_IDS = [ + computeHunkId('src/a.ts', 'modified', hunkPatch(1)), + computeHunkId('src/a.ts', 'modified', hunkPatch(10)), + computeHunkId('src/b.ts', 'modified', hunkPatch(1)), +] + +const PLAN_MARKER = 'Assign every hunk id' +const EXPLANATION_REPLY = JSON.stringify({ explanation: 'Why' }) + +const LARGE_CHANGES: FileDiffInfo[] = [ + change('src/a.ts', [0, 1, 2, 3].map((index) => bigHunk(index)).join('\n')), + change('src/b.ts', [10, 11, 12, 13].map((index) => bigHunk(index)).join('\n')), + change('src/c.ts', [20, 21, 22, 23].map((index) => bigHunk(index)).join('\n')), +] + +const LARGE_PLAN_REPLY = planReply([ + { title: 'A', hunkIds: [0, 1, 2, 3].map((index) => computeHunkId('src/a.ts', 'modified', bigHunk(index))) }, + { title: 'B', hunkIds: [10, 11, 12, 13].map((index) => computeHunkId('src/b.ts', 'modified', bigHunk(index))) }, + { title: 'C', hunkIds: [20, 21, 22, 23].map((index) => computeHunkId('src/c.ts', 'modified', bigHunk(index))) }, +]) + +const EDITED_A_HUNKS = [0, 1, 2, 3].map((index) => bigHunk(index, WALKTHROUGH_HUNK_MAX_CHARS - 10)) + +const EDITED_LARGE_CHANGES: FileDiffInfo[] = [ + change('src/a.ts', EDITED_A_HUNKS.join('\n')), + change('src/b.ts', [10, 11, 12, 13].map((index) => bigHunk(index)).join('\n')), + change('src/c.ts', [20, 21, 22, 23].map((index) => bigHunk(index)).join('\n')), +] + +const EDITED_LARGE_PLAN_REPLY = planReply([ + { title: 'A', hunkIds: EDITED_A_HUNKS.map((hunk) => computeHunkId('src/a.ts', 'modified', hunk)) }, + { title: 'B', hunkIds: [10, 11, 12, 13].map((index) => computeHunkId('src/b.ts', 'modified', bigHunk(index))) }, + { title: 'C', hunkIds: [20, 21, 22, 23].map((index) => computeHunkId('src/c.ts', 'modified', bigHunk(index))) }, +]) + +function explanationFor(prompt: string): string { + const title = prompt.match(/## Stop: (.+)/)?.[1] ?? '' + return JSON.stringify({ explanation: `explained-${title}` }) +} + interface FakeSession { info?: SessionInfo | Error changes?: FileDiffInfo[] | Error @@ -56,7 +148,7 @@ interface FakeSession { function createFakeClient(sessions: Record) { const generateCalls: string[] = [] const generateModels: Array = [] - let generateImpl: () => Promise = async () => modelReply([]) + let generateImpl: (prompt: string) => Promise = async () => modelReply([]) const client = { api: { @@ -92,7 +184,7 @@ function createFakeClient(sessions: Record) { text: vi.fn(async (input: { prompt: string; model?: ModelRef }) => { generateCalls.push(input.prompt) generateModels.push(input.model) - return { text: await generateImpl() } + return { text: await generateImpl(input.prompt) } }), }, }, @@ -103,7 +195,7 @@ function createFakeClient(sessions: Record) { client, generateCalls, generateModels, - setGenerateImpl: (impl: () => Promise) => { + setGenerateImpl: (impl: (prompt: string) => Promise) => { generateImpl = impl }, } @@ -125,6 +217,53 @@ function sessionEvent(type: string, sessionID: string): SSEEvent { } as unknown as SSEEvent } +const walkthroughRepoRoot = mkdtempSync(path.join(tmpdir(), 'walkthrough-service-')) + +afterAll(() => { + rmSync(walkthroughRepoRoot, { recursive: true, force: true }) +}) + +function createChangedRepo(): string { + const repo = path.join(walkthroughRepoRoot, uniqueName('repo')) + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'a.txt'), 'one\n') + git(['add', 'a.txt'], repo) + git(['commit', '-m', 'add a'], repo) + writeFileSync(path.join(repo, 'a.txt'), 'two\n') + return repo +} + +function createPullRequestRepo(): string { + const origin = path.join(walkthroughRepoRoot, uniqueName('pr-origin')) + const work = path.join(walkthroughRepoRoot, uniqueName('pr-work')) + const clone = path.join(walkthroughRepoRoot, uniqueName('pr-clone')) + createOrigin(origin, work) + cloneOrigin(origin, clone) + writeFileSync(path.join(clone, 'pr.txt'), 'pr\n') + git(['add', 'pr.txt'], clone) + git(['commit', '-m', 'pr'], clone) + git(['push', 'origin', 'HEAD:refs/pull/1/head'], clone) + return clone +} + +function refExists(repo: string, ref: string): boolean { + try { + git(['rev-parse', '--verify', ref], repo) + return true + } catch { + return false + } +} + +function sessionAt(directory: string, id: string = SESSION_ID): SessionInfo { + return { id, title: 'Title', location: { directory } } as SessionInfo +} + +function replyForPrompt(prompt: string): string { + const ids = [...prompt.matchAll(/^### (h_\S+) /gm)].map((match) => match[1]!) + return modelReply([{ title: 'A', explanation: 'x', hunkIds: ids }]) +} + describe('computeChangesHash', () => { it('is stable for identical changes', () => { const changes = [change('src/a.ts', hunkPatch(1))] @@ -147,29 +286,91 @@ describe('computeChangesHash', () => { }) }) +describe('isMechanicalChangePath', () => { + it('matches lockfiles by basename', () => { + const lockfiles = [ + 'pnpm-lock.yaml', + 'package-lock.json', + 'npm-shrinkwrap.json', + 'yarn.lock', + 'bun.lock', + 'bun.lockb', + 'Cargo.lock', + 'Gemfile.lock', + 'composer.lock', + 'poetry.lock', + 'uv.lock', + 'Pipfile.lock', + 'go.sum', + 'flake.lock', + 'deep/nested/pnpm-lock.yaml', + ] + expect(lockfiles.every(isMechanicalChangePath)).toBe(true) + }) + + it('matches generated suffixes and snapshot directories', () => { + expect(isMechanicalChangePath('src/__snapshots__/a.ts.snap')).toBe(true) + expect(isMechanicalChangePath('src/__snapshots__/a.ts')).toBe(true) + expect(isMechanicalChangePath('dist/bundle.min.js')).toBe(true) + expect(isMechanicalChangePath('dist/app.min.css')).toBe(true) + expect(isMechanicalChangePath('dist/bundle.js.map')).toBe(true) + }) + + it('does not match ordinary source and config files', () => { + expect(isMechanicalChangePath('src/lock.ts')).toBe(false) + expect(isMechanicalChangePath('package.json')).toBe(false) + expect(isMechanicalChangePath('src/snapshots/a.ts')).toBe(false) + expect(isMechanicalChangePath('src/map.ts')).toBe(false) + }) +}) + describe('buildWalkthroughInput', () => { - it('assigns file and hunk ordered ids', () => { - const { hunks, omittedFiles } = buildWalkthroughInput( - [ - change('src/a.ts', `${hunkPatch(1)}\n${hunkPatch(10)}`), - change('src/b.ts', hunkPatch(1)), - ], - 'My session', - ) + it('assigns a distinct content id and the file to each hunk', () => { + const { hunks, omittedFiles } = buildWalkthroughInput([ + change('src/a.ts', `${hunkPatch(1)}\n${hunkPatch(10)}`), + change('src/b.ts', hunkPatch(1)), + ]) expect(omittedFiles).toEqual([]) - expect(hunks.map((hunk) => hunk.id)).toEqual(['f0h0', 'f0h1', 'f1h0']) expect(hunks.map((hunk) => hunk.file)).toEqual(['src/a.ts', 'src/a.ts', 'src/b.ts']) + const ids = hunks.map((hunk) => hunk.id) + expect(ids.every((id) => id.startsWith('h_'))).toBe(true) + expect(new Set(ids).size).toBe(3) + }) + + it('keeps a hunk id when an earlier hunk in the file changes', () => { + const before = buildWalkthroughInput([ + change( + 'src/a.ts', + '@@ -1,2 +1,2 @@\n-const a = 1;\n+const a = 2;\n@@ -10,2 +10,2 @@\n-const b = 1;\n+const b = 2;', + ), + ]) + const after = buildWalkthroughInput([ + change( + 'src/a.ts', + '@@ -1,3 +1,3 @@\n-const a = 1;\n+const a = 2;\n+const extra = 3;\n@@ -11,2 +11,2 @@\n-const b = 1;\n+const b = 2;', + ), + ]) + + expect(before.hunks).toHaveLength(2) + expect(after.hunks).toHaveLength(2) + expect(after.hunks[0]!.id).not.toBe(before.hunks[0]!.id) + expect(after.hunks[1]!.id).toBe(before.hunks[1]!.id) + }) + + it('suffixes duplicate hunk content', () => { + const { hunks } = buildWalkthroughInput([change('src/a.ts', `${hunkPatch(1)}\n${hunkPatch(1)}`)]) + + expect(hunks).toHaveLength(2) + expect(hunks[0]!.id).not.toBe(hunks[1]!.id) + expect(hunks[1]!.id).toBe(`${hunks[0]!.id}_2`) }) it('omits files with no hunks as binary', () => { - const { hunks, omittedFiles } = buildWalkthroughInput( - [ - change('assets/logo.png', 'diff --git a/assets/logo.png b/assets/logo.png\nBinary files differ'), - change('src/a.ts', hunkPatch(1)), - ], - 'My session', - ) + const { hunks, omittedFiles } = buildWalkthroughInput([ + change('assets/logo.png', 'diff --git a/assets/logo.png b/assets/logo.png\nBinary files differ'), + change('src/a.ts', hunkPatch(1)), + ]) expect(hunks).toHaveLength(1) expect(omittedFiles).toEqual([{ file: 'assets/logo.png', reason: 'binary' }]) @@ -177,76 +378,431 @@ describe('buildWalkthroughInput', () => { it('truncates an oversized hunk and marks it', () => { const longLine = `+${'x'.repeat(WALKTHROUGH_HUNK_MAX_CHARS + 100)}` - const { hunks } = buildWalkthroughInput([change('src/a.ts', `@@ -1 +1 @@\n${longLine}`)], 'My session') + const { hunks } = buildWalkthroughInput([change('src/a.ts', `@@ -1 +1 @@\n${longLine}`)]) expect(hunks[0]!.truncated).toBe(true) expect(hunks[0]!.text).toContain('[hunk truncated]') }) + it('separates mechanical hunks from the prompt hunks', () => { + const { hunks, mechanicalHunks, omittedFiles } = buildWalkthroughInput([ + change('src/a.ts', hunkPatch(1)), + change('pnpm-lock.yaml', hunkPatch(2)), + ]) + + expect(hunks.map((hunk) => hunk.file)).toEqual(['src/a.ts']) + expect(mechanicalHunks.map((hunk) => hunk.file)).toEqual(['pnpm-lock.yaml']) + expect(omittedFiles).toEqual([]) + }) + + it('keeps mechanical hunk text within the budget and summarizes every file with counts', () => { + const oversizedPatch = Array.from({ length: 40 }, (_, index) => bigHunk(index)).join('\n') + const { mechanicalHunks, omittedFiles } = buildWalkthroughInput([ + { file: 'pnpm-lock.yaml', patch: oversizedPatch, additions: 1200, deletions: 800, status: 'modified' }, + { file: 'src/__snapshots__/a.ts.snap', patch: hunkPatch(1), additions: 2, deletions: 3, status: 'modified' }, + ]) + + expect(mechanicalHunks.map((hunk) => hunk.file)).toEqual([ + 'pnpm-lock.yaml', + 'src/__snapshots__/a.ts.snap', + ]) + expect(mechanicalHunks.reduce((total, hunk) => total + hunk.text.length, 0)).toBeLessThanOrEqual( + MECHANICAL_TEXT_BUDGET, + ) + expect(mechanicalHunks[0]).toMatchObject({ text: '', truncated: true, additions: 1200, deletions: 800 }) + expect(mechanicalHunks[1]).toMatchObject({ additions: 2, deletions: 3 }) + expect(mechanicalHunks[1]!.text).toContain('@@') + expect(omittedFiles).toEqual([]) + }) + + it('stores mechanical hunk text and counts while within the budget', () => { + const { mechanicalHunks } = buildWalkthroughInput([ + { + file: 'pnpm-lock.yaml', + patch: `${hunkPatch(1)}\n${hunkPatch(2)}`, + additions: 2, + deletions: 2, + status: 'modified', + }, + ]) + + expect(mechanicalHunks).toHaveLength(1) + expect(mechanicalHunks[0]!.text).toContain('@@ -1,2 +1,2 @@') + expect(mechanicalHunks[0]!.text).toContain('@@ -2,2 +2,2 @@') + expect(mechanicalHunks[0]!.additions).toBe(2) + expect(mechanicalHunks[0]!.deletions).toBe(2) + }) + + it('omits a binary mechanical file as binary', () => { + const { mechanicalHunks, omittedFiles } = buildWalkthroughInput([ + change('pnpm-lock.yaml', 'diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml\nBinary files differ'), + ]) + + expect(mechanicalHunks).toEqual([]) + expect(omittedFiles).toEqual([{ file: 'pnpm-lock.yaml', reason: 'binary' }]) + }) + + it('labels a pure rename as renamed', () => { + const { omittedFiles } = buildWalkthroughInput([ + change( + 'src/new.ts', + 'diff --git a/src/old.ts b/src/new.ts\nsimilarity index 100%\nrename from src/old.ts\nrename to src/new.ts', + ), + ]) + + expect(omittedFiles).toEqual([{ file: 'src/new.ts', reason: 'renamed' }]) + }) + + it('labels a mode-only change as modeChange', () => { + const { omittedFiles } = buildWalkthroughInput([ + change('script.sh', 'diff --git a/script.sh b/script.sh\nold mode 100644\nnew mode 100755'), + ]) + + expect(omittedFiles).toEqual([{ file: 'script.sh', reason: 'modeChange' }]) + }) +}) + +describe('buildWalkthroughOutline', () => { + const smallChange = (index: number) => change(`src/f${index}.ts`, hunkPatch(index)) + + it('fits 300 small files and lists every hunk id', () => { + const input = buildWalkthroughInput(Array.from({ length: 300 }, (_, index) => smallChange(index))) + const { hunks, omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') + + expect(omittedFiles).toEqual([]) + expect(hunks).toHaveLength(300) + for (const hunk of input.hunks) { + expect(outline).toContain(hunk.id) + } + expect(fitsSingleCall(input, 'My session')).toBe(true) + expect( + buildWalkthroughPlanPrompt({ title: 'My session', outline, previousStops: [] }).length, + ).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) + }) + + it('omits trailing files as budget and never half-includes a file', () => { + const input = buildWalkthroughInput( + Array.from({ length: 400 }, (_, index) => change(`src/f${index}.ts`, longPatch(3, index * 3))), + ) + const { omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') + + expect(omittedFiles.some((file) => file.reason === 'budget')).toBe(true) + + const idsByFile = new Map() + for (const hunk of input.hunks) { + const ids = idsByFile.get(hunk.file) ?? [] + ids.push(hunk.id) + idsByFile.set(hunk.file, ids) + } + + const includedFiles = new Set() + for (const [file, ids] of idsByFile) { + const present = ids.filter((id) => outline.includes(id)).length + expect(present === 0 || present === ids.length).toBe(true) + if (present > 0) { + includedFiles.add(file) + } + } + + expect(omittedFiles.map((file) => file.file)).toEqual( + [...idsByFile.keys()].filter((file) => !includedFiles.has(file)), + ) + }) + + it('falls back to header-only outlines when a single file does not fit at the default preview depth', () => { + const input = buildWalkthroughInput([change('src/huge.ts', longPatch(100))]) + const { hunks, omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') + + expect(hunks).toHaveLength(100) + expect(omittedFiles).toEqual([]) + for (const hunk of input.hunks) { + expect(outline).toContain(hunk.id) + } + }) + + it('keeps mechanical hunks out of the outline', () => { + const input = buildWalkthroughInput([ + change('src/a.ts', hunkPatch(1)), + change('pnpm-lock.yaml', hunkPatch(2)), + ]) + const { omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') + + expect(input.mechanicalHunks).toHaveLength(1) + expect(outline).not.toContain(input.mechanicalHunks[0]!.id) + expect(outline).not.toContain('pnpm-lock.yaml') + expect(omittedFiles).toEqual([]) + }) + + it('reserves the title in the budget so a long title admits less content', () => { + const input = buildWalkthroughInput(Array.from({ length: 800 }, (_, index) => smallChange(index))) + const longTitle = 'T'.repeat(20_000) + + const withLongTitle = buildWalkthroughOutline(input, longTitle) + const withShortTitle = buildWalkthroughOutline(input, 'short') + + expect(withShortTitle.omittedFiles).toEqual([]) + expect(withLongTitle.hunks.length).toBeLessThan(withShortTitle.hunks.length) + expect( + buildWalkthroughPlanPrompt({ + title: longTitle, + outline: withLongTitle.outline, + previousStops: [], + }).length, + ).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) + }) + it('bounds the included hunks and omits the remaining files as budget', () => { - const changes = Array.from({ length: 10 }, (_, index) => change(`src/f${index}.ts`, bigHunk(index))) - const { hunks, omittedFiles } = buildWalkthroughInput(changes, 'My session') + const input = buildWalkthroughInput( + Array.from({ length: 200 }, (_, index) => change(`src/f${index}.ts`, longPatch(6, index * 6))), + ) + const { hunks, omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') - const included = hunks.reduce((sum, hunk) => sum + hunk.text.length, 0) - expect(included).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) + expect(hunks.length).toBeLessThan(input.hunks.length) expect(omittedFiles.filter((file) => file.reason === 'budget').length).toBeGreaterThan(0) expect(omittedFiles.every((file) => file.reason === 'budget')).toBe(true) + expect(outline.length).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) }) it('never half-includes a file', () => { - const patch = Array.from({ length: 9 }, (_, index) => bigHunk(index)).join('\n') - const { hunks, omittedFiles } = buildWalkthroughInput([change('src/a.ts', patch)], 'My session') - - expect(hunks).toEqual([]) - expect(omittedFiles).toEqual([{ file: 'src/a.ts', reason: 'budget' }]) + const input = buildWalkthroughInput([ + change('src/small.ts', hunkPatch(1)), + change('src/huge.ts', longPatch(2000)), + ]) + const { outline } = buildWalkthroughOutline(input, 'My session') + + const hugeIds = input.hunks.filter((hunk) => hunk.file === 'src/huge.ts').map((hunk) => hunk.id) + expect(hugeIds.length).toBeGreaterThan(0) + expect(hugeIds.every((id) => !outline.includes(id))).toBe(true) }) - it('keeps the prompt within budget for many small hunks behind a very long path', () => { + it('keeps the outline within budget for many small hunks behind a very long path', () => { const longPath = `src/${'p'.repeat(996)}.ts` const manyHunks = Array.from( { length: 300 }, (_, index) => `@@ -${index + 1} +${index + 1} @@\n+line${index}`, ).join('\n') - const changes = [change('src/small.ts', hunkPatch(1)), change(longPath, manyHunks)] - - const { hunks, omittedFiles } = buildWalkthroughInput(changes, 'My session') - const prompt = buildWalkthroughPrompt({ title: 'My session', hunks }) + const input = buildWalkthroughInput([ + change('src/small.ts', hunkPatch(1)), + change(longPath, manyHunks), + ]) + const { hunks, omittedFiles, outline } = buildWalkthroughOutline(input, 'My session') expect(hunks.length).toBeGreaterThan(0) - expect(prompt.length).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) expect(omittedFiles).toEqual([{ file: longPath, reason: 'budget' }]) + expect( + buildWalkthroughPlanPrompt({ title: 'My session', outline, previousStops: [] }).length, + ).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) }) - it('reserves the title in the budget so a long title admits less content', () => { - const changes = Array.from({ length: 20 }, (_, index) => change(`src/f${index}.ts`, bigHunk(index))) - const longTitle = 'T'.repeat(20_000) + it('retries at a shallower preview depth so every file fits when the single call does not', () => { + const input = buildWalkthroughInput( + Array.from({ length: 70 }, (_, index) => change(`src/f${index}.ts`, longPatch(1, index))), + ) - const withLongTitle = buildWalkthroughInput(changes, longTitle) - const withShortTitle = buildWalkthroughInput(changes, 'short') + expect(fitsSingleCall(input, 'My session')).toBe(false) - expect(withLongTitle.hunks.length).toBeLessThan(withShortTitle.hunks.length) - expect( - buildWalkthroughPrompt({ title: longTitle, hunks: withLongTitle.hunks }).length, - ).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) + const { hunks, omittedFiles } = buildWalkthroughOutline(input, 'My session') + + expect(omittedFiles).toEqual([]) + expect(hunks).toHaveLength(70) + }) + + it('reserves room for the previous-stops hint', () => { + const input = buildWalkthroughInput( + Array.from({ length: 1000 }, (_, index) => smallChange(index)), + ) + const previousStops = [{ title: 'A'.repeat(5000), hunkIds: [input.hunks[0]!.id] }] + const { outline } = buildWalkthroughOutline(input, 'My session', previousStops) + const prompt = buildWalkthroughPlanPrompt({ title: 'My session', outline, previousStops }) + + expect(prompt).toContain('Previous stops') + expect(prompt.length).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) }) }) -describe('buildWalkthroughPrompt', () => { +describe('formatHunkOutline', () => { + it('renders the id, file, status, header and change counts', () => { + const hunk = hunkFixture('src/a.ts', '@@ -1,2 +1,2 @@\n-const a = 1;\n+const a = 2;') + const [first] = formatHunkOutline(hunk).split('\n') + + expect(first).toBe(`### ${hunk.id} src/a.ts (modified) @@ -1,2 +1,2 @@ +1 -1`) + }) + + it('previews changed lines and counts content lines that start with -- or ++', () => { + const hunk = hunkFixture( + 'src/a.ts', + '@@ -1,4 +1,4 @@\n--- a/x\n+++ b/x\n-const a = 1;\n+const a = 2;\n-const b = 1;\n+const b = 2;', + ) + + const single = formatHunkOutline(hunk, 1).split('\n') + expect(single).toHaveLength(2) + expect(single[1]).toBe('--- a/x') + + const full = formatHunkOutline(hunk).split('\n') + expect(full).toContain('--- a/x') + expect(full).toContain('+++ b/x') + expect(full).toContain('-const a = 1;') + expect(full).toContain('+const a = 2;') + expect(full).toContain('-const b = 1;') + expect(full).toContain('+const b = 2;') + expect(full[0]).toContain('+3 -3') + }) + + it('defaults to the shared preview line count', () => { + const text = `@@ -1,8 +1,8 @@\n${Array.from({ length: 8 }, (_, index) => `+line${index}`).join('\n')}` + const hunk = hunkFixture('src/a.ts', text) + + expect(formatHunkOutline(hunk).split('\n')).toHaveLength(1 + WALKTHROUGH_OUTLINE_PREVIEW_LINES) + }) + + it('truncates a long changed line to the line max', () => { + const hunk = hunkFixture('src/a.ts', `@@ -1 +1 @@\n+${'x'.repeat(200)}`) + const line = formatHunkOutline(hunk).split('\n')[1]! + + expect(line).toHaveLength(WALKTHROUGH_OUTLINE_LINE_MAX_CHARS) + }) +}) + +describe('buildWalkthroughPlanPrompt', () => { + const outline = '### h_1 src/a.ts (modified) @@ -1 +1 @@ +1 -1\n+const a = 2;' + + it('contains the preamble, the plan rules and the outline', () => { + const prompt = buildWalkthroughPlanPrompt({ title: 'My session', outline, previousStops: [] }) + + expect(prompt).toContain('My session') + expect(prompt).toContain('Assign every hunk id') + expect(prompt).toContain(`at most ${WALKTHROUGH_MAX_STOPS} stops`) + expect(prompt).toContain(outline) + }) + + it('lists previous stops when given', () => { + const prompt = buildWalkthroughPlanPrompt({ + title: 'My session', + outline, + previousStops: [{ title: 'A', hunkIds: ['h_1'] }], + }) + + expect(prompt).toContain('Previous stops') + expect(prompt).toContain('A: h_1') + }) + + it('drops previous stops when they would push the prompt over the diff budget', () => { + const prompt = buildWalkthroughPlanPrompt({ + title: 'My session', + outline, + previousStops: [{ title: 'T'.repeat(WALKTHROUGH_DIFF_MAX_CHARS), hunkIds: ['h_1'] }], + }) + + expect(prompt).not.toContain('Previous stops') + expect(prompt).toContain(outline) + expect(prompt.length).toBeLessThanOrEqual(WALKTHROUGH_DIFF_MAX_CHARS) + }) +}) + +describe('parseWalkthroughPlan', () => { const hunks: WalkthroughHunk[] = [ - { - id: 'f0h0', - file: 'src/a.ts', - status: 'modified', - header: '@@ -1,2 +1,2 @@', - text: hunkPatch(1), - truncated: false, - }, + hunkFixture('src/a.ts', hunkPatch(1)), + hunkFixture('src/a.ts', hunkPatch(2)), + hunkFixture('src/b.ts', hunkPatch(3)), ] + const idA0 = hunks[0]!.id + const idA1 = hunks[1]!.id + const idB0 = hunks[2]!.id + + it('drops unknown ids and repeat references, first occurrence wins', () => { + const parsed = parseWalkthroughPlan( + planReply([ + { title: 'A', hunkIds: [idA0, 'nope'] }, + { title: 'B', hunkIds: [idA0, idA1] }, + ]), + hunks, + ) + + expect(parsed?.stops.map((stop) => stop.hunkIds)).toEqual([[idA0], [idA1], [idB0]]) + }) + + it('drops stops left empty', () => { + const parsed = parseWalkthroughPlan( + planReply([ + { title: 'A', hunkIds: ['nope'] }, + { title: 'B', hunkIds: [idA0, idA1, idB0] }, + ]), + hunks, + ) + + expect(parsed?.stops.map((stop) => stop.title)).toEqual(['B']) + }) + + it('appends unreferenced hunks as a pending remaining stop', () => { + const parsed = parseWalkthroughPlan(planReply([{ title: 'A', hunkIds: [idA0] }]), hunks) + + expect(parsed?.stops.at(-1)).toEqual({ + id: computeStopId([idA1, idB0]), + title: 'Remaining changes', + explanation: '', + hunkIds: [idA1, idB0], + status: 'pending', + explanationKey: null, + }) + }) + + it('does not append a remaining stop when every hunk is covered', () => { + const parsed = parseWalkthroughPlan( + planReply([{ title: 'A', hunkIds: [idA0, idA1, idB0] }]), + hunks, + ) + + expect(parsed?.stops).toHaveLength(1) + expect(parsed?.stops[0]!.title).toBe('A') + }) + + it('caps model stops at 20 and truncates titles', () => { + const manyHunks: WalkthroughHunk[] = Array.from( + { length: WALKTHROUGH_MAX_STOPS + 5 }, + (_, index) => hunkFixture('src/a.ts', hunkPatch(index)), + ) + const stops = manyHunks.map((hunk) => ({ + title: 't'.repeat(WALKTHROUGH_TEXT_MAX_CHARS + 50), + hunkIds: [hunk.id], + })) + const parsed = parseWalkthroughPlan(planReply(stops), manyHunks) + + const modelStops = parsed!.stops.filter((stop) => stop.title !== 'Remaining changes') + expect(modelStops).toHaveLength(WALKTHROUGH_MAX_STOPS) + expect(modelStops[0]!.title.length).toBeLessThanOrEqual(WALKTHROUGH_TEXT_MAX_CHARS) + }) + + it('marks model stops pending with an empty explanation', () => { + const parsed = parseWalkthroughPlan( + planReply([ + { title: 'A', hunkIds: [idA0] }, + { title: 'B', hunkIds: [idA1] }, + ]), + hunks, + ) + + expect(parsed!.stops.every((stop) => stop.status === 'pending')).toBe(true) + expect(parsed!.stops.every((stop) => stop.explanation === '')).toBe(true) + }) + + it('returns null when the response is unparseable', () => { + expect(parseWalkthroughPlan('not json at all', hunks)).toBeNull() + expect(parseWalkthroughPlan('{not json', hunks)).toBeNull() + expect(parseWalkthroughPlan('{"summary":"x"}', hunks)).toBeNull() + }) + + it('returns null when no model stop survives', () => { + expect(parseWalkthroughPlan(planReply([{ title: 'A', hunkIds: ['nope'] }]), hunks)).toBeNull() + }) +}) + +describe('buildWalkthroughPrompt', () => { + const hunks: WalkthroughHunk[] = [hunkFixture('src/a.ts', hunkPatch(1))] it('lists every hunk with its id, file and status in a diff block', () => { const prompt = buildWalkthroughPrompt({ title: 'My session', hunks }) - expect(prompt).toContain('### f0h0 src/a.ts (modified)') + expect(prompt).toContain(`### ${hunks[0]!.id} src/a.ts (modified)`) expect(prompt).toContain('```diff') expect(prompt).toContain(hunkPatch(1)) expect(prompt).toContain('My session') @@ -261,32 +817,32 @@ describe('buildWalkthroughPrompt', () => { }) describe('parseWalkthroughResponse', () => { - const hunks: WalkthroughHunk[] = ['f0h0', 'f0h1', 'f1h0'].map((id, index) => ({ - id, - file: index === 2 ? 'src/b.ts' : 'src/a.ts', - status: 'modified', - header: '@@ -1,2 +1,2 @@', - text: hunkPatch(index + 1), - truncated: false, - })) + const hunks: WalkthroughHunk[] = [ + hunkFixture('src/a.ts', hunkPatch(1)), + hunkFixture('src/a.ts', hunkPatch(2)), + hunkFixture('src/b.ts', hunkPatch(3)), + ] + const idA0 = hunks[0]!.id + const idA1 = hunks[1]!.id + const idB0 = hunks[2]!.id it('drops unknown ids and repeat references, first occurrence wins', () => { const parsed = parseWalkthroughResponse( modelReply([ - { title: 'A', explanation: 'first', hunkIds: ['f0h0', 'nope'] }, - { title: 'B', explanation: 'second', hunkIds: ['f0h0', 'f0h1'] }, + { title: 'A', explanation: 'first', hunkIds: [idA0, 'nope'] }, + { title: 'B', explanation: 'second', hunkIds: [idA0, idA1] }, ]), hunks, ) - expect(parsed?.stops.map((stop) => stop.hunkIds)).toEqual([['f0h0'], ['f0h1'], ['f1h0']]) + expect(parsed?.stops.map((stop) => stop.hunkIds)).toEqual([[idA0], [idA1], [idB0]]) }) it('drops stops left empty', () => { const parsed = parseWalkthroughResponse( modelReply([ { title: 'A', explanation: 'first', hunkIds: ['nope'] }, - { title: 'B', explanation: 'second', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }, + { title: 'B', explanation: 'second', hunkIds: [idA0, idA1, idB0] }, ]), hunks, ) @@ -296,20 +852,23 @@ describe('parseWalkthroughResponse', () => { it('appends unreferenced hunks as a final remaining stop', () => { const parsed = parseWalkthroughResponse( - modelReply([{ title: 'A', explanation: 'first', hunkIds: ['f0h0'] }]), + modelReply([{ title: 'A', explanation: 'first', hunkIds: [idA0] }]), hunks, ) expect(parsed?.stops.at(-1)).toEqual({ + id: computeStopId([idA1, idB0]), title: 'Remaining changes', explanation: 'These changes were not covered by the generated walkthrough.', - hunkIds: ['f0h1', 'f1h0'], + hunkIds: [idA1, idB0], + status: 'ready', + explanationKey: null, }) }) it('does not append a remaining stop when every hunk is covered', () => { const parsed = parseWalkthroughResponse( - modelReply([{ title: 'A', explanation: 'first', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }]), + modelReply([{ title: 'A', explanation: 'first', hunkIds: [idA0, idA1, idB0] }]), hunks, ) @@ -320,14 +879,7 @@ describe('parseWalkthroughResponse', () => { it('caps model stops and text lengths', () => { const manyHunks: WalkthroughHunk[] = Array.from( { length: WALKTHROUGH_MAX_STOPS + 5 }, - (_, index) => ({ - id: `f0h${index}`, - file: 'src/a.ts', - status: 'modified', - header: '@@ -1 +1 @@', - text: hunkPatch(index), - truncated: false, - }), + (_, index) => hunkFixture('src/a.ts', hunkPatch(index)), ) const stops = manyHunks.map((hunk, index) => ({ title: `Stop ${index}`, @@ -361,17 +913,18 @@ describe('ChangeWalkthroughService', () => { let sessions: Record let fake: ReturnType let service: ChangeWalkthroughService + let gitService: GitService + let fetchRemoteRef: ReturnType - const threeHunks: FileDiffInfo[] = [ - change('src/a.ts', `${hunkPatch(1)}\n${hunkPatch(10)}`), - change('src/b.ts', hunkPatch(1)), - ] + const threeHunks: FileDiffInfo[] = THREE_HUNKS beforeEach(() => { db = createTestDb() sessions = { [SESSION_ID]: { changes: threeHunks } } fake = createFakeClient(sessions) - service = new ChangeWalkthroughService(db, fake.client) + fetchRemoteRef = vi.fn(async () => {}) + gitService = { fetchRemoteRef } as unknown as GitService + service = new ChangeWalkthroughService(db, fake.client, new SettingsService(db), createGitAuthService(), gitService) }) afterEach(() => { @@ -393,7 +946,7 @@ describe('ChangeWalkthroughService', () => { }) it('reports stale when the changes differ from the stored hash', async () => { - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) await service.generate(SESSION_ID, {}) sessions[SESSION_ID]!.changes = [change('src/a.ts', hunkPatch(99))] @@ -411,74 +964,322 @@ describe('ChangeWalkthroughService', () => { expect(state.stale).toBe(false) }) - it('stores stops covering every hunk exactly once', async () => { - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f1h0'] }])) + it('does not read changes while generating', async () => { + let resolveGenerate: (text: string) => void = () => {} + fake.setGenerateImpl(() => new Promise((resolve) => { + resolveGenerate = resolve + })) - const { walkthrough, created } = await service.generate(SESSION_ID, {}) + const pending = service.generate(SESSION_ID, {}) + await vi.waitFor(() => expect(fake.generateCalls).toHaveLength(1)) - expect(created).toBe(true) - const ids = walkthrough.stops.flatMap((stop) => stop.hunkIds) - expect(new Set(ids).size).toBe(ids.length) - expect(new Set(ids)).toEqual(new Set(['f0h0', 'f0h1', 'f1h0'])) - expect(walkthrough.stops.at(-1)!.title).toBe('Remaining changes') - }) + const diff = vi.mocked(fake.client.api.session.diff) + const messages = vi.mocked(fake.client.api.message.list) + const diffCalls = diff.mock.calls.length + const messageCalls = messages.mock.calls.length - it('returns the stored walkthrough without a model call when changes are unchanged', async () => { - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) - const first = await service.generate(SESSION_ID, {}) + await service.getState(SESSION_ID) + await service.getState(SESSION_ID) - const second = await service.generate(SESSION_ID, {}) + expect(diff.mock.calls.length).toBe(diffCalls) + expect(messages.mock.calls.length).toBe(messageCalls) - expect(second.created).toBe(false) - expect(second.walkthrough).toEqual(first.walkthrough) - expect(fake.generateCalls).toHaveLength(1) + resolveGenerate(modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + await pending }) - it('calls the model again when regenerate is set', async () => { - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) - await service.generate(SESSION_ID, {}) - - await service.generate(SESSION_ID, { regenerate: true }) + it('caches the current hash until a session execution event', async () => { + await service.getState(SESSION_ID) + await service.getState(SESSION_ID) - expect(fake.generateCalls).toHaveLength(2) + expect(vi.mocked(fake.client.api.session.diff)).toHaveBeenCalledTimes(1) + expect(vi.mocked(fake.client.api.message.list)).toHaveBeenCalledTimes(2) }) - it('generates with the session selected model', async () => { - sessions[SESSION_ID]!.info = { - id: SESSION_ID, - title: 'Title', - model: { providerID: 'anthropic', id: 'claude-sonnet-4' }, - } as SessionInfo - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) - + it('recomputes the hash after session.execution.succeeded', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) await service.generate(SESSION_ID, {}) - expect(fake.generateModels[0]).toEqual({ providerID: 'anthropic', id: 'claude-sonnet-4' }) - }) + await service.getState(SESSION_ID) + sessions[SESSION_ID]!.changes = [change('src/a.ts', hunkPatch(99))] - it('falls back to the resolved default when the session has no model', async () => { - fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) + const cached = await service.getState(SESSION_ID) + expect(cached.stale).toBe(false) - await service.generate(SESSION_ID, {}) + service.handleEvent(sessionEvent('session.execution.succeeded', SESSION_ID)) - expect(fake.generateModels[0]).toEqual({ providerID: 'openai', id: 'gpt-5-mini' }) + const refreshed = await service.getState(SESSION_ID) + expect(refreshed.stale).toBe(true) + expect(refreshed.currentDiffHash).toBe(computeChangesHash([change('src/a.ts', hunkPatch(99))])) }) - it('coalesces concurrent generation into one model call', async () => { - let resolveGenerate: (text: string) => void = () => {} - const gate = new Promise((resolve) => { - resolveGenerate = resolve - }) - fake.setGenerateImpl(() => gate) + it.each(['session.revert.staged', 'session.revert.committed', 'session.revert.cleared'])( + 'recomputes the hash after %s', + async (eventType) => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + await service.generate(SESSION_ID, {}) - const first = service.generate(SESSION_ID, {}) - const second = service.generate(SESSION_ID, {}) - resolveGenerate(modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }])) + await service.getState(SESSION_ID) + sessions[SESSION_ID]!.changes = [change('src/a.ts', hunkPatch(99))] - const [firstResult, secondResult] = await Promise.all([first, second]) - expect(firstResult).toBe(secondResult) - expect(fake.generateCalls).toHaveLength(1) - }) + const cached = await service.getState(SESSION_ID) + expect(cached.stale).toBe(false) + + service.handleEvent(sessionEvent(eventType, SESSION_ID)) + + const refreshed = await service.getState(SESSION_ID) + expect(refreshed.stale).toBe(true) + expect(refreshed.currentDiffHash).toBe(computeChangesHash([change('src/a.ts', hunkPatch(99))])) + }, + ) + + it('stores stops covering every hunk exactly once', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: [THREE_HUNK_IDS[0]!, THREE_HUNK_IDS[2]!] }])) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + const ids = walkthrough.stops.flatMap((stop) => stop.hunkIds) + expect(new Set(ids).size).toBe(ids.length) + expect(new Set(ids)).toEqual(new Set(THREE_HUNK_IDS)) + expect(walkthrough.stops.at(-1)!.title).toBe('Remaining changes') + }) + + it('keeps mechanical hunks out of the prompt and appends a mechanical stop', async () => { + const lockText = hunkPatch(7) + sessions[SESSION_ID]!.changes = [change('src/a.ts', hunkPatch(1)), change('pnpm-lock.yaml', lockText)] + fake.setGenerateImpl(async () => + modelReply([{ title: 'A', explanation: 'x', hunkIds: [computeHunkId('src/a.ts', 'modified', hunkPatch(1))] }]), + ) + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(fake.generateCalls).toHaveLength(1) + expect(fake.generateCalls[0]).not.toContain('pnpm-lock.yaml') + const mechanical = walkthrough.stops.at(-1)! + expect(mechanical.title).toBe('Mechanical changes') + expect(mechanical.hunkIds).toEqual([computeHunkId('pnpm-lock.yaml', 'modified', lockText)]) + + const referenced = walkthrough.stops.flatMap((stop) => stop.hunkIds) + expect(new Set(referenced).size).toBe(referenced.length) + expect(new Set(referenced)).toEqual(new Set(walkthrough.hunks.map((hunk) => hunk.id))) + }) + + it('bounds stored mechanical text yet lists every mechanical file with counts', async () => { + const oversizedPatch = Array.from({ length: 40 }, (_, index) => bigHunk(index)).join('\n') + sessions[SESSION_ID]!.changes = [ + change('src/a.ts', hunkPatch(1)), + { file: 'pnpm-lock.yaml', patch: oversizedPatch, additions: 1200, deletions: 800, status: 'modified' }, + { file: 'src/__snapshots__/a.ts.snap', patch: hunkPatch(2), additions: 2, deletions: 3, status: 'modified' }, + ] + fake.setGenerateImpl(async () => + modelReply([{ title: 'A', explanation: 'x', hunkIds: [computeHunkId('src/a.ts', 'modified', hunkPatch(1))] }]), + ) + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + const mechanicalHunks = walkthrough.hunks.filter((hunk) => isMechanicalChangePath(hunk.file)) + expect(mechanicalHunks.map((hunk) => hunk.file)).toEqual([ + 'pnpm-lock.yaml', + 'src/__snapshots__/a.ts.snap', + ]) + expect(mechanicalHunks.reduce((total, hunk) => total + hunk.text.length, 0)).toBeLessThanOrEqual( + MECHANICAL_TEXT_BUDGET, + ) + expect(mechanicalHunks[0]).toMatchObject({ text: '', additions: 1200, deletions: 800 }) + expect(mechanicalHunks[1]).toMatchObject({ additions: 2, deletions: 3 }) + + const mechanicalStop = walkthrough.stops.find((stop) => stop.title === 'Mechanical changes')! + expect(new Set(mechanicalStop.hunkIds)).toEqual(new Set(mechanicalHunks.map((hunk) => hunk.id))) + }) + + it('stores only a mechanical stop without a model call', async () => { + sessions[SESSION_ID]!.changes = [ + change('pnpm-lock.yaml', hunkPatch(1)), + change('src/__snapshots__/a.ts.snap', hunkPatch(2)), + ] + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + expect(fake.generateCalls).toHaveLength(0) + expect(walkthrough.summary).toBe('Only lock files, snapshots or generated files changed.') + expect(walkthrough.stops).toHaveLength(1) + expect(walkthrough.stops[0]!.title).toBe('Mechanical changes') + expect(walkthrough.stops[0]!.hunkIds).toHaveLength(2) + expect(new Set(walkthrough.stops[0]!.hunkIds)).toEqual( + new Set(walkthrough.hunks.map((hunk) => hunk.id)), + ) + }) + + it('writes a truthful summary when source files are omitted but mechanical files changed', async () => { + const file = 'src/huge.ts' + const patch = Array.from( + { length: 2000 }, + (_, index) => `@@ -${index + 1},1 +${index + 1},1 @@\n+line${index}`, + ).join('\n') + sessions[SESSION_ID]!.changes = [change(file, patch), change('pnpm-lock.yaml', hunkPatch(1))] + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(walkthrough.summary).not.toBe('Only lock files, snapshots or generated files changed.') + expect(walkthrough.stops.map((stop) => stop.title)).toContain('Mechanical changes') + expect(fake.generateCalls).toHaveLength(0) + }) + + it('does not claim mechanical-only when a binary source file was omitted', async () => { + sessions[SESSION_ID]!.changes = [ + change('assets/logo.png', 'diff --git a/assets/logo.png b/assets/logo.png\nBinary files differ'), + change('pnpm-lock.yaml', hunkPatch(1)), + ] + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(walkthrough.summary).not.toBe('Only lock files, snapshots or generated files changed.') + expect(walkthrough.stops[0]!.title).toBe('Mechanical changes') + }) + + it('never reuses the single-call remaining placeholder as a real explanation', async () => { + const remainingIds = [THREE_HUNK_IDS[1]!, THREE_HUNK_IDS[2]!] + fake.setGenerateImpl(async () => + modelReply([{ title: 'A', explanation: 'x', hunkIds: [THREE_HUNK_IDS[0]!] }]), + ) + + const first = await service.generate(SESSION_ID, {}) + const firstRemaining = first.walkthrough.stops.at(-1)! + expect(firstRemaining.title).toBe('Remaining changes') + expect(firstRemaining.explanationKey).toBeNull() + + sessions[SESSION_ID]!.changes = [ + change('src/a.ts', `${hunkPatch(99)}\n${hunkPatch(10)}`), + change('src/b.ts', hunkPatch(1)), + ] + fake.setGenerateImpl(async () => modelReply([{ title: 'B', explanation: 'fresh', hunkIds: remainingIds }])) + + const second = await service.generate(SESSION_ID, {}) + + const stopB = second.walkthrough.stops.find((stop) => stop.title === 'B')! + expect(stopB.explanation).toBe('fresh') + const placeholderStop = second.walkthrough.stops.find( + (stop) => stop.explanation === 'These changes were not covered by the generated walkthrough.', + ) + expect(placeholderStop?.explanationKey).toBeNull() + }) + + it('returns the stored walkthrough without a model call when changes are unchanged', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + const first = await service.generate(SESSION_ID, {}) + + const second = await service.generate(SESSION_ID, {}) + + expect(second.created).toBe(false) + expect(second.walkthrough).toEqual(first.walkthrough) + expect(fake.generateCalls).toHaveLength(1) + expect(vi.mocked(fake.client.api.model.list)).toHaveBeenCalledTimes(1) + }) + + it('calls the model again when regenerate is set', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + await service.generate(SESSION_ID, {}) + + await service.generate(SESSION_ID, { regenerate: true }) + + expect(fake.generateCalls).toHaveLength(2) + }) + + it('generates with the session selected model', async () => { + sessions[SESSION_ID]!.info = { + id: SESSION_ID, + title: 'Title', + model: { providerID: 'anthropic', id: 'claude-sonnet-4' }, + } as SessionInfo + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + await service.generate(SESSION_ID, {}) + + expect(fake.generateModels[0]).toEqual({ providerID: 'anthropic', id: 'claude-sonnet-4' }) + }) + + it('falls back to the resolved default when the session has no model', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + await service.generate(SESSION_ID, {}) + + expect(fake.generateModels[0]).toEqual({ providerID: 'openai', id: 'gpt-5-mini' }) + }) + + it('uses the walkthrough model preference', async () => { + new SettingsService(db).updateSettings({ walkthroughModel: 'openai/gpt-5-mini' }) + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(fake.generateModels.every((model) => model?.providerID === 'openai' && model.id === 'gpt-5-mini')).toBe(true) + expect(fake.generateModels.length).toBeGreaterThan(0) + expect(walkthrough.model).toBe('openai/gpt-5-mini') + }) + + it('ignores an invalid walkthrough model preference', async () => { + new SettingsService(db).updateSettings({ walkthroughModel: 'not-a-ref' }) + sessions[SESSION_ID]!.info = { + id: SESSION_ID, + title: 'Title', + model: { providerID: 'anthropic', id: 'claude-sonnet-4' }, + } as SessionInfo + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + await service.generate(SESSION_ID, {}) + + expect(fake.generateModels[0]).toEqual({ providerID: 'anthropic', id: 'claude-sonnet-4' }) + }) + + it('resolves the default model once per generation', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : EXPLANATION_REPLY), + ) + + await service.generate(SESSION_ID, {}) + + expect(vi.mocked(fake.client.api.model.list)).toHaveBeenCalledTimes(1) + }) + + it('includes the resolved default model in the explanation key', async () => { + fake.setGenerateImpl(async () => modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + const first = await service.generate(SESSION_ID, {}) + const firstKey = first.walkthrough.stops[0]!.explanationKey + + vi.mocked(fake.client.api.model.list).mockResolvedValue({ + data: [{ providerID: 'openai', id: 'gpt-5', enabled: true }], + } as never) + vi.mocked(fake.client.api.model.default).mockResolvedValue({ + data: { providerID: 'openai', id: 'gpt-5', enabled: true }, + } as never) + + const second = await service.generate(SESSION_ID, { regenerate: true }) + const secondKey = second.walkthrough.stops[0]!.explanationKey + + expect(firstKey).not.toBeNull() + expect(secondKey).not.toBeNull() + expect(firstKey).not.toBe(secondKey) + }) + + it('coalesces concurrent generation into one model call', async () => { + let resolveGenerate: (text: string) => void = () => {} + const gate = new Promise((resolve) => { + resolveGenerate = resolve + }) + fake.setGenerateImpl(() => gate) + + const first = service.generate(SESSION_ID, {}) + const second = service.generate(SESSION_ID, {}) + resolveGenerate(modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }])) + + const [firstResult, secondResult] = await Promise.all([first, second]) + expect(firstResult).toBe(secondResult) + expect(fake.generateCalls).toHaveLength(1) + }) it('rejects with 409 when the session has no changes', async () => { sessions[SESSION_ID]!.changes = [] @@ -489,6 +1290,31 @@ describe('ChangeWalkthroughService', () => { }) }) + it('names the source in the no-changes message', async () => { + const repo = createChangedRepo() + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + + await expect(service.generate(SESSION_ID, { source: { kind: 'staged' } })).rejects.toMatchObject({ + status: 409, + code: 'WALKTHROUGH_NO_CHANGES', + message: 'The staged changes contain no changes to walk through', + }) + }) + + it('names the source in the no-text-changes message', async () => { + const repo = path.join(walkthroughRepoRoot, uniqueName('repo')) + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'logo.png'), Buffer.from([0x00, 0x89, 0x50, 0x4e, 0x47])) + git(['add', 'logo.png'], repo) + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + + await expect(service.generate(SESSION_ID, { source: { kind: 'staged' } })).rejects.toMatchObject({ + status: 409, + code: 'WALKTHROUGH_NO_TEXT_CHANGES', + message: 'The staged changes contain no text changes', + }) + }) + it('rejects with 409 when every change is binary', async () => { sessions[SESSION_ID]!.changes = [ change('assets/logo.png', 'diff --git a/assets/logo.png b/assets/logo.png\nBinary files differ'), @@ -500,9 +1326,12 @@ describe('ChangeWalkthroughService', () => { }) }) - it('rejects with 413 and the omitted files when a text file exceeds the context budget', async () => { + it('rejects with 413 and the omitted files when the outline admits no hunk', async () => { const file = 'src/huge.ts' - const patch = Array.from({ length: 10 }, (_, index) => bigHunk(index)).join('\n') + const patch = Array.from( + { length: 2000 }, + (_, index) => `@@ -${index + 1},1 +${index + 1},1 @@\n+line${index}`, + ).join('\n') sessions[SESSION_ID]!.changes = [change(file, patch)] await expect(service.generate(SESSION_ID, {})).rejects.toMatchObject({ @@ -520,16 +1349,18 @@ describe('ChangeWalkthroughService', () => { status: 502, code: 'WALKTHROUGH_UNPARSEABLE', }) + expect(fake.generateCalls).toHaveLength(2) }) it('rejects with 504 when generation times out', async () => { - const timingOut = new ChangeWalkthroughService(db, fake.client, { timeoutMs: 5 }) + const timingOut = new ChangeWalkthroughService(db, fake.client, new SettingsService(db), createGitAuthService(), gitService, { timeoutMs: 5 }) fake.setGenerateImpl(() => new Promise(() => {})) await expect(timingOut.generate(SESSION_ID, {})).rejects.toMatchObject({ status: 504, code: 'WALKTHROUGH_TIMEOUT', }) + expect(fake.generateCalls).toHaveLength(2) }) it('rejects with 502 when the model call fails', async () => { @@ -538,6 +1369,38 @@ describe('ChangeWalkthroughService', () => { }) await expect(service.generate(SESSION_ID, {})).rejects.toMatchObject({ status: 502 }) + expect(fake.generateCalls).toHaveLength(2) + }) + + it('retries an unparseable reply once', async () => { + let calls = 0 + fake.setGenerateImpl(async () => { + calls += 1 + return calls === 1 ? 'not json' : modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }]) + }) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + expect(fake.generateCalls).toHaveLength(2) + expect(walkthrough.stops).toHaveLength(1) + }) + + it('retries a failed call once', async () => { + let calls = 0 + fake.setGenerateImpl(async () => { + calls += 1 + if (calls === 1) { + throw new Error('model unavailable') + } + return modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }]) + }) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + expect(fake.generateCalls).toHaveLength(2) + expect(walkthrough.stops).toHaveLength(1) }) it('rejects with 502 when reading changes fails', async () => { @@ -554,8 +1417,279 @@ describe('ChangeWalkthroughService', () => { await expect(service.generate('ses_missing', {})).rejects.toMatchObject({ status: 404 }) }) + it('plans then explains each stop', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : EXPLANATION_REPLY), + ) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + expect(walkthrough.stops.map((stop) => stop.title)).toEqual(['A', 'B', 'C']) + expect(walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + expect(walkthrough.stops.every((stop) => stop.explanation === 'Why')).toBe(true) + expect(fake.generateCalls).toHaveLength(4) + expect(fake.generateCalls.filter((prompt) => prompt.includes(PLAN_MARKER))).toHaveLength(1) + }) + + it('exposes pending stops while explaining', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + const explainResolvers: Array<(text: string) => void> = [] + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + return new Promise((resolve) => { + explainResolvers.push(resolve) + }) + }) + + const pending = service.generate(SESSION_ID, {}) + + await vi.waitFor(async () => { + const state = await service.getState(SESSION_ID) + expect(state.generating).toBe(true) + expect(state.walkthrough?.stops).toHaveLength(3) + expect(state.walkthrough?.stops.every((stop) => stop.status === 'pending')).toBe(true) + }) + + await vi.waitFor(() => expect(explainResolvers).toHaveLength(3)) + for (const resolve of explainResolvers) { + resolve(EXPLANATION_REPLY) + } + + const { walkthrough } = await pending + expect(walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + }) + + it('marks a stop failed when its explanation fails twice', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + if (prompt.includes('## Stop: B')) { + return Promise.resolve('not json') + } + return Promise.resolve(EXPLANATION_REPLY) + }) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + const byTitle = new Map(walkthrough.stops.map((stop) => [stop.title, stop])) + expect(byTitle.get('B')?.status).toBe('failed') + expect(byTitle.get('A')?.status).toBe('ready') + expect(byTitle.get('C')?.status).toBe('ready') + expect(fake.generateCalls.filter((prompt) => prompt.includes('## Stop: B'))).toHaveLength(2) + }) + + it('reuses explanations for unchanged stops after an edit', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + const first = await service.generate(SESSION_ID, {}) + expect(first.walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + const callsAfterFirst = fake.generateCalls.length + expect(callsAfterFirst).toBe(4) + + sessions[SESSION_ID]!.changes = EDITED_LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? EDITED_LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + const second = await service.generate(SESSION_ID, {}) + + expect(second.created).toBe(true) + const secondCalls = fake.generateCalls.slice(callsAfterFirst) + expect(secondCalls).toHaveLength(2) + expect(secondCalls.filter((prompt) => prompt.includes(PLAN_MARKER))).toHaveLength(1) + expect(secondCalls.filter((prompt) => prompt.includes('## Stop: A'))).toHaveLength(1) + expect(secondCalls.filter((prompt) => prompt.includes('## Stop: B'))).toHaveLength(0) + expect(secondCalls.filter((prompt) => prompt.includes('## Stop: C'))).toHaveLength(0) + + const byTitle = new Map(second.walkthrough.stops.map((stop) => [stop.title, stop])) + expect(byTitle.get('A')!.explanation).toBe('explained-A') + expect(byTitle.get('B')!.explanation).toBe('explained-B') + expect(byTitle.get('C')!.explanation).toBe('explained-C') + expect(second.walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + }) + + it('retries only failed stops on an unchanged diff', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + if (prompt.includes('## Stop: B')) { + return Promise.resolve('not json') + } + return Promise.resolve(explanationFor(prompt)) + }) + + const first = await service.generate(SESSION_ID, {}) + const firstByTitle = new Map(first.walkthrough.stops.map((stop) => [stop.title, stop])) + expect(firstByTitle.get('B')!.status).toBe('failed') + const callsAfterFirst = fake.generateCalls.length + + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + const second = await service.generate(SESSION_ID, {}) + + expect(second.created).toBe(true) + const secondCalls = fake.generateCalls.slice(callsAfterFirst) + expect(secondCalls).toHaveLength(1) + expect(secondCalls[0]).toContain('## Stop: B') + expect(secondCalls.filter((prompt) => prompt.includes(PLAN_MARKER))).toHaveLength(0) + + const secondByTitle = new Map(second.walkthrough.stops.map((stop) => [stop.title, stop])) + expect(secondByTitle.get('B')!.status).toBe('ready') + expect(secondByTitle.get('A')!.explanation).toBe('explained-A') + expect(secondByTitle.get('C')!.explanation).toBe('explained-C') + }) + + it('regenerate ignores previous explanations', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + await service.generate(SESSION_ID, {}) + const callsAfterFirst = fake.generateCalls.length + + await service.generate(SESSION_ID, { regenerate: true }) + + expect(fake.generateCalls.length - callsAfterFirst).toBe(4) + }) + + it('marks resumed stops pending while they are being explained', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + if (prompt.includes('## Stop: B')) { + return Promise.resolve('not json') + } + return Promise.resolve(explanationFor(prompt)) + }) + + const first = await service.generate(SESSION_ID, {}) + expect(first.walkthrough.stops.find((stop) => stop.title === 'B')!.status).toBe('failed') + + let resolveExplain: (text: string) => void = () => {} + fake.setGenerateImpl((prompt) => { + if (prompt.includes('## Stop: B')) { + return new Promise((resolve) => { + resolveExplain = resolve + }) + } + return Promise.resolve(explanationFor(prompt)) + }) + + const pending = service.generate(SESSION_ID, {}) + + await vi.waitFor(async () => { + const state = await service.getState(SESSION_ID) + expect(state.walkthrough?.stops.find((stop) => stop.title === 'B')?.status).toBe('pending') + }) + + resolveExplain(EXPLANATION_REPLY) + const resumed = await pending + expect(resumed.walkthrough.stops.find((stop) => stop.title === 'B')!.status).toBe('ready') + }) + + it('does not reuse explanations across models', async () => { + sessions[SESSION_ID]!.info = { + id: SESSION_ID, + title: 'Title', + model: { providerID: 'anthropic', id: 'claude-sonnet-4' }, + } as SessionInfo + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + await service.generate(SESSION_ID, {}) + const callsAfterFirst = fake.generateCalls.length + + sessions[SESSION_ID]!.info = { + id: SESSION_ID, + title: 'Title', + model: { providerID: 'openai', id: 'gpt-5' }, + } as SessionInfo + sessions[SESSION_ID]!.changes = EDITED_LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? EDITED_LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + await service.generate(SESSION_ID, {}) + + expect(fake.generateCalls.length - callsAfterFirst).toBe(4) + }) + + it('lists surviving previous stops in the plan prompt', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + await service.generate(SESSION_ID, {}) + const callsAfterFirst = fake.generateCalls.length + + sessions[SESSION_ID]!.changes = EDITED_LARGE_CHANGES + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? EDITED_LARGE_PLAN_REPLY : explanationFor(prompt)), + ) + + await service.generate(SESSION_ID, {}) + + const planPrompt = fake.generateCalls.slice(callsAfterFirst).find((prompt) => prompt.includes(PLAN_MARKER)) + expect(planPrompt).toBeDefined() + expect(planPrompt).toContain('Previous stops') + expect(planPrompt).toContain('- B:') + expect(planPrompt).toContain('- C:') + expect(planPrompt).not.toContain('- A:') + }) + + it('keeps a single call for small diffs', async () => { + fake.setGenerateImpl(async () => + modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }]), + ) + + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(fake.generateCalls).toHaveLength(1) + expect(walkthrough.stops).toHaveLength(1) + expect(walkthrough.stops[0]!.status).toBe('ready') + }) + + it('does not cache a diff read that an execution event invalidated', async () => { + const diffMock = vi.mocked(fake.client.api.session.diff) + let resolveDiff: (changes: FileDiffInfo[]) => void = () => {} + diffMock.mockImplementationOnce( + () => new Promise((resolve) => { + resolveDiff = resolve + }), + ) + + const pending = service.getState(SESSION_ID) + await vi.waitFor(() => expect(diffMock).toHaveBeenCalledTimes(1)) + service.handleEvent(sessionEvent('session.execution.succeeded', SESSION_ID)) + resolveDiff(threeHunks) + await pending + + await service.getState(SESSION_ID) + expect(diffMock).toHaveBeenCalledTimes(2) + }) + describe('startGeneration', () => { - const coveringReply = modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }]) + const coveringReply = modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }]) it('reports generating while the model call is pending, then the stored walkthrough', async () => { let resolveGenerate: (text: string) => void = () => {} @@ -568,6 +1702,7 @@ describe('ChangeWalkthroughService', () => { expect(state.walkthrough).toBeNull() expect(state.error).toBeNull() + await vi.waitFor(() => expect(fake.generateCalls).toHaveLength(1)) resolveGenerate(coveringReply) await vi.waitFor(async () => { @@ -577,6 +1712,17 @@ describe('ChangeWalkthroughService', () => { }) }) + it('stores a mechanical-only walkthrough without a model call', async () => { + sessions[SESSION_ID]!.changes = [change('pnpm-lock.yaml', hunkPatch(1))] + + const state = await service.startGeneration(SESSION_ID, {}) + + expect(fake.generateCalls).toHaveLength(0) + expect(state.generating).toBe(false) + expect(state.walkthrough?.stops).toHaveLength(1) + expect(state.walkthrough?.stops[0]!.title).toBe('Mechanical changes') + }) + it('rejects with 409 when the session has no changes', async () => { sessions[SESSION_ID]!.changes = [] @@ -607,11 +1753,19 @@ describe('ChangeWalkthroughService', () => { expect((await service.getState(SESSION_ID)).error).not.toBeNull() }) - fake.setGenerateImpl(() => new Promise(() => {})) + let resolveGenerate: (text: string) => void = () => {} + fake.setGenerateImpl(() => new Promise((resolve) => { + resolveGenerate = resolve + })) const state = await service.startGeneration(SESSION_ID, { regenerate: true }) expect(state.generating).toBe(true) expect(state.error).toBeNull() + + resolveGenerate(coveringReply) + await vi.waitFor(async () => { + expect((await service.getState(SESSION_ID)).generating).toBe(false) + }) }) it('clears the recorded error when the session is deleted', async () => { @@ -628,7 +1782,7 @@ describe('ChangeWalkthroughService', () => { }) describe('handleEvent', () => { - const coveringReply = modelReply([{ title: 'A', explanation: 'x', hunkIds: ['f0h0', 'f0h1', 'f1h0'] }]) + const coveringReply = modelReply([{ title: 'A', explanation: 'x', hunkIds: THREE_HUNK_IDS }]) it('deletes the stored walkthrough when its session is deleted', async () => { fake.setGenerateImpl(async () => coveringReply) @@ -636,7 +1790,7 @@ describe('ChangeWalkthroughService', () => { service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) - expect(getChangeWalkthrough(db, SESSION_ID)).toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, 'session')).toBeNull() }) it('keeps the stored walkthrough for other session events', async () => { @@ -646,7 +1800,7 @@ describe('ChangeWalkthroughService', () => { service.handleEvent(sessionEvent('session.execution.succeeded', SESSION_ID)) service.handleEvent(sessionEvent('session.deleted', 'ses_other')) - expect(getChangeWalkthrough(db, SESSION_ID)).not.toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, 'session')).not.toBeNull() }) it('does not store a walkthrough whose session was deleted during generation', async () => { @@ -661,7 +1815,7 @@ describe('ChangeWalkthroughService', () => { resolveGenerate(coveringReply) await expect(pending).rejects.toMatchObject({ status: 404 }) - expect(getChangeWalkthrough(db, SESSION_ID)).toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, 'session')).toBeNull() }) it('stores walkthroughs again after an earlier deletion of the same session id', async () => { @@ -670,7 +1824,431 @@ describe('ChangeWalkthroughService', () => { await service.generate(SESSION_ID, {}) - expect(getChangeWalkthrough(db, SESSION_ID)).not.toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, 'session')).not.toBeNull() + }) + + it('skips queued stop explanations once the session is deleted', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + const planStops = Array.from({ length: 8 }, (_, index) => { + const file = index < 4 ? 'src/a.ts' : 'src/b.ts' + const hunk = index < 4 ? index : index - 4 + 10 + return { title: `S${index}`, hunkIds: [computeHunkId(file, 'modified', bigHunk(hunk))] } + }) + const explainResolvers: Array<(text: string) => void> = [] + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(planReply(planStops)) + } + return new Promise((resolve) => { + explainResolvers.push(resolve) + }) + }) + + const pending = service.generate(SESSION_ID, {}) + + await vi.waitFor(() => expect(explainResolvers).toHaveLength(4)) + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + for (const resolve of explainResolvers) { + resolve(EXPLANATION_REPLY) + } + + await expect(pending).rejects.toMatchObject({ status: 404 }) + expect(fake.generateCalls.filter((prompt) => prompt.includes('## Stop:'))).toHaveLength(4) + }) + + it('clears the cached hash and its epoch when the session is deleted', async () => { + fake.setGenerateImpl(async () => coveringReply) + await service.generate(SESSION_ID, {}) + await service.getState(SESSION_ID) + + const internals = service as unknown as { + currentHashes: Map + hashEpochs: Map + } + expect(internals.currentHashes.size).toBeGreaterThan(0) + + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + + expect(internals.currentHashes.size).toBe(0) + expect(internals.hashEpochs.size).toBe(0) + }) + + it('does not track a hash epoch for a session with no cached hash', () => { + service.handleEvent(sessionEvent('session.execution.succeeded', SESSION_ID)) + + const internals = service as unknown as { hashEpochs: Map } + expect(internals.hashEpochs.size).toBe(0) + }) + + it('does not cache a hash from overlapping reads invalidated between completions', async () => { + const diffMock = vi.mocked(fake.client.api.session.diff) + const resolvers: Array<(changes: FileDiffInfo[]) => void> = [] + diffMock.mockImplementation(() => new Promise((resolve) => { + resolvers.push(resolve) + })) + + const first = service.getState(SESSION_ID) + const second = service.getState(SESSION_ID) + await vi.waitFor(() => expect(resolvers).toHaveLength(2)) + + service.handleEvent(sessionEvent('session.execution.succeeded', SESSION_ID)) + + resolvers[0]!(threeHunks) + resolvers[1]!(threeHunks) + await Promise.all([first, second]) + + const internals = service as unknown as { + currentHashes: Map + hashEpochs: Map + } + expect(internals.currentHashes.size).toBe(0) + expect(internals.hashEpochs.size).toBe(0) + + diffMock.mockResolvedValue([change('src/a.ts', hunkPatch(99))]) + const refreshed = await service.getState(SESSION_ID) + expect(refreshed.currentDiffHash).toBe(computeChangesHash([change('src/a.ts', hunkPatch(99))])) + }) + + it('drops a hash read that completes after the session is deleted', async () => { + const diffMock = vi.mocked(fake.client.api.session.diff) + let resolveDiff: (changes: FileDiffInfo[]) => void = () => {} + diffMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveDiff = resolve + })) + + const pending = service.getState(SESSION_ID) + await vi.waitFor(() => expect(diffMock).toHaveBeenCalledTimes(1)) + + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + resolveDiff(threeHunks) + await pending + + const internals = service as unknown as { + currentHashes: Map + hashEpochs: Map + pendingHashReads: Map + } + expect(internals.currentHashes.size).toBe(0) + expect(internals.hashEpochs.size).toBe(0) + expect(internals.pendingHashReads.size).toBe(0) + }) + }) + + describe('per-source storage', () => { + it('stores walkthroughs per source', async () => { + const repo = createChangedRepo() + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt)) + + await service.generate(SESSION_ID, {}) + await service.generate(SESSION_ID, { source: { kind: 'uncommitted' } }) + + const sessionStored = getChangeWalkthrough(db, SESSION_ID, 'session') + const uncommittedStored = getChangeWalkthrough(db, SESSION_ID, 'uncommitted') + expect(sessionStored).not.toBeNull() + expect(uncommittedStored).not.toBeNull() + expect(sessionStored!.diffHash).not.toBe(uncommittedStored!.diffHash) + expect(uncommittedStored!.source).toEqual({ kind: 'uncommitted' }) + }) + + it('deletes every source when the session is deleted', async () => { + const repo = createChangedRepo() + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt)) + + await service.generate(SESSION_ID, {}) + await service.generate(SESSION_ID, { source: { kind: 'uncommitted' } }) + + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + + expect(getChangeWalkthrough(db, SESSION_ID, 'session')).toBeNull() + expect(getChangeWalkthrough(db, SESSION_ID, 'uncommitted')).toBeNull() + }) + }) + + describe('pull request sources', () => { + it('does not fetch on GET and tolerates a missing pull request ref', async () => { + const repo = createChangedRepo() + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + + const state = await service.getState(SESSION_ID, { kind: 'pullRequest', number: 1, base: 'main' }) + + expect(fetchRemoteRef).not.toHaveBeenCalled() + expect(state.error).toBeNull() + expect(state.currentDiffHash).toBeNull() + expect(state.stale).toBe(false) + }) + + it('fetches once during generation, reads the ref on GET, and deletes it with the session', async () => { + const repo = createPullRequestRepo() + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + fetchRemoteRef.mockImplementation(async (target: { fullPath: string }, remote: string, refspecs: string[]) => { + git(['fetch', remote, ...refspecs], target.fullPath) + }) + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt)) + + const headRef = `refs/ocm-walkthrough/${SESSION_ID}/pr/1/head` + const baseRef = `refs/ocm-walkthrough/${SESSION_ID}/pr/1/base/main` + + await service.generate(SESSION_ID, { source: { kind: 'pullRequest', number: 1, base: 'main' } }) + + expect(fetchRemoteRef).toHaveBeenCalledTimes(1) + expect(refExists(repo, headRef)).toBe(true) + expect(refExists(repo, baseRef)).toBe(true) + + fetchRemoteRef.mockClear() + const state = await service.getState(SESSION_ID, { kind: 'pullRequest', number: 1, base: 'main' }) + + expect(fetchRemoteRef).not.toHaveBeenCalled() + expect(state.error).toBeNull() + expect(state.currentDiffHash).not.toBeNull() + + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + + await vi.waitFor(() => { + expect(refExists(repo, headRef)).toBe(false) + expect(refExists(repo, baseRef)).toBe(false) + }) + }) + + it('keeps another session refs when one session is deleted', async () => { + const repo = createPullRequestRepo() + const secondSessionId = 'ses_walkthrough_2' + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + sessions[secondSessionId] = { changes: threeHunks, info: sessionAt(repo, secondSessionId) } + fetchRemoteRef.mockImplementation(async (target: { fullPath: string }, remote: string, refspecs: string[]) => { + git(['fetch', remote, ...refspecs], target.fullPath) + }) + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt)) + + await service.generate(SESSION_ID, { source: { kind: 'pullRequest', number: 1, base: 'main' } }) + await service.generate(secondSessionId, { source: { kind: 'pullRequest', number: 1, base: 'main' } }) + + const firstHead = `refs/ocm-walkthrough/${SESSION_ID}/pr/1/head` + const firstBase = `refs/ocm-walkthrough/${SESSION_ID}/pr/1/base/main` + const secondHead = `refs/ocm-walkthrough/${secondSessionId}/pr/1/head` + const secondBase = `refs/ocm-walkthrough/${secondSessionId}/pr/1/base/main` + + expect(firstHead).not.toBe(secondHead) + expect(refExists(repo, firstHead)).toBe(true) + expect(refExists(repo, secondHead)).toBe(true) + + service.handleEvent(sessionEvent('session.deleted', SESSION_ID)) + + await vi.waitFor(() => { + expect(refExists(repo, firstHead)).toBe(false) + expect(refExists(repo, firstBase)).toBe(false) + }) + expect(refExists(repo, secondHead)).toBe(true) + expect(refExists(repo, secondBase)).toBe(true) + }) + + it('prefers the upstream remote when generating', async () => { + const repo = createPullRequestRepo() + const originUrl = git(['remote', 'get-url', 'origin'], repo) + git(['remote', 'add', 'upstream', originUrl], repo) + sessions[SESSION_ID] = { changes: threeHunks, info: sessionAt(repo) } + fake.setGenerateImpl(async (prompt) => replyForPrompt(prompt)) + + await service + .generate(SESSION_ID, { source: { kind: 'pullRequest', number: 1, base: 'main' } }) + .catch(() => {}) + + expect(fetchRemoteRef).toHaveBeenCalledTimes(1) + expect(fetchRemoteRef.mock.calls[0]?.[1]).toBe('upstream') + }) + }) + + describe('progressive persistence and in-memory state', () => { + it('writes once after the plan, every four explained stops, and at the end', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + const hunkIds = [ + ...[0, 1, 2, 3].map((index) => computeHunkId('src/a.ts', 'modified', bigHunk(index))), + ...[10, 11, 12, 13].map((index) => computeHunkId('src/b.ts', 'modified', bigHunk(index))), + ...[20, 21, 22, 23].map((index) => computeHunkId('src/c.ts', 'modified', bigHunk(index))), + ] + const planStops = Array.from({ length: 10 }, (_, index) => ({ title: `S${index}`, hunkIds: [] as string[] })) + hunkIds.forEach((id, index) => { + planStops[Math.min(index, planStops.length - 1)]!.hunkIds.push(id) + }) + + fake.setGenerateImpl((prompt) => + Promise.resolve(prompt.includes(PLAN_MARKER) ? planReply(planStops) : EXPLANATION_REPLY), + ) + + const prepare = vi.spyOn(db, 'prepare') + try { + const { walkthrough } = await service.generate(SESSION_ID, {}) + + expect(walkthrough.stops).toHaveLength(10) + expect(walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + + const writes = prepare.mock.calls.filter( + ([sql]) => typeof sql === 'string' && sql.includes('INSERT INTO change_walkthroughs'), + ).length + expect(writes).toBe(4) + } finally { + prepare.mockRestore() + } + }) + + it('serves an in-progress walkthrough from memory after its row is removed', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + const explainResolvers: Array<(text: string) => void> = [] + fake.setGenerateImpl((prompt) => { + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + return new Promise((resolve) => { + explainResolvers.push(resolve) + }) + }) + + const pending = service.generate(SESSION_ID, {}) + + await vi.waitFor(async () => { + const state = await service.getState(SESSION_ID) + expect(state.walkthrough?.stops).toHaveLength(3) + expect(state.walkthrough?.stops.every((stop) => stop.status === 'pending')).toBe(true) + }) + + deleteChangeWalkthroughs(db, SESSION_ID) + + const state = await service.getState(SESSION_ID) + expect(state.walkthrough?.stops).toHaveLength(3) + + await vi.waitFor(() => expect(explainResolvers).toHaveLength(3)) + for (const resolve of explainResolvers) { + resolve(EXPLANATION_REPLY) + } + await pending + }) + + it('resumes a walkthrough row saved after the plan', async () => { + sessions[SESSION_ID]!.changes = LARGE_CHANGES + const input = buildWalkthroughInput(LARGE_CHANGES) + const savedStops = input.hunks.slice(0, 3).map((hunk, index) => ({ + id: computeStopId([hunk.id]), + title: `S${index}`, + explanation: '', + hunkIds: [hunk.id], + status: 'pending' as const, + explanationKey: null, + })) + saveChangeWalkthrough(db, { + sessionId: SESSION_ID, + source: { kind: 'session' }, + diffHash: computeChangesHash(LARGE_CHANGES), + model: null, + summary: 'Saved plan', + stops: savedStops, + hunks: input.hunks, + omittedFiles: [], + createdAt: 1, + }) + fake.setGenerateImpl((prompt) => Promise.resolve(explanationFor(prompt))) + + const stored = getChangeWalkthrough(db, SESSION_ID, 'session') + expect(stored?.diffHash).toBe(computeChangesHash(LARGE_CHANGES)) + expect(stored?.stops).toHaveLength(3) + + const { walkthrough, created } = await service.generate(SESSION_ID, {}) + + expect(created).toBe(true) + expect(walkthrough.stops).toHaveLength(3) + expect(walkthrough.stops.every((stop) => stop.status === 'ready')).toBe(true) + expect(fake.generateCalls.filter((prompt) => prompt.includes(PLAN_MARKER))).toHaveLength(0) + expect(fake.generateCalls).toHaveLength(3) + }) + }) + + describe('global model call limiter', () => { + it('never exceeds four in-flight model calls across concurrent generations', async () => { + const secondSession = 'ses_walkthrough_second' + sessions[SESSION_ID] = { changes: LARGE_CHANGES } + sessions[secondSession] = { changes: LARGE_CHANGES } + + const planStops = Array.from({ length: 8 }, (_, index) => { + const file = index < 4 ? 'src/a.ts' : 'src/b.ts' + const hunk = index < 4 ? index : index - 4 + 10 + return { title: `S${index}`, hunkIds: [computeHunkId(file, 'modified', bigHunk(hunk))] } + }) + + let inFlight = 0 + let maxInFlight = 0 + fake.setGenerateImpl(async (prompt) => { + inFlight += 1 + maxInFlight = Math.max(maxInFlight, inFlight) + await new Promise((resolve) => setTimeout(resolve, 2)) + inFlight -= 1 + return prompt.includes(PLAN_MARKER) ? planReply(planStops) : EXPLANATION_REPLY + }) + + await Promise.all([service.generate(SESSION_ID, {}), service.generate(secondSession, {})]) + + expect(maxInFlight).toBeLessThanOrEqual(4) + expect(maxInFlight).toBeGreaterThanOrEqual(2) + }) + + it('skips a queued explain call when the session is deleted while waiting', async () => { + const victim = 'ses_walkthrough_victim' + const victimChanges = [ + change('victim/a.ts', Array.from({ length: 8 }, (_, index) => bigHunk(index)).join('\n')), + ] + const victimPlan = planReply( + Array.from({ length: 4 }, (_, group) => ({ + title: `V${group}`, + hunkIds: [group * 2, group * 2 + 1].map((hunk) => + computeHunkId('victim/a.ts', 'modified', bigHunk(hunk)), + ), + })), + ) + + sessions[SESSION_ID] = { changes: LARGE_CHANGES } + sessions[victim] = { changes: victimChanges } + + let saturatorExplainStarted = 0 + let victimExplainStarted = 0 + const saturatorResolvers: Array<(text: string) => void> = [] + const victimResolvers: Array<(text: string) => void> = [] + + fake.setGenerateImpl((prompt) => { + if (prompt.includes('victim/')) { + if (prompt.includes('## Stop:')) { + victimExplainStarted += 1 + return new Promise((resolve) => { + victimResolvers.push(resolve) + }) + } + return Promise.resolve(victimPlan) + } + if (prompt.includes(PLAN_MARKER)) { + return Promise.resolve(LARGE_PLAN_REPLY) + } + saturatorExplainStarted += 1 + return new Promise((resolve) => { + saturatorResolvers.push(resolve) + }) + }) + + const saturator = service.generate(SESSION_ID, {}) + await vi.waitFor(() => expect(saturatorExplainStarted).toBe(3)) + + const victimRun = service.generate(victim, {}) + await vi.waitFor(() => expect(victimExplainStarted).toBe(1)) + + service.handleEvent(sessionEvent('session.deleted', victim)) + + for (const resolve of saturatorResolvers) { + resolve(EXPLANATION_REPLY) + } + for (const resolve of victimResolvers) { + resolve(EXPLANATION_REPLY) + } + + await saturator + await expect(victimRun).rejects.toMatchObject({ status: 404 }) + expect(victimExplainStarted).toBe(1) }) }) }) diff --git a/backend/test/services/walkthrough-sources.test.ts b/backend/test/services/walkthrough-sources.test.ts new file mode 100644 index 000000000..d8d62cf0f --- /dev/null +++ b/backend/test/services/walkthrough-sources.test.ts @@ -0,0 +1,436 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { afterAll, describe, expect, it, vi } from 'vitest' +import type { FileDiffInfo, SessionInfo } from '@opencode-manager/shared/opencode' +import type { WalkthroughSource } from '@opencode-manager/shared/schemas' +import { + DEFAULT_WALKTHROUGH_SOURCE, + parseWalkthroughSourceKey, + walkthroughSourceKey, +} from '@opencode-manager/shared/schemas' +import { ChangeWalkthroughError } from '../../src/services/change-walkthrough-error' +import { + planPullRequestFetch, + readWalkthroughChanges, +} from '../../src/services/walkthrough-sources' +import type { OpenCodeClient } from '../../src/services/opencode/client' +import { cloneOrigin, createCommittedRepo, createOrigin, git, uniqueName } from '../helpers/git-fixtures' + +const TEST_SESSION_ID = 'ses_test' +const workspaceRoot = mkdtempSync(path.join(tmpdir(), 'walkthrough-sources-')) +process.env.GIT_CONFIG_NOSYSTEM = '1' +process.env.GIT_CONFIG_GLOBAL = '/dev/null' + +const GIT_ENV = { GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_TERMINAL_PROMPT: '0' } + +afterAll(() => { + rmSync(workspaceRoot, { recursive: true, force: true }) +}) + +function repoPath(prefix: string): string { + return path.join(workspaceRoot, uniqueName(prefix)) +} + +function sessionAt(directory: string): SessionInfo { + return { id: TEST_SESSION_ID, title: 'Test', location: { directory } } as SessionInfo +} + +function readAt(directory: string, source: WalkthroughSource): Promise { + return readWalkthroughChanges({ + client: {} as OpenCodeClient, + session: sessionAt(directory), + source, + gitEnv: GIT_ENV, + }) +} + +describe('readWalkthroughChanges', () => { + it('delegates the session source to the session diff', async () => { + const changes: FileDiffInfo[] = [ + { file: 'src/a.ts', patch: '@@ -1 +1 @@\n-a\n+b', additions: 1, deletions: 1, status: 'modified' }, + ] + const client = { + api: { + message: { list: vi.fn(async () => ({ data: [{ id: 'msg-1' }], cursor: {} })) }, + session: { diff: vi.fn(async () => changes) }, + }, + } as unknown as OpenCodeClient + + const result = await readWalkthroughChanges({ + client, + session: sessionAt('/not/a/repo'), + source: DEFAULT_WALKTHROUGH_SOURCE, + gitEnv: GIT_ENV, + }) + + expect(result).toEqual(changes) + }) + + it('reports staged-only edits only in staged', async () => { + const repo = repoPath('staged') + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'a.txt'), 'one\n') + git(['add', 'a.txt'], repo) + git(['commit', '-m', 'add a'], repo) + writeFileSync(path.join(repo, 'a.txt'), 'two\n') + git(['add', 'a.txt'], repo) + + const staged = await readAt(repo, { kind: 'staged' }) + const unstaged = await readAt(repo, { kind: 'unstaged' }) + const uncommitted = await readAt(repo, { kind: 'uncommitted' }) + + expect(staged.map((entry) => entry.file)).toEqual(['a.txt']) + expect(staged[0]?.status).toBe('modified') + expect(unstaged).toEqual([]) + expect(uncommitted.map((entry) => entry.file)).toEqual(['a.txt']) + }) + + it('reports an untracked file as added in uncommitted and unstaged', async () => { + const repo = repoPath('untracked') + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'new.txt'), 'hello\n') + + const uncommitted = await readAt(repo, { kind: 'uncommitted' }) + const unstaged = await readAt(repo, { kind: 'unstaged' }) + + expect(uncommitted).toHaveLength(1) + expect(uncommitted[0]).toMatchObject({ file: 'new.txt', status: 'added', additions: 1, deletions: 0 }) + expect(unstaged.map((entry) => entry.file)).toEqual(['new.txt']) + expect(unstaged[0]?.status).toBe('added') + }) + + it('parses untracked paths with spaces and non-ASCII characters', async () => { + const repo = repoPath('special-names') + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'my file.txt'), 'hello\n') + writeFileSync(path.join(repo, 'é.txt'), 'bonjour\n') + + const unstaged = await readAt(repo, { kind: 'unstaged' }) + + expect(unstaged.map((entry) => entry.file).sort()).toEqual(['my file.txt', 'é.txt']) + }) + + it('reports branch commits against the base', async () => { + const repo = repoPath('branch') + createCommittedRepo(repo, 'main') + git(['checkout', '-b', 'feature'], repo) + writeFileSync(path.join(repo, 'feature.txt'), 'feature\n') + git(['add', 'feature.txt'], repo) + git(['commit', '-m', 'feature'], repo) + + const explicit = await readAt(repo, { kind: 'branch', base: 'main' }) + const resolved = await readAt(repo, { kind: 'branch' }) + + expect(explicit.map((entry) => entry.file)).toEqual(['feature.txt']) + expect(explicit[0]?.status).toBe('added') + expect(resolved.map((entry) => entry.file)).toEqual(['feature.txt']) + }) + + it('reports a pull request ref pushed to the origin', async () => { + const origin = repoPath('origin') + const work = repoPath('work') + const clone = repoPath('clone') + createOrigin(origin, work) + cloneOrigin(origin, clone) + + git(['checkout', '-b', 'feature'], clone) + writeFileSync(path.join(clone, 'pr.txt'), 'pr\n') + git(['add', 'pr.txt'], clone) + git(['commit', '-m', 'pr'], clone) + git(['push', 'origin', 'HEAD:refs/pull/1/head'], clone) + + const source = { kind: 'pullRequest', number: 1, base: 'main' } as const + const plan = await planPullRequestFetch(clone, TEST_SESSION_ID, source) + git(['fetch', 'origin', ...plan.refspecs], clone) + + const changes = await readAt(clone, source) + + expect(changes.map((entry) => entry.file)).toEqual(['pr.txt']) + expect(changes[0]?.status).toBe('added') + }) + + it('diffs a pull request against the base from the remote that supplies the head', async () => { + const upstream = repoPath('pr-fork-upstream') + const upstreamWork = repoPath('pr-fork-upstream-work') + createOrigin(upstream, upstreamWork) + writeFileSync(path.join(upstreamWork, 'upstream.txt'), 'upstream\n') + git(['add', 'upstream.txt'], upstreamWork) + git(['commit', '-m', 'upstream advance'], upstreamWork) + git(['push', 'origin', 'main'], upstreamWork) + + const origin = repoPath('pr-fork-origin') + const originWork = repoPath('pr-fork-origin-work') + createOrigin(origin, originWork) + + const clone = repoPath('pr-fork-clone') + cloneOrigin(origin, clone) + git(['remote', 'add', 'upstream', upstream], clone) + git(['fetch', 'upstream'], clone) + + git(['checkout', '-b', 'feature', 'upstream/main'], clone) + writeFileSync(path.join(clone, 'pr.txt'), 'pr\n') + git(['add', 'pr.txt'], clone) + git(['commit', '-m', 'pr'], clone) + git(['push', 'upstream', 'HEAD:refs/pull/1/head'], clone) + + const source = { kind: 'pullRequest', number: 1, base: 'main' } as const + const plan = await planPullRequestFetch(clone, TEST_SESSION_ID, source) + expect(plan.remote).toBe('upstream') + git(['fetch', 'upstream', ...plan.refspecs], clone) + + const changes = await readAt(clone, source) + + expect(changes.map((entry) => entry.file)).toEqual(['pr.txt']) + }) + + it('does not fetch the pull request ref when reading changes', async () => { + const origin = repoPath('pr-no-fetch-origin') + const work = repoPath('pr-no-fetch-work') + const clone = repoPath('pr-no-fetch-clone') + createOrigin(origin, work) + cloneOrigin(origin, clone) + + git(['checkout', '-b', 'feature'], clone) + writeFileSync(path.join(clone, 'pr.txt'), 'pr\n') + git(['add', 'pr.txt'], clone) + git(['commit', '-m', 'pr'], clone) + git(['push', 'origin', 'HEAD:refs/pull/1/head'], clone) + + const error = await readAt(clone, { kind: 'pullRequest', number: 1, base: 'main' }).catch((thrown) => thrown) + + expect(error).toBeInstanceOf(ChangeWalkthroughError) + expect(error).toMatchObject({ status: 502, code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + }) + + it('prefers the upstream remote when it exists', async () => { + const repo = repoPath('pr-upstream') + createCommittedRepo(repo) + git(['remote', 'add', 'origin', 'https://example.com/base.git'], repo) + git(['remote', 'add', 'upstream', 'https://example.com/fork.git'], repo) + + const plan = await planPullRequestFetch(repo, TEST_SESSION_ID, { kind: 'pullRequest', number: 7, base: 'main' }) + + expect(plan.remote).toBe('upstream') + expect(plan.refspecs).toEqual([ + `+refs/pull/7/head:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/7/head`, + `+refs/heads/main:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/7/base/main`, + ]) + expect(plan.headRef).toBe(`refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/7/head`) + expect(plan.baseRef).toBe(`refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/7/base/main`) + }) + + it('falls back to the origin remote when no upstream exists', async () => { + const repo = repoPath('pr-origin') + createCommittedRepo(repo) + git(['remote', 'add', 'origin', 'https://example.com/base.git'], repo) + + const plan = await planPullRequestFetch(repo, TEST_SESSION_ID, { kind: 'pullRequest', number: 3, base: 'main' }) + + expect(plan.remote).toBe('origin') + expect(plan.refspecs).toEqual([ + `+refs/pull/3/head:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/3/head`, + `+refs/heads/main:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/3/base/main`, + ]) + }) + + it('resolves the default base from the selected remote HEAD when no base is given', async () => { + const repo = repoPath('pr-default-base') + createCommittedRepo(repo) + git(['remote', 'add', 'origin', 'https://example.com/base.git'], repo) + git(['remote', 'add', 'upstream', 'https://example.com/fork.git'], repo) + git(['update-ref', 'refs/remotes/upstream/trunk', 'HEAD'], repo) + git(['symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/trunk'], repo) + + const plan = await planPullRequestFetch(repo, TEST_SESSION_ID, { kind: 'pullRequest', number: 9 }) + + expect(plan.remote).toBe('upstream') + expect(plan.refspecs).toEqual([ + `+refs/pull/9/head:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/9/head`, + `+refs/heads/trunk:refs/ocm-walkthrough/${TEST_SESSION_ID}/pr/9/base/trunk`, + ]) + }) + + it('excludes untracked files from staged and branch sources', async () => { + const repo = repoPath('not-untracked') + createCommittedRepo(repo, 'main') + git(['checkout', '-b', 'feature'], repo) + writeFileSync(path.join(repo, 'feature.txt'), 'feature\n') + git(['add', 'feature.txt'], repo) + git(['commit', '-m', 'feature'], repo) + writeFileSync(path.join(repo, 'loose.txt'), 'loose\n') + + const staged = await readAt(repo, { kind: 'staged' }) + const branch = await readAt(repo, { kind: 'branch', base: 'main' }) + + expect(staged).toEqual([]) + expect(branch.map((entry) => entry.file)).toEqual(['feature.txt']) + }) + + it('rejects a non-git directory with a 409 source error', async () => { + const dir = repoPath('plain') + mkdirSync(dir, { recursive: true }) + + const error = await readAt(dir, { kind: 'staged' }).catch((thrown) => thrown) + + expect(error).toBeInstanceOf(ChangeWalkthroughError) + expect(error).toMatchObject({ status: 409, code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + }) + + it('rejects a branch source when no base branch exists', async () => { + const repo = repoPath('nobase') + createCommittedRepo(repo, 'trunk') + + const error = await readAt(repo, { kind: 'branch' }).catch((thrown) => thrown) + + expect(error).toBeInstanceOf(ChangeWalkthroughError) + expect(error).toMatchObject({ status: 409, code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + }) + + it('resolves a base branch that only exists as a remote-tracking ref', async () => { + const origin = repoPath('base-origin') + const work = repoPath('base-work') + const clone = repoPath('base-clone') + createOrigin(origin, work, ['develop']) + cloneOrigin(origin, clone) + + git(['checkout', '-b', 'feature'], clone) + writeFileSync(path.join(clone, 'feature.txt'), 'feature\n') + git(['add', 'feature.txt'], clone) + git(['commit', '-m', 'feature'], clone) + + const changes = await readAt(clone, { kind: 'branch', base: 'develop' }) + + expect(changes.map((entry) => entry.file)).toEqual(['feature.txt']) + }) + + it('rejects a branch source when the diff fails after the base resolves', async () => { + const repo = repoPath('diff-failure') + mkdirSync(repo, { recursive: true }) + git(['init', '-b', 'main'], repo) + git(['config', 'user.email', 'test@test.com'], repo) + git(['config', 'user.name', 'Test'], repo) + writeFileSync(path.join(repo, 'base.txt'), 'base\n') + git(['add', 'base.txt'], repo) + git(['commit', '-m', 'base'], repo) + git(['checkout', '-b', 'feature'], repo) + writeFileSync(path.join(repo, 'feature.txt'), 'feature\n') + git(['add', 'feature.txt'], repo) + git(['commit', '-m', 'feature'], repo) + + const tree = git(['rev-parse', 'main^{tree}'], repo) + rmSync(path.join(repo, '.git', 'objects', tree.slice(0, 2), tree.slice(2)), { force: true }) + + const error = await readAt(repo, { kind: 'branch', base: 'main' }).catch((thrown) => thrown) + + expect(error).toBeInstanceOf(ChangeWalkthroughError) + expect(error).toMatchObject({ status: 502, code: 'WALKTHROUGH_SOURCE_UNAVAILABLE' }) + expect((error as Error).message).toContain('unable to read tree') + }) + + it('reads root-relative paths when diff prefix config changes the header', async () => { + for (const [key, value] of [['diff.mnemonicPrefix', 'true'], ['diff.noprefix', 'true']]) { + const repo = repoPath(`prefix-${key}`) + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'tracked.txt'), 'one\n') + git(['add', 'tracked.txt'], repo) + git(['commit', '-m', 'add tracked'], repo) + writeFileSync(path.join(repo, 'tracked.txt'), 'two\n') + writeFileSync(path.join(repo, 'untracked.txt'), 'new\n') + git(['config', key!, value!], repo) + + const unstaged = await readAt(repo, { kind: 'unstaged' }) + + expect(unstaged.map((entry) => entry.file).sort()).toEqual(['tracked.txt', 'untracked.txt']) + expect(unstaged.find((entry) => entry.file === 'tracked.txt')?.patch).toContain('-one') + expect(unstaged.find((entry) => entry.file === 'untracked.txt')?.status).toBe('added') + } + }) + + it('ignores an external diff driver for tracked and untracked files', async () => { + const repo = repoPath('external-diff') + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'tracked.txt'), 'one\n') + git(['add', 'tracked.txt'], repo) + git(['commit', '-m', 'add tracked'], repo) + writeFileSync(path.join(repo, 'tracked.txt'), 'two\n') + writeFileSync(path.join(repo, 'untracked.txt'), 'new\n') + git(['config', 'diff.external', 'false'], repo) + + const unstaged = await readAt(repo, { kind: 'unstaged' }) + + expect(unstaged.map((entry) => entry.file).sort()).toEqual(['tracked.txt', 'untracked.txt']) + }) + + it('reads unstaged worktree edits and uncommitted content without a HEAD commit', async () => { + const repo = repoPath('no-head') + mkdirSync(repo, { recursive: true }) + git(['init', '-b', 'main'], repo) + git(['config', 'user.email', 'test@test.com'], repo) + git(['config', 'user.name', 'Test'], repo) + writeFileSync(path.join(repo, 'a.txt'), 'one\n') + git(['add', 'a.txt'], repo) + writeFileSync(path.join(repo, 'a.txt'), 'two\n') + + const unstaged = await readAt(repo, { kind: 'unstaged' }) + const uncommitted = await readAt(repo, { kind: 'uncommitted' }) + + expect(unstaged.map((entry) => entry.file)).toEqual(['a.txt']) + expect(unstaged[0]?.patch).toContain('-one') + expect(unstaged[0]?.patch).toContain('+two') + expect(uncommitted.map((entry) => entry.file)).toEqual(['a.txt']) + expect(uncommitted[0]?.patch).toContain('+two') + expect(uncommitted[0]?.patch).not.toContain('+one') + }) + + it('includes every untracked file in a single uncommitted read', async () => { + const repo = repoPath('many-untracked') + createCommittedRepo(repo) + mkdirSync(path.join(repo, 'nested'), { recursive: true }) + writeFileSync(path.join(repo, 'b.txt'), 'b\n') + writeFileSync(path.join(repo, 'a.txt'), 'a\n') + writeFileSync(path.join(repo, 'nested', 'c.txt'), 'c\n') + + const uncommitted = await readAt(repo, { kind: 'uncommitted' }) + + expect(uncommitted.map((entry) => entry.file).sort()).toEqual(['a.txt', 'b.txt', 'nested/c.txt']) + expect(uncommitted.every((entry) => entry.status === 'added')).toBe(true) + }) + + it('rejects a diff larger than the output cap with a 413 error', async () => { + const repo = repoPath('too-large') + createCommittedRepo(repo) + writeFileSync(path.join(repo, 'huge.txt'), 'x'.repeat(6_000_000)) + + const error = await readAt(repo, { kind: 'unstaged' }).catch((thrown) => thrown) + + expect(error).toBeInstanceOf(ChangeWalkthroughError) + expect(error).toMatchObject({ status: 413, code: 'WALKTHROUGH_DIFF_TOO_LARGE' }) + }) +}) + +describe('walkthroughSourceKey', () => { + it('round-trips every source kind', () => { + const sources: WalkthroughSource[] = [ + { kind: 'session' }, + { kind: 'uncommitted' }, + { kind: 'staged' }, + { kind: 'unstaged' }, + { kind: 'branch' }, + { kind: 'branch', base: 'develop' }, + { kind: 'pullRequest', number: 12 }, + { kind: 'pullRequest', number: 12, base: 'develop' }, + ] + + for (const source of sources) { + expect(parseWalkthroughSourceKey(walkthroughSourceKey(source))).toEqual(source) + } + }) + + it('rejects invalid keys', () => { + expect(parseWalkthroughSourceKey('branch:-x')).toBeNull() + expect(parseWalkthroughSourceKey('pr:0:')).toBeNull() + expect(parseWalkthroughSourceKey('pr:abc:main')).toBeNull() + expect(parseWalkthroughSourceKey('nonsense')).toBeNull() + }) +}) diff --git a/backend/test/utils/concurrency.test.ts b/backend/test/utils/concurrency.test.ts new file mode 100644 index 000000000..3ced1a5be --- /dev/null +++ b/backend/test/utils/concurrency.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from 'vitest' +import { createLimiter, mapWithConcurrency } from '../../src/utils/concurrency' + +describe('mapWithConcurrency', () => { + it('keeps result order', async () => { + const result = await mapWithConcurrency([1, 2, 3, 4, 5], 2, async (item) => item * 2) + expect(result).toEqual([2, 4, 6, 8, 10]) + }) + + it('runs at most limit calls at once', async () => { + let inFlight = 0 + let maxInFlight = 0 + + await mapWithConcurrency([1, 2, 3, 4, 5, 6], 2, async (item) => { + inFlight += 1 + maxInFlight = Math.max(maxInFlight, inFlight) + await new Promise((resolve) => setTimeout(resolve, 1)) + inFlight -= 1 + return item + }) + + expect(maxInFlight).toBe(2) + }) + + it('propagates a rejection', async () => { + await expect( + mapWithConcurrency([1, 2, 3], 2, async (item) => { + if (item === 2) { + throw new Error('boom') + } + return item + }), + ).rejects.toThrow('boom') + }) + + it('passes the index to the callback', async () => { + const result = await mapWithConcurrency(['a', 'b'], 2, async (item, index) => `${item}${index}`) + expect(result).toEqual(['a0', 'b1']) + }) +}) + +describe('createLimiter', () => { + it('runs at most limit tasks at once', async () => { + const limiter = createLimiter(2) + let inFlight = 0 + let maxInFlight = 0 + + const task = async (value: number): Promise => { + inFlight += 1 + maxInFlight = Math.max(maxInFlight, inFlight) + await new Promise((resolve) => setTimeout(resolve, 1)) + inFlight -= 1 + return value + } + + const results = await Promise.all([1, 2, 3, 4, 5].map((value) => limiter.run(() => task(value)))) + + expect(results).toEqual([1, 2, 3, 4, 5]) + expect(maxInFlight).toBe(2) + expect(limiter.activeCount).toBe(0) + }) + + it('queues tasks beyond the limit until a slot frees', async () => { + const limiter = createLimiter(1) + const order: string[] = [] + let releaseFirst: () => void = () => {} + const first = limiter.run(async () => { + order.push('first') + await new Promise((resolve) => { + releaseFirst = resolve + }) + }) + + const second = limiter.run(async () => { + order.push('second') + }) + + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(order).toEqual(['first']) + releaseFirst() + await Promise.all([first, second]) + expect(order).toEqual(['first', 'second']) + }) + + it('releases the slot when a task rejects', async () => { + const limiter = createLimiter(1) + + await expect(limiter.run(async () => { + throw new Error('boom') + })).rejects.toThrow('boom') + + await expect(limiter.run(async () => 'ok')).resolves.toBe('ok') + expect(limiter.activeCount).toBe(0) + }) +}) diff --git a/backend/test/utils/json-extract.test.ts b/backend/test/utils/json-extract.test.ts index fc9be96bb..a99d67771 100644 --- a/backend/test/utils/json-extract.test.ts +++ b/backend/test/utils/json-extract.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest' -import { extractFirstJsonObject } from '../../src/utils/json-extract' +import { z } from 'zod' +import { extractFirstJsonObject, parseFirstJsonObject } from '../../src/utils/json-extract' describe('extractFirstJsonObject', () => { it('returns a plain JSON object', () => { @@ -38,3 +39,23 @@ describe('extractFirstJsonObject', () => { expect(extractFirstJsonObject('{"a":"unterminated}')).toBeNull() }) }) + +describe('parseFirstJsonObject', () => { + const schema = z.object({ a: z.number() }) + + it('parses and validates the first object', () => { + expect(parseFirstJsonObject('prefix {"a":1} suffix', schema)).toEqual({ a: 1 }) + }) + + it('returns null when there is no object', () => { + expect(parseFirstJsonObject('no object', schema)).toBeNull() + }) + + it('returns null for invalid JSON', () => { + expect(parseFirstJsonObject('{not json', schema)).toBeNull() + }) + + it('returns null when the object fails the schema', () => { + expect(parseFirstJsonObject('{"a":"nope"}', schema)).toBeNull() + }) +}) diff --git a/backend/test/utils/process.test.ts b/backend/test/utils/process.test.ts index 1d89e74aa..d284473f8 100644 --- a/backend/test/utils/process.test.ts +++ b/backend/test/utils/process.test.ts @@ -35,6 +35,16 @@ describe('executeCommand signal handling', () => { expect(result).toHaveLength(100) }) + it('flags truncated output on the structured result when exit codes are ignored', async () => { + const result = await executeCommand( + ['sh', '-c', 'i=0; while [ $i -lt 1000 ]; do printf "0123456789"; i=$((i+1)); done'], + { maxOutputChars: 100, silent: true, ignoreExitCode: true }, + ) + + expect(result.truncated).toBe(true) + expect(result.stdout).toHaveLength(100) + }) + it('returns the full stdout when it stays within maxOutputChars', async () => { const result = await executeCommand(['sh', '-c', 'printf "hello"'], { maxOutputChars: 100, silent: true }) diff --git a/frontend/src/api/changeWalkthroughs.ts b/frontend/src/api/changeWalkthroughs.ts index 352c318cf..cd193742a 100644 --- a/frontend/src/api/changeWalkthroughs.ts +++ b/frontend/src/api/changeWalkthroughs.ts @@ -1,16 +1,28 @@ import { fetchWrapper } from './fetchWrapper' import { API_BASE_URL } from '@/config' +import { walkthroughSourceKey } from '@opencode-manager/shared/schemas' import type { ChangeWalkthroughState, + ChangeWalkthroughStateWire, GenerateChangeWalkthroughRequest, + WalkthroughSource, } from '@opencode-manager/shared/schemas' function walkthroughUrl(sessionId: string): string { return `${API_BASE_URL}/api/change-walkthroughs/${encodeURIComponent(sessionId)}` } -export async function getChangeWalkthrough(sessionId: string): Promise { - return fetchWrapper(walkthroughUrl(sessionId)) +export async function getChangeWalkthrough( + sessionId: string, + source: WalkthroughSource, + hunksFor?: string, +): Promise { + return fetchWrapper(walkthroughUrl(sessionId), { + params: { + source: walkthroughSourceKey(source), + ...(hunksFor === undefined ? {} : { hunksFor }), + }, + }) } export async function generateChangeWalkthrough( diff --git a/frontend/src/components/navigation/ToolSidePanel.test.tsx b/frontend/src/components/navigation/ToolSidePanel.test.tsx index e8e4edd66..4c2a292bb 100644 --- a/frontend/src/components/navigation/ToolSidePanel.test.tsx +++ b/frontend/src/components/navigation/ToolSidePanel.test.tsx @@ -27,6 +27,7 @@ vi.mock('@/components/session/ChangesWalkthroughSheet', () => ({ ChangesWalkthroughProvider: ({ children }: { children: ReactNode }) => <>{children}, ChangesWalkthroughNav: () => null, ChangesWalkthroughRegenerate: () => null, + ChangesWalkthroughSourcePicker: () => null, ChangesWalkthroughView: () =>
walkthrough-tool
, })) diff --git a/frontend/src/components/navigation/ToolSidePanel.tsx b/frontend/src/components/navigation/ToolSidePanel.tsx index 1c4bd2b9f..26ae41bc5 100644 --- a/frontend/src/components/navigation/ToolSidePanel.tsx +++ b/frontend/src/components/navigation/ToolSidePanel.tsx @@ -8,6 +8,7 @@ import { ChangesWalkthroughNav, ChangesWalkthroughProvider, ChangesWalkthroughRegenerate, + ChangesWalkthroughSourcePicker, ChangesWalkthroughView, } from '@/components/session/ChangesWalkthroughSheet' import { PreviewWorkspace } from '@/components/preview/PreviewPanel' @@ -167,6 +168,7 @@ export function ToolSidePanel({

{activeLabel}

{activeTool === 'walkthrough' ? ( <> + @@ -188,7 +190,7 @@ export function ToolSidePanel({ <> {panelContent ? ( activeTool === 'walkthrough' && sessionId ? ( - + {panelContent} ) : ( diff --git a/frontend/src/components/repo/MultiRunSheet.test.tsx b/frontend/src/components/repo/MultiRunSheet.test.tsx index a91161219..1b47ae264 100644 --- a/frontend/src/components/repo/MultiRunSheet.test.tsx +++ b/frontend/src/components/repo/MultiRunSheet.test.tsx @@ -500,7 +500,9 @@ describe('MultiRunSheet', () => { await user.click(within(card).getByRole('button', { name: /walkthrough/i })) expect(await screen.findByText('Change walkthrough')).toBeInTheDocument() - await waitFor(() => expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_11')) + await waitFor(() => + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_11', { kind: 'session' }), + ) }) it('lists fusions with truncation and opens the fusion session', async () => { diff --git a/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx b/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx index 9b58ef338..97d139268 100644 --- a/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx +++ b/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest' -import { render, screen, waitFor } from '@testing-library/react' +import { render, screen, waitFor, within } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import type { ReactNode } from 'react' @@ -19,11 +19,27 @@ vi.mock('@/api/changeWalkthroughs', () => ({ const walkthrough: ChangeWalkthrough = { sessionId: 'ses_1', + source: { kind: 'session' }, diffHash: 'hash-1', + model: null, summary: 'This change adds a greeting.', stops: [ - { title: 'Add the greeting', explanation: 'Introduces the greeting helper.', hunkIds: ['f0h0'] }, - { title: 'Wire it up', explanation: 'Calls the helper from the entry point.', hunkIds: ['f1h0'] }, + { + id: 's_greet', + title: 'Add the greeting', + explanation: 'Introduces the greeting helper.', + hunkIds: ['f0h0'], + status: 'ready', + explanationKey: null, + }, + { + id: 's_wire', + title: 'Wire it up', + explanation: 'Calls the helper from the entry point.', + hunkIds: ['f1h0'], + status: 'ready', + explanationKey: null, + }, ], hunks: [ { @@ -47,6 +63,53 @@ const walkthrough: ChangeWalkthrough = { createdAt: 1, } +const mechanicalWalkthrough: ChangeWalkthrough = { + ...walkthrough, + stops: [ + { + id: 's_mech', + title: 'Mechanical changes', + explanation: 'Lock files, snapshots and generated files.', + hunkIds: ['m_lock', 'm_snap', 'm_map'], + status: 'ready', + explanationKey: null, + }, + ], + hunks: [ + { + id: 'm_lock', + file: 'pnpm-lock.yaml', + status: 'modified', + header: '@@ -1 +1 @@', + text: '@@ -1 +1 @@\n-old-lock\n+new-lock', + truncated: false, + additions: 120, + deletions: 80, + }, + { + id: 'm_snap', + file: 'src/__snapshots__/a.ts.snap', + status: 'added', + header: '', + text: '', + truncated: true, + additions: 5, + deletions: 0, + }, + { + id: 'm_map', + file: 'dist/app.js.map', + status: 'deleted', + header: '', + text: '', + truncated: true, + additions: 0, + deletions: 7, + }, + ], + omittedFiles: [], +} + function state(overrides: Partial = {}): ChangeWalkthroughState { return { walkthrough, currentDiffHash: 'hash-1', stale: false, generating: false, error: null, ...overrides } } @@ -96,7 +159,7 @@ describe('ChangesWalkthroughSheet', () => { await user.click(generate) await waitFor(() => { - expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', {}) + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { source: { kind: 'session' } }) }) expect(await screen.findByText('This change adds a greeting.')).toBeInTheDocument() expect(screen.getByText('Overview')).toBeInTheDocument() @@ -168,7 +231,7 @@ describe('ChangesWalkthroughSheet', () => { expect(screen.getByText('This change adds a greeting.')).toBeInTheDocument() }) - it('shows the stale warning and regenerates', async () => { + it('updates a stale walkthrough incrementally', async () => { const user = userEvent.setup() mocks.getChangeWalkthrough.mockResolvedValue(state({ stale: true, currentDiffHash: 'hash-2' })) mocks.generateChangeWalkthrough.mockResolvedValue(state({ generating: true })) @@ -178,10 +241,10 @@ describe('ChangesWalkthroughSheet', () => { await screen.findByText('Changes have been updated since this walkthrough was generated'), ).toBeInTheDocument() - await user.click(screen.getByRole('button', { name: 'Regenerate' })) + await user.click(screen.getByRole('button', { name: 'Update walkthrough' })) await waitFor(() => { - expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { regenerate: true }) + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { source: { kind: 'session' } }) }) }) @@ -195,10 +258,154 @@ describe('ChangesWalkthroughSheet', () => { await user.click(screen.getByRole('button', { name: 'Regenerate walkthrough' })) await waitFor(() => { - expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { regenerate: true }) + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { + regenerate: true, + source: { kind: 'session' }, + }) + }) + }) + + it('switches to staged changes', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + mocks.generateChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Staged' })) + + await waitFor(() => { + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_1', { kind: 'staged' }) + }) + + await user.click(await screen.findByRole('button', { name: 'Generate walkthrough' })) + + await waitFor(() => { + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { source: { kind: 'staged' } }) + }) + }) + + it('renders the source picker in the drawer header before any walkthrough exists', async () => { + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + renderSheet() + + const header = screen.getByRole('button', { name: 'Close' }).parentElement + expect(header).not.toBeNull() + expect( + within(header as HTMLElement).getByRole('combobox', { name: 'Changes to walk through' }), + ).toBeInTheDocument() + }) + + it('sends the branch base', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + mocks.generateChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Branch vs base' })) + + const base = await screen.findByLabelText('Base branch') + await user.type(base, 'develop') + await user.tab() + + await waitFor(() => { + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_1', { kind: 'branch', base: 'develop' }) + }) + + await user.click(screen.getByRole('button', { name: 'Generate walkthrough' })) + + await waitFor(() => { + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { + source: { kind: 'branch', base: 'develop' }, + }) }) }) + it('prompts for a pull request number and withholds the actions until it is valid', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + mocks.generateChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Pull request' })) + + const number = await screen.findByLabelText('Pull request number') + await user.type(number, '0') + await user.tab() + + expect(await screen.findByText('Enter a pull request number')).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Generate walkthrough' })).not.toBeInTheDocument() + expect(mocks.generateChangeWalkthrough).not.toHaveBeenCalled() + expect(mocks.getChangeWalkthrough).not.toHaveBeenCalledWith( + 'ses_1', + expect.objectContaining({ kind: 'pullRequest' }), + ) + }) + + it('commits a valid pull request number and restores the generate action', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + mocks.generateChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Pull request' })) + + const number = await screen.findByLabelText('Pull request number') + await user.type(number, '12') + await user.tab() + + await waitFor(() => { + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_1', { kind: 'pullRequest', number: 12 }) + }) + expect(screen.queryByText('Enter a pull request number')).not.toBeInTheDocument() + expect(await screen.findByRole('button', { name: 'Generate walkthrough' })).toBeEnabled() + }) + + it('resets the source before querying a new session', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + const { rerender, onOpenChange } = renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Staged' })) + await waitFor(() => { + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_1', { kind: 'staged' }) + }) + + rerender() + + await waitFor(() => { + expect(mocks.getChangeWalkthrough).toHaveBeenCalledWith('ses_2', { kind: 'session' }) + }) + expect(mocks.getChangeWalkthrough).not.toHaveBeenCalledWith('ses_2', { kind: 'staged' }) + }) + + it('clears a generation error when the source changes', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null })) + mocks.generateChangeWalkthrough.mockRejectedValue( + new FetchError('Not a git repository', 409, 'WALKTHROUGH_NOT_A_REPO'), + ) + renderSheet() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Branch vs base' })) + + await user.click(await screen.findByRole('button', { name: 'Generate walkthrough' })) + expect(await screen.findByText('Not a git repository')).toBeInTheDocument() + + await user.click(screen.getByRole('combobox', { name: 'Changes to walk through' })) + await user.click(await screen.findByRole('option', { name: 'Staged' })) + + await waitFor(() => { + expect(screen.queryByText('Not a git repository')).not.toBeInTheDocument() + }) + expect(await screen.findByRole('button', { name: 'Generate walkthrough' })).toBeVisible() + }) + it('scrolls back to the top of the overview when a walkthrough is regenerated', async () => { const user = userEvent.setup() const scrollTo = vi.spyOn(Element.prototype, 'scrollTo') @@ -228,6 +435,55 @@ describe('ChangesWalkthroughSheet', () => { expect(screen.queryByRole('button', { name: 'Generate walkthrough' })).not.toBeInTheDocument() }) + it('renders ready and pending stops while generating', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue( + state({ + generating: true, + walkthrough: { + ...walkthrough, + stops: [ + { ...walkthrough.stops[0], status: 'ready' }, + { ...walkthrough.stops[1], status: 'pending', explanation: '' }, + ], + }, + }), + ) + renderSheet() + + expect(await screen.findByText(/1 of 2 stops explained/)).toBeInTheDocument() + expect(screen.getByRole('button', { name: '1. Add the greeting' })).toBeInTheDocument() + expect(screen.getByLabelText('Explaining')).toBeInTheDocument() + + await user.click(screen.getByRole('button', { name: '1. Add the greeting' })) + expect(await screen.findByText('Introduces the greeting helper.')).toBeInTheDocument() + }) + + it('retries unexplained stops', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue( + state({ + walkthrough: { + ...walkthrough, + stops: [ + { ...walkthrough.stops[0], status: 'failed', explanation: '' }, + { ...walkthrough.stops[1], status: 'ready' }, + ], + }, + }), + ) + mocks.generateChangeWalkthrough.mockResolvedValue(state({ generating: true })) + renderSheet() + + await user.click(await screen.findByRole('button', { name: /1\. Add the greeting/ })) + expect(await screen.findByText('This stop could not be explained.')).toBeInTheDocument() + await user.click(screen.getByRole('button', { name: 'Retry unexplained stops' })) + + await waitFor(() => { + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', { source: { kind: 'session' } }) + }) + }) + it('renders a generation failure recorded by the server', async () => { mocks.getChangeWalkthrough.mockResolvedValue( state({ @@ -287,6 +543,81 @@ describe('ChangesWalkthroughSheet', () => { expect(screen.getByText('assets/logo.png — binary file')).toBeInTheDocument() }) + it('labels renamed and mode-change omitted files', async () => { + mocks.getChangeWalkthrough.mockResolvedValue( + state({ + walkthrough: { + ...walkthrough, + omittedFiles: [ + { file: 'src/old.ts', reason: 'renamed' }, + { file: 'script.sh', reason: 'modeChange' }, + ], + }, + }), + ) + renderSheet() + + expect(await screen.findByText('src/old.ts — renamed')).toBeInTheDocument() + expect(screen.getByText('script.sh — mode change')).toBeInTheDocument() + }) + + it('renders mechanical files as a compact summary list with counts and diff text for budgeted hunks', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: mechanicalWalkthrough })) + renderSheet() + + await user.click(await screen.findByRole('button', { name: '1. Mechanical changes' })) + expect( + await screen.findByRole('list', { name: 'Mechanical file summaries' }), + ).toBeInTheDocument() + expect(screen.getByText('pnpm-lock.yaml')).toBeInTheDocument() + expect(screen.getByText('src/__snapshots__/a.ts.snap')).toBeInTheDocument() + expect(screen.getByText('dist/app.js.map')).toBeInTheDocument() + expect(screen.getByText('+120')).toBeInTheDocument() + expect(screen.getByText('-80')).toBeInTheDocument() + expect(screen.getByText('+5')).toBeInTheDocument() + expect(screen.getByText('-7')).toBeInTheDocument() + expect(screen.getByText('new-lock')).toBeInTheDocument() + }) + + it('renders a stored mechanical stop whose hunks predate hunk counts', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue( + state({ + walkthrough: { + ...walkthrough, + stops: [ + { + id: 's_mech', + title: 'Mechanical changes', + explanation: 'Lock files', + hunkIds: ['m_lock'], + status: 'ready', + explanationKey: null, + }, + ], + hunks: [ + { + id: 'm_lock', + file: 'pnpm-lock.yaml', + status: 'modified', + header: '@@ -1 +1 @@', + text: '@@ -1 +1 @@\n-old-lock\n+new-lock', + truncated: false, + }, + ], + omittedFiles: [], + }, + }), + ) + renderSheet() + + await user.click(await screen.findByRole('button', { name: '1. Mechanical changes' })) + expect(await screen.findByText('pnpm-lock.yaml')).toBeInTheDocument() + expect(screen.getByText('new-lock')).toBeInTheDocument() + expect(screen.queryByRole('list', { name: 'Mechanical file summaries' })).not.toBeInTheDocument() + }) + it('renders raw HTML in the summary and stop explanation as inert text', async () => { const payload = '' @@ -295,7 +626,9 @@ describe('ChangesWalkthroughSheet', () => { walkthrough: { ...walkthrough, summary: `Summary ${payload}`, - stops: [{ title: 'Stop', explanation: `Explanation ${payload}`, hunkIds: ['f0h0'] }], + stops: [ + { id: 's_x', title: 'Stop', explanation: `Explanation ${payload}`, hunkIds: ['f0h0'], status: 'ready', explanationKey: null }, + ], }, }), ) diff --git a/frontend/src/components/session/ChangesWalkthroughSheet.tsx b/frontend/src/components/session/ChangesWalkthroughSheet.tsx index ab1abf1a6..dd204acfc 100644 --- a/frontend/src/components/session/ChangesWalkthroughSheet.tsx +++ b/frontend/src/components/session/ChangesWalkthroughSheet.tsx @@ -12,15 +12,24 @@ import { DropdownMenuTrigger, } from '@/components/ui/dropdown-menu' import { Badge } from '@/components/ui/badge' +import { Input } from '@/components/ui/input' +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select' import { DiffLines } from '@/components/file-browser/DiffLines' import { ScheduleRunMarkdown } from '@/components/schedules/ScheduleRunMarkdown' import { useChangeWalkthrough, useGenerateChangeWalkthrough } from '@/hooks/useChangeWalkthrough' import { GIT_STATUS_COLORS, GIT_STATUS_LABELS } from '@/lib/git-status-styles' -import { WalkthroughOmittedFileSchema } from '@opencode-manager/shared/schemas' +import { + DEFAULT_WALKTHROUGH_SOURCE, + describeWalkthroughSource, + toWalkthroughSource, + walkthroughSourceKey, + WalkthroughOmittedFileSchema, +} from '@opencode-manager/shared/schemas' import type { ChangeWalkthrough, WalkthroughHunk, WalkthroughOmittedFile, + WalkthroughSource, WalkthroughStop, } from '@opencode-manager/shared/schemas' @@ -62,6 +71,8 @@ function walkthroughErrorMessage(error: unknown): string { const OMITTED_REASON_LABELS: Record = { binary: 'binary file', budget: 'over the diff budget', + renamed: 'renamed', + modeChange: 'mode change', } function OmittedFiles({ files }: { files: WalkthroughOmittedFile[] }) { @@ -77,6 +88,36 @@ function OmittedFiles({ files }: { files: WalkthroughOmittedFile[] }) { ) } +function isMechanicalHunk(hunk: WalkthroughHunk): boolean { + return hunk.additions !== undefined || hunk.deletions !== undefined +} + +function MechanicalHunks({ hunks }: { hunks: WalkthroughHunk[] }) { + return ( +
    + {hunks.map((hunk) => ( +
  • +
    + + {hunk.file} + + + {GIT_STATUS_LABELS[hunk.status]} + + +{hunk.additions ?? 0} + -{hunk.deletions ?? 0} +
    + {hunk.text.length > 0 ? ( +
    + +
    + ) : null} +
    • + ))} +
+ ) +} + interface ChangesWalkthroughContextValue { isLoading: boolean walkthrough: ChangeWalkthrough | null @@ -92,6 +133,16 @@ interface ChangesWalkthroughContextValue { selectedHunks: WalkthroughHunk[] selectStop: (index: number | null) => void scrollRef: RefObject + failedStopCount: number + source: WalkthroughSource + setSource: (source: WalkthroughSource) => void + sourceKind: WalkthroughSource['kind'] + setSourceKind: (kind: WalkthroughSource['kind']) => void + baseInput: string + setBaseInput: (base: string) => void + numberInput: string + setNumberInput: (number: string) => void + sourcePending: boolean } const ChangesWalkthroughContext = createContext(null) @@ -102,6 +153,89 @@ function useChangesWalkthrough(): ChangesWalkthroughContextValue { return value } +const WALKTHROUGH_SOURCE_OPTIONS: { value: WalkthroughSource['kind']; label: string }[] = [ + { value: 'session', label: 'This session' }, + { value: 'uncommitted', label: 'Uncommitted' }, + { value: 'staged', label: 'Staged' }, + { value: 'unstaged', label: 'Unstaged' }, + { value: 'branch', label: 'Branch vs base' }, + { value: 'pullRequest', label: 'Pull request' }, +] + +/** Chooses which set of changes the walkthrough covers from the surrounding chrome; git sources commit as soon as they are valid. */ +export const ChangesWalkthroughSourcePicker = memo(function ChangesWalkthroughSourcePicker() { + const { sourceKind, setSourceKind, baseInput, setSource } = useChangesWalkthrough() + + const handleKindChange = useCallback( + (next: WalkthroughSource['kind']) => { + setSourceKind(next) + if (next === 'pullRequest') return + const nextSource = toWalkthroughSource({ kind: next, base: baseInput }) + if (nextSource) setSource(nextSource) + }, + [baseInput, setSourceKind, setSource], + ) + + return ( + + ) +}) + +/** Collects the base branch or pull request number for git-backed walkthrough sources. */ +const WalkthroughSourceInputs = memo(function WalkthroughSourceInputs() { + const { sourceKind, baseInput, setBaseInput, numberInput, setNumberInput, setSource } = + useChangesWalkthrough() + + const commitInputs = useCallback(() => { + const next = toWalkthroughSource({ kind: sourceKind, base: baseInput, number: Number(numberInput) }) + if (next) setSource(next) + }, [sourceKind, baseInput, numberInput, setSource]) + + if (sourceKind !== 'branch' && sourceKind !== 'pullRequest') return null + + return ( +
+ setBaseInput(event.target.value)} + onBlur={commitInputs} + onKeyDown={(event) => { + if (event.key === 'Enter') commitInputs() + }} + /> + + {sourceKind === 'pullRequest' ? ( + setNumberInput(event.target.value)} + onBlur={commitInputs} + onKeyDown={(event) => { + if (event.key === 'Enter') commitInputs() + }} + /> + ) : null} +
+ ) +}) + interface ChangesWalkthroughProviderProps { sessionId: string active: boolean @@ -110,8 +244,13 @@ interface ChangesWalkthroughProviderProps { /** Loads a session's change walkthrough and shares its state with the surrounding chrome and body. */ export function ChangesWalkthroughProvider({ sessionId, active, children }: ChangesWalkthroughProviderProps) { - const stateQuery = useChangeWalkthrough(sessionId, active) - const generate = useGenerateChangeWalkthrough(sessionId) + const [source, setSource] = useState(DEFAULT_WALKTHROUGH_SOURCE) + const [sourceKind, setSourceKind] = useState(DEFAULT_WALKTHROUGH_SOURCE.kind) + const [baseInput, setBaseInput] = useState('') + const [numberInput, setNumberInput] = useState('') + const sourceKey = walkthroughSourceKey(source) + const stateQuery = useChangeWalkthrough(sessionId, active, source) + const generate = useGenerateChangeWalkthrough(sessionId, source) const resetGenerate = generate.reset const [stopIndex, setStopIndex] = useState(null) const scrollRef = useRef(null) @@ -123,14 +262,19 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan const error = generate.error ?? stateQuery.error ?? (generating ? null : state?.error ?? null) const contextLimitFiles = isContextLimitError(error) ? omittedFilesFromError(error) : [] + const selectedSource = toWalkthroughSource({ kind: sourceKind, base: baseInput, number: Number(numberInput) }) + const sourcePending = + sourceKind === 'pullRequest' && + (selectedSource === null || walkthroughSourceKey(selectedSource) !== sourceKey) + useEffect(() => { resetGenerate() - }, [active, sessionId, resetGenerate]) + }, [active, sessionId, sourceKey, resetGenerate]) useEffect(() => { setStopIndex(null) scrollRef.current?.scrollTo?.({ top: 0 }) - }, [active, sessionId, walkthrough?.createdAt]) + }, [active, sessionId, sourceKey, walkthrough?.createdAt]) const hunksById = new Map(walkthrough?.hunks.map((hunk) => [hunk.id, hunk]) ?? []) const stops = walkthrough?.stops ?? [] @@ -140,6 +284,10 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan .map((id) => hunksById.get(id)) .filter((hunk): hunk is WalkthroughHunk => hunk !== undefined) + const failedStopCount = stops.filter( + (stop) => stop.status === 'failed' || (stop.status === 'pending' && !generating), + ).length + const selectStop = useCallback((index: number | null) => { setStopIndex(index) scrollRef.current?.scrollTo?.({ top: 0 }) @@ -160,6 +308,16 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan selectedHunks, selectStop, scrollRef, + failedStopCount, + source, + setSource, + sourceKind, + setSourceKind, + baseInput, + setBaseInput, + numberInput, + setNumberInput, + sourcePending, } return {children} @@ -219,7 +377,7 @@ export const ChangesWalkthroughNav = memo(function ChangesWalkthroughNav() { /** Regenerates the current walkthrough from the chrome; renders nothing until one exists. */ export const ChangesWalkthroughRegenerate = memo(function ChangesWalkthroughRegenerate() { - const { walkthrough, generating, regenerate } = useChangesWalkthrough() + const { walkthrough, generating, regenerate, sourcePending } = useChangesWalkthrough() if (!walkthrough) return null @@ -229,7 +387,7 @@ export const ChangesWalkthroughRegenerate = memo(function ChangesWalkthroughRege size="icon-sm" aria-label="Regenerate walkthrough" onClick={regenerate} - disabled={generating} + disabled={generating || sourcePending} > @@ -246,16 +404,33 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { error, contextLimitFiles, generate, - regenerate, stops, selectedStop, selectedHunks, selectStop, scrollRef, + failedStopCount, + source, + sourcePending, } = useChangesWalkthrough() + const readyStopCount = stops.filter((stop) => stop.status === 'ready').length + const mechanicalHunks = selectedHunks.filter(isMechanicalHunk) + const diffHunks = selectedHunks.filter((hunk) => !isMechanicalHunk(hunk)) + + if (sourcePending) { + return ( +
+ +

Enter a pull request number

+
+ ) + } + return (
+ + {error ? (

{walkthroughErrorMessage(error)}

@@ -266,7 +441,11 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { {generating ? (
- Generating walkthrough… this can take a minute or two. + + {walkthrough && stops.length > 0 + ? `Generating walkthrough… ${readyStopCount} of ${stops.length} stops explained` + : 'Generating walkthrough… this can take a minute or two.'} +
) : null} @@ -278,7 +457,7 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { generating ? null : (

- Generate a step-by-step walkthrough of the changes in this session. + Generate a step-by-step walkthrough of {describeWalkthroughSource(source)}.

@@ -290,9 +469,18 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() {

Changes have been updated since this walkthrough was generated

- +
+ ) : null} + + {failedStopCount > 0 && !generating ? ( +
+

Some stops could not be explained

+
) : null} @@ -300,8 +488,15 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { {selectedStop ? (

{selectedStop.title}

- - {selectedHunks.map((hunk) => ( + {selectedStop.status === 'ready' ? ( + + ) : ( +

+ {generating ? 'Explaining this stop…' : 'This stop could not be explained.'} +

+ )} + {mechanicalHunks.length > 0 ? : null} + {diffHunks.map((hunk) => (
@@ -325,13 +520,22 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { {stops.length > 0 ? (
    {stops.map((stop, index) => ( -
  1. +
    3. ))} @@ -359,12 +563,13 @@ export function ChangesWalkthroughSheet({ sessionId, open, onOpenChange }: Chang return ( - + + diff --git a/frontend/src/components/settings/GeneralSettings.test.tsx b/frontend/src/components/settings/GeneralSettings.test.tsx index 43ee69a14..4a5164843 100644 --- a/frontend/src/components/settings/GeneralSettings.test.tsx +++ b/frontend/src/components/settings/GeneralSettings.test.tsx @@ -70,6 +70,9 @@ describe('GeneralSettings', () => { expect(screen.getByRole('switch', { name: 'Show reasoning' })).not.toBeChecked() expect(screen.getByRole('switch', { name: 'Expand tool calls' })).not.toBeChecked() expect(screen.getByRole('switch', { name: 'Expand diffs' })).toBeChecked() + + expect(screen.getByRole('heading', { name: 'Change walkthrough' })).toBeInTheDocument() + expect(screen.getByText('Walkthrough model')).toBeInTheDocument() }) it('writes the theme and toggle preferences', async () => { diff --git a/frontend/src/components/settings/GeneralSettings.tsx b/frontend/src/components/settings/GeneralSettings.tsx index 8a62c13f4..45d3a278c 100644 --- a/frontend/src/components/settings/GeneralSettings.tsx +++ b/frontend/src/components/settings/GeneralSettings.tsx @@ -6,6 +6,7 @@ import { Label } from '@/components/ui/label' import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select' import { Switch } from '@/components/ui/switch' import { SessionAutomationSettings } from './SessionAutomationSettings' +import { WalkthroughSettings } from './WalkthroughSettings' export function GeneralSettings() { const { preferences, isLoading, updateSettings, isUpdating } = useSettings() @@ -196,6 +197,8 @@ export function GeneralSettings() { )} + +
) } diff --git a/frontend/src/components/settings/SessionAutomationSettings.tsx b/frontend/src/components/settings/SessionAutomationSettings.tsx index 1c41fcac9..d4912d9f7 100644 --- a/frontend/src/components/settings/SessionAutomationSettings.tsx +++ b/frontend/src/components/settings/SessionAutomationSettings.tsx @@ -1,5 +1,6 @@ -import { useCallback, useEffect, useRef, useState } from 'react' +import { useCallback } from 'react' import { useSettings } from '@/hooks/useSettings' +import { useAutosavedSetting } from '@/hooks/useAutosavedSetting' import { Label } from '@/components/ui/label' import { Input } from '@/components/ui/input' import { ModelCombobox } from '@/components/model/ModelCombobox' @@ -12,42 +13,37 @@ import { type SessionPermissionMode, } from '@opencode-manager/shared/schemas' -const AUTOSAVE_DELAY_MS = 800 +interface GoalDraft { + goalAuditorModel: string + goalMaxContinuations: string + goalTokenBudget: string +} + +interface GoalCommitted { + goalAuditorModel: string | undefined + goalMaxContinuations: number + goalTokenBudget: number | undefined +} + +function maxContinuationsFromInput(value: string): number | null { + const parsed = Number(value) + if (!Number.isInteger(parsed)) return null + if (parsed < GOAL_MAX_CONTINUATIONS_MIN || parsed > GOAL_MAX_CONTINUATIONS_MAX) return null + return parsed +} + +function tokenBudgetFromInput(value: string): { valid: boolean; value: number | undefined } { + const trimmed = value.trim() + if (trimmed === '') return { valid: true, value: undefined } + const parsed = Number(trimmed) + return Number.isInteger(parsed) && parsed > 0 ? { valid: true, value: parsed } : { valid: false, value: undefined } +} export function SessionAutomationSettings() { const { preferences, updateSettings } = useSettings() const sessionDefaults = preferences?.sessionDefaults const permissionMode = sessionDefaults?.permissionMode ?? DEFAULT_SESSION_DEFAULTS.permissionMode - const [goalAuditorModel, setGoalAuditorModel] = useState(sessionDefaults?.goalAuditorModel ?? '') - const [goalMaxContinuations, setGoalMaxContinuations] = useState( - String(sessionDefaults?.goalMaxContinuations ?? DEFAULT_SESSION_DEFAULTS.goalMaxContinuations), - ) - const [goalTokenBudget, setGoalTokenBudget] = useState( - sessionDefaults?.goalTokenBudget !== undefined ? String(sessionDefaults.goalTokenBudget) : '', - ) - - const committed = useRef({ - goalAuditorModel: sessionDefaults?.goalAuditorModel, - goalMaxContinuations: sessionDefaults?.goalMaxContinuations ?? DEFAULT_SESSION_DEFAULTS.goalMaxContinuations, - goalTokenBudget: sessionDefaults?.goalTokenBudget, - }) - - useEffect(() => { - setGoalAuditorModel(sessionDefaults?.goalAuditorModel ?? '') - setGoalMaxContinuations( - String(sessionDefaults?.goalMaxContinuations ?? DEFAULT_SESSION_DEFAULTS.goalMaxContinuations), - ) - setGoalTokenBudget( - sessionDefaults?.goalTokenBudget !== undefined ? String(sessionDefaults.goalTokenBudget) : '', - ) - committed.current = { - goalAuditorModel: sessionDefaults?.goalAuditorModel, - goalMaxContinuations: sessionDefaults?.goalMaxContinuations ?? DEFAULT_SESSION_DEFAULTS.goalMaxContinuations, - goalTokenBudget: sessionDefaults?.goalTokenBudget, - } - }, [sessionDefaults]) - const updateSessionDefaults = useCallback( (patch: Partial) => { updateSettings({ @@ -61,44 +57,46 @@ export function SessionAutomationSettings() { [sessionDefaults, permissionMode, updateSettings], ) - const commitGoalFields = useCallback(() => { - const patch: Partial = {} - - const nextAuditorModel = goalAuditorModel.trim() || undefined - if (nextAuditorModel !== committed.current.goalAuditorModel) { - committed.current.goalAuditorModel = nextAuditorModel - patch.goalAuditorModel = nextAuditorModel - } - - const nextMaxContinuations = Number(goalMaxContinuations) - if ( - Number.isInteger(nextMaxContinuations) && - nextMaxContinuations >= GOAL_MAX_CONTINUATIONS_MIN && - nextMaxContinuations <= GOAL_MAX_CONTINUATIONS_MAX && - nextMaxContinuations !== committed.current.goalMaxContinuations - ) { - committed.current.goalMaxContinuations = nextMaxContinuations - patch.goalMaxContinuations = nextMaxContinuations - } - - const nextTokenBudget = goalTokenBudget.trim() === '' ? undefined : Number(goalTokenBudget) - const tokenBudgetValid = nextTokenBudget === undefined || (Number.isInteger(nextTokenBudget) && nextTokenBudget > 0) - if (tokenBudgetValid && nextTokenBudget !== committed.current.goalTokenBudget) { - committed.current.goalTokenBudget = nextTokenBudget - patch.goalTokenBudget = nextTokenBudget - } - - if (Object.keys(patch).length === 0) return - updateSessionDefaults(patch) - }, [goalAuditorModel, goalMaxContinuations, goalTokenBudget, updateSessionDefaults]) - - useEffect(() => { - const timer = setTimeout(() => { - commitGoalFields() - }, AUTOSAVE_DELAY_MS) + const { draft, setDraft, commit: commitGoalFields } = useAutosavedSetting({ + stored: sessionDefaults, + toDraft: (stored) => ({ + goalAuditorModel: stored?.goalAuditorModel ?? '', + goalMaxContinuations: String(stored?.goalMaxContinuations ?? DEFAULT_SESSION_DEFAULTS.goalMaxContinuations), + goalTokenBudget: stored?.goalTokenBudget !== undefined ? String(stored.goalTokenBudget) : '', + }), + toCommitted: (current, previous) => { + const budget = tokenBudgetFromInput(current.goalTokenBudget) + return { + goalAuditorModel: current.goalAuditorModel.trim() || undefined, + goalMaxContinuations: + maxContinuationsFromInput(current.goalMaxContinuations) ?? + previous?.goalMaxContinuations ?? + DEFAULT_SESSION_DEFAULTS.goalMaxContinuations, + goalTokenBudget: budget.valid ? budget.value : previous?.goalTokenBudget, + } + }, + isEqual: (a, b) => + a.goalAuditorModel === b.goalAuditorModel && + a.goalMaxContinuations === b.goalMaxContinuations && + a.goalTokenBudget === b.goalTokenBudget, + save: (next, previous) => { + const patch: Partial = {} + if (next.goalAuditorModel !== previous?.goalAuditorModel) patch.goalAuditorModel = next.goalAuditorModel + if (next.goalMaxContinuations !== previous?.goalMaxContinuations) { + patch.goalMaxContinuations = next.goalMaxContinuations + } + if (next.goalTokenBudget !== previous?.goalTokenBudget) patch.goalTokenBudget = next.goalTokenBudget + if (Object.keys(patch).length === 0) return + updateSessionDefaults(patch) + }, + }) - return () => clearTimeout(timer) - }, [commitGoalFields]) + const updateGoalDraft = useCallback( + (patch: Partial) => { + setDraft((current) => ({ ...current, ...patch })) + }, + [setDraft], + ) return (
@@ -135,8 +133,8 @@ export function SessionAutomationSettings() { updateGoalDraft({ goalAuditorModel: value })} placeholder="OpenCode default" allowCustomValue showClear @@ -156,9 +154,9 @@ export function SessionAutomationSettings() { type="number" min={GOAL_MAX_CONTINUATIONS_MIN} max={GOAL_MAX_CONTINUATIONS_MAX} - value={goalMaxContinuations} + value={draft.goalMaxContinuations} className="w-full shrink-0 sm:w-40" - onChange={(event) => setGoalMaxContinuations(event.target.value)} + onChange={(event) => updateGoalDraft({ goalMaxContinuations: event.target.value })} onBlur={commitGoalFields} />
@@ -174,10 +172,10 @@ export function SessionAutomationSettings() { id="goalTokenBudget" type="number" min={1} - value={goalTokenBudget} + value={draft.goalTokenBudget} placeholder="No limit" className="w-full shrink-0 sm:w-40" - onChange={(event) => setGoalTokenBudget(event.target.value)} + onChange={(event) => updateGoalDraft({ goalTokenBudget: event.target.value })} onBlur={commitGoalFields} />
diff --git a/frontend/src/components/settings/WalkthroughSettings.test.tsx b/frontend/src/components/settings/WalkthroughSettings.test.tsx new file mode 100644 index 000000000..f90b38ad9 --- /dev/null +++ b/frontend/src/components/settings/WalkthroughSettings.test.tsx @@ -0,0 +1,154 @@ +import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest' +import { act, render, screen, fireEvent } from '@testing-library/react' +import { WalkthroughSettings } from './WalkthroughSettings' +import { useSettings } from '@/hooks/useSettings' +import type { UserPreferences } from '@/api/types/settings' +import type { Provider } from '@/api/providers' +import { createUseSettingsMock } from '@/test/test-utils' + +vi.mock('@/hooks/useSettings') + +const mocks = vi.hoisted(() => ({ + useProviders: vi.fn(), + useOpenCodeModelState: vi.fn(), + useOpenCodeDefaultModel: vi.fn(), +})) + +vi.mock('@/hooks/useProviders', () => ({ useProviders: mocks.useProviders })) +vi.mock('@/hooks/useModelSelection', () => ({ + useOpenCodeModelState: mocks.useOpenCodeModelState, + useOpenCodeDefaultModel: mocks.useOpenCodeDefaultModel, +})) + +const providers: Provider[] = [ + { + id: 'anthropic', + name: 'Anthropic', + models: [ + { id: 'claude-sonnet-4', key: 'claude-sonnet-4', name: 'Claude Sonnet 4', released: 0, free: false }, + ], + }, +] + +const basePreferences: UserPreferences = { + theme: 'dark', + mode: 'build', + autoScroll: true, + expandDiffs: true, + expandToolCalls: false, + showReasoning: false, + simpleChatMode: false, + keyboardShortcuts: {}, + customCommands: [], +} + +function mockUseSettings(overrides: Partial> = {}) { + vi.mocked(useSettings).mockReturnValue(createUseSettingsMock({ preferences: basePreferences, ...overrides })) +} + +describe('WalkthroughSettings', () => { + beforeAll(() => { + Element.prototype.hasPointerCapture ??= () => false + Element.prototype.setPointerCapture ??= () => {} + Element.prototype.releasePointerCapture ??= () => {} + }) + + beforeEach(() => { + vi.clearAllMocks() + mocks.useProviders.mockReturnValue({ data: { providers, models: [] } }) + mocks.useOpenCodeModelState.mockReturnValue({ data: { favorite: [], recent: [], variant: {} } }) + mocks.useOpenCodeDefaultModel.mockReturnValue({ data: null }) + }) + + it('saves a chosen model after the debounce', () => { + vi.useFakeTimers() + try { + const updateSettings = vi.fn() + mockUseSettings({ updateSettings }) + render() + + fireEvent.focus(screen.getByLabelText('Walkthrough model')) + fireEvent.click(screen.getByRole('option', { name: /Claude Sonnet 4/ })) + act(() => { + vi.advanceTimersByTime(800) + }) + + expect(updateSettings).toHaveBeenCalledTimes(1) + expect(updateSettings).toHaveBeenCalledWith({ walkthroughModel: 'anthropic/claude-sonnet-4' }) + } finally { + vi.useRealTimers() + } + }) + + it('clears the walkthrough model to an empty string', () => { + vi.useFakeTimers() + try { + const updateSettings = vi.fn() + mockUseSettings({ + preferences: { ...basePreferences, walkthroughModel: 'anthropic/claude-sonnet-4' }, + updateSettings, + }) + render() + + fireEvent.click(screen.getByRole('button', { name: 'Clear' })) + act(() => { + vi.advanceTimersByTime(800) + }) + + expect(updateSettings).toHaveBeenCalledTimes(1) + expect(updateSettings).toHaveBeenCalledWith({ walkthroughModel: '' }) + } finally { + vi.useRealTimers() + } + }) + + it('does not save an unchanged value', () => { + vi.useFakeTimers() + try { + const updateSettings = vi.fn() + mockUseSettings({ + preferences: { ...basePreferences, walkthroughModel: 'anthropic/claude-sonnet-4' }, + updateSettings, + }) + render() + + act(() => { + vi.advanceTimersByTime(800) + }) + + expect(updateSettings).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('does not save when no model is stored and the field is empty', () => { + vi.useFakeTimers() + try { + const updateSettings = vi.fn() + mockUseSettings({ updateSettings }) + render() + + act(() => { + vi.advanceTimersByTime(800) + }) + + expect(updateSettings).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('keeps the OpenCode default as a selectable model when the field is unset', () => { + mocks.useOpenCodeDefaultModel.mockReturnValue({ data: 'anthropic/claude-sonnet-4' }) + mockUseSettings() + render() + + const input = screen.getByLabelText('Walkthrough model') + expect(input).toHaveValue('') + + fireEvent.focus(input) + expect(screen.getByRole('option', { name: /Claude Sonnet 4/ })).toBeInTheDocument() + expect(screen.queryByRole('option', { name: /^Default:/ })).not.toBeInTheDocument() + }) +}) diff --git a/frontend/src/components/settings/WalkthroughSettings.tsx b/frontend/src/components/settings/WalkthroughSettings.tsx new file mode 100644 index 000000000..c2dd0ffb7 --- /dev/null +++ b/frontend/src/components/settings/WalkthroughSettings.tsx @@ -0,0 +1,43 @@ +import { useSettings } from '@/hooks/useSettings' +import { useAutosavedSetting } from '@/hooks/useAutosavedSetting' +import { Label } from '@/components/ui/label' +import { ModelCombobox } from '@/components/model/ModelCombobox' + +export function WalkthroughSettings() { + const { preferences, updateSettings } = useSettings() + const storedWalkthroughModel = preferences?.walkthroughModel + + const { draft: walkthroughModel, setDraft: setWalkthroughModel } = useAutosavedSetting({ + stored: storedWalkthroughModel, + toDraft: (stored) => stored ?? '', + toCommitted: (draft) => draft.trim(), + isEqual: (a, b) => a === b, + save: (next) => updateSettings({ walkthroughModel: next }), + }) + + return ( +
+

Change walkthrough

+ +
+
+ +

+ Model that writes change walkthroughs. A small, fast model works well. Leave empty to use the session's model, or the OpenCode default. +

+
+ +
+
+ ) +} diff --git a/frontend/src/hooks/useAutosavedSetting.test.tsx b/frontend/src/hooks/useAutosavedSetting.test.tsx new file mode 100644 index 000000000..a649b3a71 --- /dev/null +++ b/frontend/src/hooks/useAutosavedSetting.test.tsx @@ -0,0 +1,55 @@ +import { renderHook, act } from '@testing-library/react' +import { describe, expect, it, vi } from 'vitest' +import { useAutosavedSetting } from './useAutosavedSetting' + +function renderTrimmedSetting(stored: string | undefined, save: (next: string, previous: string | undefined) => void) { + return renderHook( + ({ value }: { value: string | undefined }) => + useAutosavedSetting({ + stored: value, + toDraft: (draftStored) => draftStored ?? '', + toCommitted: (draft) => draft.trim(), + isEqual: (a, b) => a === b, + save, + }), + { initialProps: { value: stored } }, + ) +} + +describe('useAutosavedSetting', () => { + it('saves the normalized value once after the debounce', () => { + vi.useFakeTimers() + try { + const save = vi.fn() + const { result } = renderTrimmedSetting(undefined, save) + + act(() => result.current.setDraft(' anthropic/claude ')) + act(() => { + vi.advanceTimersByTime(800) + }) + + expect(save).toHaveBeenCalledTimes(1) + expect(save).toHaveBeenCalledWith('anthropic/claude', '') + } finally { + vi.useRealTimers() + } + }) + + it('skips an unchanged value and resyncs from the stored value', () => { + const save = vi.fn() + const { result, rerender } = renderTrimmedSetting('a', save) + + act(() => result.current.commit()) + expect(save).not.toHaveBeenCalled() + + act(() => result.current.setDraft('b')) + act(() => result.current.commit()) + expect(save).toHaveBeenCalledWith('b', 'a') + + rerender({ value: 'c' }) + expect(result.current.draft).toBe('c') + + act(() => result.current.commit()) + expect(save).toHaveBeenCalledTimes(1) + }) +}) diff --git a/frontend/src/hooks/useAutosavedSetting.ts b/frontend/src/hooks/useAutosavedSetting.ts new file mode 100644 index 000000000..222412c8c --- /dev/null +++ b/frontend/src/hooks/useAutosavedSetting.ts @@ -0,0 +1,74 @@ +import { useCallback, useEffect, useRef, useState } from 'react' +import type { Dispatch, SetStateAction } from 'react' +import { useDebouncedValue } from './useDebouncedValue' + +export const AUTOSAVE_DELAY_MS = 800 + +interface AutosavedSettingHandlers { + toDraft: (stored: TStored) => TDraft + toCommitted: (draft: TDraft, previous: TCommitted | undefined) => TCommitted + isEqual: (a: TCommitted, b: TCommitted) => boolean + save: (next: TCommitted, previous: TCommitted | undefined) => void +} + +interface UseAutosavedSettingOptions + extends AutosavedSettingHandlers { + stored: TStored + delayMs?: number +} + +interface UseAutosavedSettingResult { + draft: TDraft + setDraft: Dispatch> + commit: () => void +} + +export function useAutosavedSetting({ + stored, + toDraft, + toCommitted, + isEqual, + save, + delayMs = AUTOSAVE_DELAY_MS, +}: UseAutosavedSettingOptions): UseAutosavedSettingResult { + const handlers = useRef>({ + toDraft, + toCommitted, + isEqual, + save, + }) + + useEffect(() => { + handlers.current = { toDraft, toCommitted, isEqual, save } + }) + + const [draft, setDraft] = useState(() => toDraft(stored)) + const committed = useRef(toCommitted(draft, undefined)) + + useEffect(() => { + const nextDraft = handlers.current.toDraft(stored) + setDraft(nextDraft) + committed.current = handlers.current.toCommitted(nextDraft, committed.current) + }, [stored]) + + const commitDraft = useCallback((value: TDraft) => { + const { toCommitted: normalize, isEqual: equal, save: persist } = handlers.current + const next = normalize(value, committed.current) + if (equal(next, committed.current)) return + const previous = committed.current + committed.current = next + persist(next, previous) + }, []) + + const debouncedDraft = useDebouncedValue(draft, delayMs) + + useEffect(() => { + commitDraft(debouncedDraft) + }, [commitDraft, debouncedDraft]) + + const commit = useCallback(() => { + commitDraft(draft) + }, [commitDraft, draft]) + + return { draft, setDraft, commit } +} diff --git a/frontend/src/hooks/useChangeWalkthrough.test.tsx b/frontend/src/hooks/useChangeWalkthrough.test.tsx index 2439cbaa9..dfeb28f70 100644 --- a/frontend/src/hooks/useChangeWalkthrough.test.tsx +++ b/frontend/src/hooks/useChangeWalkthrough.test.tsx @@ -3,7 +3,12 @@ import { act, renderHook, waitFor } from '@testing-library/react' import type { ReactNode } from 'react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { changeWalkthroughQueryKey, useChangeWalkthrough, useGenerateChangeWalkthrough } from './useChangeWalkthrough' -import type { ChangeWalkthrough, ChangeWalkthroughState } from '@opencode-manager/shared/schemas' +import { DEFAULT_WALKTHROUGH_SOURCE, walkthroughHunksIdentity } from '@opencode-manager/shared/schemas' +import type { + ChangeWalkthrough, + ChangeWalkthroughState, + ChangeWalkthroughStateWire, +} from '@opencode-manager/shared/schemas' const mocks = vi.hoisted(() => ({ getChangeWalkthrough: vi.fn(), @@ -17,7 +22,9 @@ vi.mock('@/api/changeWalkthroughs', () => ({ const walkthroughForA: ChangeWalkthrough = { sessionId: 'ses_A', + source: { kind: 'session' }, diffHash: 'hash-a', + model: null, summary: 'A summary', stops: [], hunks: [], @@ -53,7 +60,7 @@ describe('useGenerateChangeWalkthrough', () => { mocks.generateChangeWalkthrough.mockReturnValue(pending.promise) const { result, rerender } = renderHook( - ({ sessionId }: { sessionId: string }) => useGenerateChangeWalkthrough(sessionId), + ({ sessionId }: { sessionId: string }) => useGenerateChangeWalkthrough(sessionId, DEFAULT_WALKTHROUGH_SOURCE), { initialProps: { sessionId: 'ses_A' }, wrapper: createWrapper(queryClient) }, ) @@ -61,7 +68,7 @@ describe('useGenerateChangeWalkthrough', () => { result.current.mutate({}) }) await waitFor(() => { - expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_A', {}) + expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_A', { source: { kind: 'session' } }) }) rerender({ sessionId: 'ses_B' }) @@ -71,9 +78,11 @@ describe('useGenerateChangeWalkthrough', () => { }) await waitFor(() => { - expect(queryClient.getQueryData(changeWalkthroughQueryKey('ses_A'))).toEqual(state({ generating: true })) + expect(queryClient.getQueryData(changeWalkthroughQueryKey('ses_A', 'session'))).toEqual( + state({ generating: true }), + ) }) - expect(queryClient.getQueryData(changeWalkthroughQueryKey('ses_B'))).toBeUndefined() + expect(queryClient.getQueryData(changeWalkthroughQueryKey('ses_B', 'session'))).toBeUndefined() }) }) @@ -90,7 +99,9 @@ describe('useChangeWalkthrough', () => { .mockResolvedValueOnce(state({ generating: true })) .mockResolvedValueOnce(state({ walkthrough: walkthroughForA })) - const { result } = renderHook(() => useChangeWalkthrough('ses_A', true), { wrapper: createWrapper(queryClient) }) + const { result } = renderHook(() => useChangeWalkthrough('ses_A', true, DEFAULT_WALKTHROUGH_SOURCE), { + wrapper: createWrapper(queryClient), + }) await waitFor(() => expect(result.current.data?.generating).toBe(true)) @@ -108,4 +119,71 @@ describe('useChangeWalkthrough', () => { vi.useRealTimers() } }) + + it('merges cached hunks when the server omits them for a matching identity', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + try { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + const walkthroughWithHunks: ChangeWalkthrough = { + ...walkthroughForA, + hunks: [ + { + id: 'h_1', + file: 'src/a.ts', + status: 'modified', + header: '@@ -1 +1 @@', + text: '@@ -1 +1 @@\n-a\n+b', + truncated: false, + }, + ], + } + const compact: ChangeWalkthroughStateWire = { + ...state({ generating: true }), + walkthrough: { ...walkthroughWithHunks, hunks: undefined }, + } + mocks.getChangeWalkthrough + .mockResolvedValueOnce(state({ walkthrough: walkthroughWithHunks, generating: true })) + .mockResolvedValueOnce(compact) + + const { result } = renderHook(() => useChangeWalkthrough('ses_A', true, DEFAULT_WALKTHROUGH_SOURCE), { + wrapper: createWrapper(queryClient), + }) + + await waitFor(() => expect(result.current.data?.walkthrough?.hunks).toHaveLength(1)) + + await act(async () => { + await vi.advanceTimersByTimeAsync(2_000) + }) + + await waitFor(() => expect(mocks.getChangeWalkthrough).toHaveBeenCalledTimes(2)) + expect(mocks.getChangeWalkthrough).toHaveBeenLastCalledWith( + 'ses_A', + DEFAULT_WALKTHROUGH_SOURCE, + walkthroughHunksIdentity(walkthroughWithHunks), + ) + expect(result.current.data?.walkthrough).toEqual(walkthroughWithHunks) + } finally { + vi.useRealTimers() + } + }) + + it('invalidating the session key refetches every source', async () => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + mocks.getChangeWalkthrough.mockResolvedValue(state()) + + renderHook(() => useChangeWalkthrough('ses_A', true, { kind: 'staged' }), { + wrapper: createWrapper(queryClient), + }) + renderHook(() => useChangeWalkthrough('ses_A', true, { kind: 'unstaged' }), { + wrapper: createWrapper(queryClient), + }) + + await waitFor(() => expect(mocks.getChangeWalkthrough).toHaveBeenCalledTimes(2)) + + await act(async () => { + await queryClient.invalidateQueries({ queryKey: changeWalkthroughQueryKey('ses_A') }) + }) + + await waitFor(() => expect(mocks.getChangeWalkthrough).toHaveBeenCalledTimes(4)) + }) }) diff --git a/frontend/src/hooks/useChangeWalkthrough.ts b/frontend/src/hooks/useChangeWalkthrough.ts index 8438606b9..5d70d9ad6 100644 --- a/frontend/src/hooks/useChangeWalkthrough.ts +++ b/frontend/src/hooks/useChangeWalkthrough.ts @@ -1,34 +1,83 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' import { generateChangeWalkthrough, getChangeWalkthrough } from '@/api/changeWalkthroughs' -import type { GenerateChangeWalkthroughRequest } from '@opencode-manager/shared/schemas' +import { walkthroughHunksIdentity, walkthroughSourceKey } from '@opencode-manager/shared/schemas' +import type { + ChangeWalkthrough, + ChangeWalkthroughState, + ChangeWalkthroughStateWire, + ChangeWalkthroughWire, + GenerateChangeWalkthroughRequest, + WalkthroughSource, +} from '@opencode-manager/shared/schemas' const GENERATING_POLL_INTERVAL_MS = 2_000 -export function changeWalkthroughQueryKey(sessionId: string) { - return ['change-walkthrough', sessionId] as const +export function changeWalkthroughQueryKey(sessionId: string, sourceKey?: string) { + return sourceKey === undefined + ? (['change-walkthrough', sessionId] as const) + : (['change-walkthrough', sessionId, sourceKey] as const) } -export function useChangeWalkthrough(sessionId: string | undefined, enabled: boolean) { +function isCompleteWalkthrough(walkthrough: ChangeWalkthroughWire): walkthrough is ChangeWalkthrough { + return walkthrough.hunks !== undefined +} + +function mergeChangeWalkthroughState( + state: ChangeWalkthroughStateWire, + cached: ChangeWalkthrough | null, +): ChangeWalkthroughState { + const incoming = state.walkthrough + if (!incoming) { + return { ...state, walkthrough: null } + } + if (isCompleteWalkthrough(incoming)) { + return { ...state, walkthrough: incoming } + } + const hunks = + cached && walkthroughHunksIdentity(cached) === walkthroughHunksIdentity(incoming) + ? cached.hunks + : [] + return { ...state, walkthrough: { ...incoming, hunks } } +} + +export function useChangeWalkthrough( + sessionId: string | undefined, + enabled: boolean, + source: WalkthroughSource, +) { + const sourceKey = walkthroughSourceKey(source) + const queryClient = useQueryClient() + const queryKey = changeWalkthroughQueryKey(sessionId ?? '', sourceKey) return useQuery({ - queryKey: changeWalkthroughQueryKey(sessionId ?? ''), - queryFn: () => getChangeWalkthrough(sessionId!), + queryKey, + queryFn: async () => { + const cached = queryClient.getQueryData(queryKey) + const cachedWalkthrough = cached?.walkthrough ?? null + const hunksFor = cachedWalkthrough ? walkthroughHunksIdentity(cachedWalkthrough) : undefined + const state = + hunksFor === undefined + ? await getChangeWalkthrough(sessionId!, source) + : await getChangeWalkthrough(sessionId!, source, hunksFor) + return mergeChangeWalkthroughState(state, cachedWalkthrough) + }, enabled: enabled && !!sessionId, staleTime: 30_000, refetchInterval: (query) => (query.state.data?.generating ? GENERATING_POLL_INTERVAL_MS : false), }) } -export function useGenerateChangeWalkthrough(sessionId: string | undefined) { +export function useGenerateChangeWalkthrough(sessionId: string | undefined, source: WalkthroughSource) { const queryClient = useQueryClient() + const sourceKey = walkthroughSourceKey(source) return useMutation({ mutationFn: async (request: GenerateChangeWalkthroughRequest) => { const targetSessionId = sessionId! - const state = await generateChangeWalkthrough(targetSessionId, request) - return { sessionId: targetSessionId, state } + const state = await generateChangeWalkthrough(targetSessionId, { ...request, source }) + return { sessionId: targetSessionId, sourceKey, state } }, - onSuccess: ({ sessionId: targetSessionId, state }) => { - queryClient.setQueryData(changeWalkthroughQueryKey(targetSessionId), state) + onSuccess: ({ sessionId: targetSessionId, sourceKey: targetSourceKey, state }) => { + queryClient.setQueryData(changeWalkthroughQueryKey(targetSessionId, targetSourceKey), state) }, }) } diff --git a/frontend/src/lib/unified-diff.test.ts b/frontend/src/lib/unified-diff.test.ts index 79c629d4b..2f2503be8 100644 --- a/frontend/src/lib/unified-diff.test.ts +++ b/frontend/src/lib/unified-diff.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { parseDiffLines, splitDiffHunks } from '@opencode-manager/shared/utils'; +import { countDiffLineChanges, parseDiffLines, splitDiffHunks, splitUnifiedDiffByFile } from '@opencode-manager/shared/utils'; const DIFF = [ 'diff --git a/src/app.ts b/src/app.ts', @@ -88,6 +88,31 @@ describe('parseDiffLines', () => { }); }); +describe('countDiffLineChanges', () => { + it('counts additions and removals across hunks', () => { + expect(countDiffLineChanges(DIFF)).toEqual({ added: 3, removed: 2 }); + }); + + it('counts changed lines whose content starts with -- or ++', () => { + const diff = '@@ -1,3 +1,3 @@\n--- old comment\n+++ heading\n const a = 1;'; + + expect(countDiffLineChanges(diff)).toEqual({ added: 1, removed: 1 }); + }); + + it('ignores file headers before a hunk', () => { + const diff = '--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b'; + + expect(countDiffLineChanges(diff)).toEqual({ added: 1, removed: 1 }); + }); + + it('returns zero for a patch with no changed lines', () => { + expect(countDiffLineChanges('diff --git a/x b/x\nold mode 100644\nnew mode 100755')).toEqual({ + added: 0, + removed: 0, + }); + }); +}); + describe('splitDiffHunks', () => { it('returns one entry per hunk with the header line and body', () => { const hunks = splitDiffHunks(DIFF); @@ -117,3 +142,222 @@ describe('splitDiffHunks', () => { expect(splitDiffHunks('')).toEqual([]); }); }); + +describe('splitUnifiedDiffByFile', () => { + it('splits a modified file and counts additions and removals', () => { + const result = splitUnifiedDiffByFile(DIFF); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ file: 'src/app.ts', status: 'modified', additions: 3, deletions: 2 }); + expect(result[0]!.patch.startsWith('diff --git a/src/app.ts b/src/app.ts')).toBe(true); + }); + + it('counts changed lines whose content starts with -- or ++', () => { + const diff = [ + 'diff --git a/src/a.sql b/src/a.sql', + '--- a/src/a.sql', + '+++ b/src/a.sql', + '@@ -1,2 +1,2 @@', + '--- old comment', + '+++ heading', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'src/a.sql', status: 'modified', additions: 1, deletions: 1 }); + }); + + it('reads an added file from the new side', () => { + const diff = [ + 'diff --git a/src/new.ts b/src/new.ts', + 'new file mode 100644', + 'index 0000000..1111111', + '--- /dev/null', + '+++ b/src/new.ts', + '@@ -0,0 +1,2 @@', + '+one', + '+two', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'src/new.ts', status: 'added', additions: 2, deletions: 0 }); + }); + + it('reads a deleted file from the old side', () => { + const diff = [ + 'diff --git a/src/old.ts b/src/old.ts', + 'deleted file mode 100644', + 'index 1111111..0000000', + '--- a/src/old.ts', + '+++ /dev/null', + '@@ -1,2 +0,0 @@', + '-one', + '-two', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'src/old.ts', status: 'deleted', additions: 0, deletions: 2 }); + }); + + it('reads a renamed file with no content changes', () => { + const diff = [ + 'diff --git a/src/old.ts b/src/new.ts', + 'similarity index 100%', + 'rename from src/old.ts', + 'rename to src/new.ts', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'src/new.ts', status: 'modified', additions: 0, deletions: 0 }); + }); + + it('reads a mode-only change path from a mnemonic diff header', () => { + const diff = [ + 'diff --git i/script.sh w/script.sh', + 'old mode 100644', + 'new mode 100755', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'script.sh', status: 'modified', additions: 0, deletions: 0 }); + }); + + it('reads a mode-only change path from a prefix-less diff header', () => { + const diff = [ + 'diff --git script.sh script.sh', + 'old mode 100644', + 'new mode 100755', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'script.sh', status: 'modified' }); + }); + + it('keeps a combined merge-conflict block instead of dropping it', () => { + const diff = [ + 'diff --cc f.txt', + 'index d791e9b,00dbdcf..0000000', + '--- a/f.txt', + '+++ b/f.txt', + '@@@ -1,3 -1,3 +1,7 @@@', + ' line1', + '++<<<<<<< HEAD', + ' +MAIN', + '++=======', + '+ FEATURE', + '++>>>>>>> feat', + ' line3', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'f.txt', status: 'modified' }); + expect(file!.patch.startsWith('diff --cc f.txt')).toBe(true); + }); + + it('keeps a binary block with no hunks', () => { + const diff = [ + 'diff --git a/assets/logo.png b/assets/logo.png', + 'index 1111111..2222222 100644', + 'Binary files a/assets/logo.png and b/assets/logo.png differ', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'assets/logo.png', status: 'modified' }); + expect(file!.patch).not.toContain('@@'); + }); + + it('splits multiple files', () => { + const diff = [ + DIFF, + '', + 'diff --git a/src/other.ts b/src/other.ts', + '--- a/src/other.ts', + '+++ b/src/other.ts', + '@@ -1 +1 @@', + '-a', + '+b', + ].join('\n'); + + expect(splitUnifiedDiffByFile(diff).map((entry) => entry.file)).toEqual(['src/app.ts', 'src/other.ts']); + }); + + it('parses a path containing a space with the trailing tab git appends', () => { + const diff = [ + 'diff --git a/my file.txt b/my file.txt', + 'index 5626abf..f719efd 100644', + '--- a/my file.txt\t', + '+++ b/my file.txt\t', + '@@ -1 +1 @@', + '-one', + '+two', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'my file.txt', status: 'modified' }); + }); + + it('decodes a C-quoted octal UTF-8 path', () => { + const diff = [ + 'diff --git "a/\\303\\251.txt" "b/\\303\\251.txt"', + 'index 587be6b..ccc9bd6 100644', + '--- "a/\\303\\251.txt"', + '+++ "b/\\303\\251.txt"', + '@@ -1 +1 @@', + '-x', + '+xx', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'é.txt', status: 'modified' }); + }); + + it('decodes a quoted rename target', () => { + const diff = [ + 'diff --git "a/\\303\\251.txt" "b/\\303\\251 renamed.txt"', + 'similarity index 100%', + 'rename from "\\303\\251.txt"', + 'rename to "\\303\\251 renamed.txt"', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'é renamed.txt', status: 'modified' }); + }); + + it('decodes a quoted path in the header fallback', () => { + const diff = [ + 'diff --git "a/\\303\\251.bin" "b/\\303\\251.bin"', + 'index 1111111..2222222 100644', + 'Binary files "a/\\303\\251.bin" and "b/\\303\\251.bin" differ', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'é.bin', status: 'modified' }); + }); + + it('decodes a quoted header path when only the new side is quoted', () => { + const diff = [ + 'diff --git a/plain.bin "b/\\303\\251.bin"', + 'index 1111111..2222222 100644', + 'Binary files a/plain.bin and "b/\\303\\251.bin" differ', + ].join('\n'); + + const [file] = splitUnifiedDiffByFile(diff); + + expect(file).toMatchObject({ file: 'é.bin', status: 'modified' }); + }); + + it('returns an empty list for an empty diff', () => { + expect(splitUnifiedDiffByFile('')).toEqual([]); + }); +}); diff --git a/shared/src/schemas/change-walkthroughs.ts b/shared/src/schemas/change-walkthroughs.ts index 6aca878ca..aec74059d 100644 --- a/shared/src/schemas/change-walkthroughs.ts +++ b/shared/src/schemas/change-walkthroughs.ts @@ -1,9 +1,107 @@ import { z } from "zod"; +import { gitRefSchema } from './git' export const WALKTHROUGH_DIFF_MAX_CHARS = 60000; export const WALKTHROUGH_HUNK_MAX_CHARS = 8000; +export const MECHANICAL_TEXT_BUDGET = 20000; export const WALKTHROUGH_MAX_STOPS = 20; export const WALKTHROUGH_TEXT_MAX_CHARS = 2000; +export const WALKTHROUGH_OUTLINE_PREVIEW_LINES = 6; +export const WALKTHROUGH_OUTLINE_LINE_MAX_CHARS = 160; + +export const WalkthroughSourceSchema = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('session') }), + z.object({ kind: z.literal('uncommitted') }), + z.object({ kind: z.literal('staged') }), + z.object({ kind: z.literal('unstaged') }), + z.object({ kind: z.literal('branch'), base: gitRefSchema.optional() }), + z.object({ kind: z.literal('pullRequest'), number: z.number().int().positive(), base: gitRefSchema.optional() }), +]) + +export type WalkthroughSource = z.infer + +export const DEFAULT_WALKTHROUGH_SOURCE: WalkthroughSource = { kind: 'session' } + +export function walkthroughSourceKey(source: WalkthroughSource): string { + switch (source.kind) { + case 'session': + case 'uncommitted': + case 'staged': + case 'unstaged': + return source.kind + case 'branch': + return `branch:${source.base ?? ''}` + case 'pullRequest': + return `pr:${source.number}:${source.base ?? ''}` + } +} + +export function describeWalkthroughSource(source: WalkthroughSource): string { + switch (source.kind) { + case 'session': + return 'the changes in this session' + case 'uncommitted': + return 'uncommitted changes' + case 'staged': + return 'staged changes' + case 'unstaged': + return 'unstaged changes' + case 'branch': + return source.base ? `branch against ${source.base}` : 'branch against the default branch' + case 'pullRequest': + return `pull request #${source.number}` + } +} + +export function toWalkthroughSource(candidate: { + kind: WalkthroughSource['kind'] + base?: string + number?: number +}): WalkthroughSource | null { + const base = candidate.base?.trim() + switch (candidate.kind) { + case 'session': + case 'uncommitted': + case 'staged': + case 'unstaged': + return { kind: candidate.kind } + case 'branch': + return parseWalkthroughSourceCandidate(base ? { kind: 'branch', base } : { kind: 'branch' }) + case 'pullRequest': + return parseWalkthroughSourceCandidate( + base + ? { kind: 'pullRequest', number: candidate.number, base } + : { kind: 'pullRequest', number: candidate.number }, + ) + } +} + +export function parseWalkthroughSourceKey(key: string): WalkthroughSource | null { + if (key === 'session' || key === 'uncommitted' || key === 'staged' || key === 'unstaged') { + return { kind: key } + } + if (key.startsWith('branch:')) { + return toWalkthroughSource({ kind: 'branch', base: key.slice('branch:'.length) }) + } + if (key.startsWith('pr:')) { + const rest = key.slice('pr:'.length) + const separator = rest.indexOf(':') + if (separator < 0) { + return null + } + return toWalkthroughSource({ + kind: 'pullRequest', + number: Number(rest.slice(0, separator)), + base: rest.slice(separator + 1), + }) + } + return null +} + +function parseWalkthroughSourceCandidate(candidate: unknown): WalkthroughSource | null { + const parsed = WalkthroughSourceSchema.safeParse(candidate) + return parsed.success ? parsed.data : null +} export const WalkthroughHunkSchema = z.object({ id: z.string(), @@ -12,28 +110,35 @@ export const WalkthroughHunkSchema = z.object({ header: z.string(), text: z.string(), truncated: z.boolean(), + additions: z.number().int().nonnegative().optional(), + deletions: z.number().int().nonnegative().optional(), }); export type WalkthroughHunk = z.infer; export const WalkthroughStopSchema = z.object({ + id: z.string(), title: z.string(), explanation: z.string(), hunkIds: z.array(z.string()), + status: z.enum(["pending", "ready", "failed"]), + explanationKey: z.string().nullable(), }); export type WalkthroughStop = z.infer; export const WalkthroughOmittedFileSchema = z.object({ file: z.string(), - reason: z.enum(["binary", "budget"]), + reason: z.enum(["binary", "budget", "renamed", "modeChange"]), }); export type WalkthroughOmittedFile = z.infer; export const ChangeWalkthroughSchema = z.object({ sessionId: z.string(), + source: WalkthroughSourceSchema, diffHash: z.string(), + model: z.string().nullable(), summary: z.string(), stops: z.array(WalkthroughStopSchema), hunks: z.array(WalkthroughHunkSchema), @@ -61,8 +166,27 @@ export const ChangeWalkthroughStateSchema = z.object({ export type ChangeWalkthroughState = z.infer; +export const ChangeWalkthroughWireSchema = ChangeWalkthroughSchema.extend({ + hunks: z.array(WalkthroughHunkSchema).optional(), +}); + +export type ChangeWalkthroughWire = z.infer; + +export const ChangeWalkthroughStateWireSchema = ChangeWalkthroughStateSchema.extend({ + walkthrough: ChangeWalkthroughWireSchema.nullable(), +}); + +export type ChangeWalkthroughStateWire = z.infer; + +export function walkthroughHunksIdentity( + walkthrough: Pick, +): string { + return `${walkthrough.diffHash}:${walkthrough.createdAt}` +} + export const GenerateChangeWalkthroughRequestSchema = z.object({ regenerate: z.boolean().optional(), + source: WalkthroughSourceSchema.optional(), }); export type GenerateChangeWalkthroughRequest = z.infer; diff --git a/shared/src/schemas/git.ts b/shared/src/schemas/git.ts index a4e8edc7b..979091685 100644 --- a/shared/src/schemas/git.ts +++ b/shared/src/schemas/git.ts @@ -1,12 +1,22 @@ import { z } from 'zod' import { getBranchNameError } from '../utils/repo' -const branchNameSchema = z.string().trim().superRefine((value, ctx) => { +function addBranchNameIssue(value: string, ctx: z.RefinementCtx) { const error = getBranchNameError(value) if (error) { ctx.addIssue({ code: 'custom', message: error }) } -}) +} + +const branchNameSchema = z.string().trim().superRefine(addBranchNameIssue) + +export const gitRefSchema = z + .string() + .trim() + .min(1) + .max(200) + .regex(/^\S+$/) + .superRefine(addBranchNameIssue) export const RenameBranchRequestSchema = z.object({ from: branchNameSchema, diff --git a/shared/src/schemas/internal-assistant.ts b/shared/src/schemas/internal-assistant.ts index 86758fcdd..9c00f595c 100644 --- a/shared/src/schemas/internal-assistant.ts +++ b/shared/src/schemas/internal-assistant.ts @@ -51,6 +51,7 @@ export const AssistantSettingsPatchSchema = UserPreferencesSchema.pick({ colorTheme: true, mode: true, defaultModel: true, + walkthroughModel: true, defaultAgent: true, autoScroll: true, expandDiffs: true, diff --git a/shared/src/schemas/settings.ts b/shared/src/schemas/settings.ts index fa0ed2dfb..d2921e370 100644 --- a/shared/src/schemas/settings.ts +++ b/shared/src/schemas/settings.ts @@ -177,6 +177,7 @@ export const UserPreferencesSchema = z.object({ colorTheme: ColorThemeIdSchema.optional(), mode: z.enum(["plan", "build"]), defaultModel: z.string().optional(), + walkthroughModel: z.string().optional(), defaultAgent: z.string().optional(), autoScroll: z.boolean(), expandDiffs: z.boolean(), diff --git a/shared/src/utils/unified-diff.ts b/shared/src/utils/unified-diff.ts index edf6fdaf2..1c1e9ade6 100644 --- a/shared/src/utils/unified-diff.ts +++ b/shared/src/utils/unified-diff.ts @@ -1,3 +1,5 @@ +import type { FileDiffInfo } from '../opencode' + export interface DiffLine { type: "add" | "remove" | "context" | "header" | "hunk"; content: string; @@ -90,6 +92,19 @@ export function parseDiffLines(diff: string): DiffLine[] { return result; } +export function countDiffLineChanges(text: string): { added: number; removed: number } { + let added = 0; + let removed = 0; + for (const line of parseDiffLines(text)) { + if (line.type === "add") { + added += 1; + } else if (line.type === "remove") { + removed += 1; + } + } + return { added, removed }; +} + export function splitDiffHunks(patch: string): DiffHunk[] { const hunks: DiffHunk[] = []; let current: string[] | null = null; @@ -115,3 +130,159 @@ export function splitDiffHunks(patch: string): DiffHunk[] { function toHunk(lines: string[]): DiffHunk { return { header: lines[0]!, text: lines.join("\n") }; } + +export function splitUnifiedDiffByFile(diff: string): FileDiffInfo[] { + const blocks: string[][] = []; + + for (const line of diff.split("\n")) { + if (line.startsWith("diff --git ") || line.startsWith("diff --cc ") || line.startsWith("diff --combined ")) { + blocks.push([line]); + } else if (blocks.length > 0) { + blocks[blocks.length - 1]!.push(line); + } + } + + return blocks.map((lines) => toUnifiedFileDiff(lines)); +} + +function toUnifiedFileDiff(lines: string[]): FileDiffInfo { + const header = lines[0]!; + const newPath = lines.find((line) => line.startsWith("+++ ")); + const oldPath = lines.find((line) => line.startsWith("--- ")); + const renameTo = lines.find((line) => line.startsWith("rename to ")); + let file = ""; + let status: FileDiffInfo["status"] = "modified"; + + if (newPath && newPath.slice(4) !== "/dev/null") { + file = parseDiffPathToken(newPath.slice(4), "b/"); + } else if (oldPath && oldPath.slice(4) !== "/dev/null") { + file = parseDiffPathToken(oldPath.slice(4), "a/"); + } else if (renameTo) { + file = parseDiffPathToken(renameTo.slice("rename to ".length), ""); + } else { + file = newPathFromDiffHeader(header); + } + + for (const line of lines) { + if (line.startsWith("new file mode")) { + status = "added"; + } else if (line.startsWith("deleted file mode")) { + status = "deleted"; + } + } + + const patch = lines.join("\n"); + const { added: additions, removed: deletions } = countDiffLineChanges(patch); + + return { file, status, patch, additions, deletions }; +} + +function stripDiffPathPrefix(path: string, prefix: string): string { + return path.startsWith(prefix) ? path.slice(prefix.length) : path; +} + +const GIT_C_ESCAPES: Record = { + a: 0x07, + b: 0x08, + f: 0x0c, + n: 0x0a, + r: 0x0d, + t: 0x09, + v: 0x0b, + "\\": 0x5c, + '"': 0x22, +}; + +function parseDiffPathToken(raw: string, prefix: string): string { + const token = raw.startsWith('"') ? decodeGitQuotedPath(raw) : raw.split("\t")[0]!; + return stripDiffPathPrefix(token, prefix); +} + +function decodeGitQuotedPath(raw: string): string { + const bytes: number[] = []; + const encoder = new TextEncoder(); + let index = 1; + + while (index < raw.length) { + const char = raw[index]!; + if (char === '"') { + break; + } + if (char === "\\") { + const octal = /^[0-7]{1,3}/.exec(raw.slice(index + 1)); + if (octal) { + bytes.push(parseInt(octal[0]!, 8) & 0xff); + index += 1 + octal[0]!.length; + continue; + } + const next = raw[index + 1]; + if (next === undefined) { + break; + } + const escaped = GIT_C_ESCAPES[next]; + bytes.push(...(escaped === undefined ? encoder.encode(next) : [escaped])); + index += 2; + continue; + } + bytes.push(...encoder.encode(char)); + index += 1; + } + + return new TextDecoder().decode(new Uint8Array(bytes)); +} + +function newPathFromDiffHeader(header: string): string { + const match = /^diff --(?:git|cc|combined) (.*)$/.exec(header); + const rest = match ? match[1]! : ""; + + if (rest.startsWith('"')) { + const end = findClosingQuoteFrom(rest, 0); + if (end >= 0) { + let index = end + 1; + while (rest[index] === " ") { + index += 1; + } + return stripKnownDiffPrefix(parseDiffPathToken(rest.slice(index), "")); + } + } + + const quoted = rest.indexOf(' "'); + if (quoted >= 0) { + return stripKnownDiffPrefix(parseDiffPathToken(rest.slice(quoted + 1), "")); + } + + for (const prefix of GIT_DIFF_DEST_PREFIXES) { + const index = rest.lastIndexOf(prefix); + if (index >= 0) { + return parseDiffPathToken(rest.slice(index + 1), prefix.trim()); + } + } + + const lastSpace = rest.lastIndexOf(" "); + return lastSpace >= 0 ? rest.slice(lastSpace + 1) : rest; +} + +const GIT_DIFF_DEST_PREFIXES = [" b/", " w/", " 2/", " 3/", " i/", " c/", " o/", " 1/", " a/"]; +const GIT_DIFF_PREFIXES = ["a/", "b/", "i/", "w/", "c/", "o/", "1/", "2/", "3/"]; + +function stripKnownDiffPrefix(path: string): string { + for (const prefix of GIT_DIFF_PREFIXES) { + if (path.startsWith(prefix)) { + return path.slice(prefix.length); + } + } + return path; +} + +function findClosingQuoteFrom(raw: string, start: number): number { + for (let index = start + 1; index < raw.length; index += 1) { + if (raw[index] === "\\") { + index += 1; + continue; + } + if (raw[index] === '"') { + return index; + } + } + return -1; +}