diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fd249b..3f9f459 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Fixed + +- Fixed `revpack status` next-step guidance when the local checkout is ahead of the latest PR/MR head. +- Fixed `revpack publish review` leaving `review.md` populated after publishing, which could republish the same review note during later incremental reviews. +- Fixed `revpack publish all` updating the PR/MR description summary again when `revpack status` already reported the summary as published. +- Fixed debug error logging repeating the user-facing error message before the stack frames. + ## [0.4.0] - 2026-06-07 ### Added diff --git a/src/cli/commands/prepare.ts b/src/cli/commands/prepare.ts index f220f57..bb7da43 100644 --- a/src/cli/commands/prepare.ts +++ b/src/cli/commands/prepare.ts @@ -59,51 +59,44 @@ export function registerPrepareCommand(program: Command): void { console.log(chalk.green(`✓ Bundle ${action}`)); console.log(''); console.log(` ${chalk.bold(targetDisplayId)}: ${target.title}`); - console.log(` ${chalk.dim('State:')} ${stateColor(target.state)}`); - console.log(` ${chalk.dim('Author:')} ${isLocal ? target.author : `@${target.author}`}`); - console.log(` ${chalk.dim('Branch:')} ${target.sourceBranch} → ${target.targetBranch}`); - console.log(` ${chalk.dim(`${targetLabel} updated:`)} ${formatDate(target.updatedAt)}`); - console.log(` ${chalk.dim('Threads:')} ${bundle.threads.length} unresolved`); - console.log(` ${chalk.dim('Files:')} ${bundle.diffs.length} changed`); + console.log(` ${chalk.dim('State:')} ${stateColor(target.state)}`); + console.log(` ${chalk.dim('Author:')} ${isLocal ? target.author : `@${target.author}`}`); + console.log(` ${chalk.dim('Branch:')} ${target.sourceBranch} → ${target.targetBranch}`); + console.log(` ${chalk.dim(`Updated:`)} ${formatDate(target.updatedAt)}`); + console.log(` ${chalk.dim('Files:')} ${bundle.diffs.length} changed`); + console.log(` ${chalk.dim('Threads:')} ${bundle.threads.length} unresolved`); console.log(''); // Prepare summary — changes if (result.hasCheckpoint) { - console.log(` ${chalk.dim('Changes since last recorded review state:')}`); - console.log(` ${chalk.dim('Target code:')} ${result.targetCodeChanged ? 'yes' : 'no'}`); + console.log(` ${chalk.dim('Changes since last checkpoint:')}`); + console.log(` ${chalk.dim('Code:')} ${result.targetCodeChanged ? 'yes' : 'no'}`); console.log( - ` ${chalk.dim('Threads/replies:')} ${result.threadsChanged != null ? (result.threadsChanged ? 'yes' : 'no') : 'unknown'}`, + ` ${chalk.dim('Threads:')} ${result.threadsChanged != null ? (result.threadsChanged ? 'yes' : 'no') : 'unknown'}`, ); console.log( - ` ${chalk.dim('Description:')} ${result.descriptionChanged != null ? (result.descriptionChanged ? 'yes' : 'no') : 'unknown'}`, + ` ${chalk.dim('Description:')} ${result.descriptionChanged != null ? (result.descriptionChanged ? 'yes' : 'no') : 'unknown'}`, ); if (result.prunedReplies > 0) { console.log(` ${chalk.dim('Stale replies pruned:')} ${result.prunedReplies}`); } - if (result.publishedActionCount > 0) { - console.log(` ${chalk.dim('Previously published:')} ${result.publishedActionCount} previous`); - } console.log(''); // Focus guidance if (result.targetCodeChanged) { - console.log(` ${chalk.dim('Focus:')} updated diff and unresolved thread updates`); + console.log(` ${chalk.dim('Focus:')} updated diff and unresolved thread updates`); } else if (result.threadsChanged) { - console.log(` ${chalk.dim('Focus:')} updated threads/replies and pending outputs`); + console.log(` ${chalk.dim('Focus:')} updated threads/replies and pending outputs`); } else { - console.log(` ${chalk.dim('Focus:')} no remote changes since the last recorded review state`); + console.log(` ${chalk.dim('Focus:')} no remote changes since the last recorded review state`); } - console.log(''); } else if (mode !== 'fresh') { - console.log(` ${chalk.dim('Review mode:')} fresh review — no recorded review state found`); + console.log(` ${chalk.dim('Focus:')} fresh review — no recorded review state found`); console.log(''); } - - // Key paths - const bundleDir = bundle.bundlePath; - console.log(` ${chalk.dim('Bundle:')} ${bundleDir}`); - console.log(` ${chalk.dim('Context:')} ${result.contextPath}`); + // Context path + console.log(` ${chalk.dim('Context:')} ${result.contextPath}`); console.log(''); // Warnings @@ -120,8 +113,8 @@ export function registerPrepareCommand(program: Command): void { console.log(formatGuidanceLine(' Ask your agent to read .revpack/CONTEXT.md')); console.log(formatGuidanceLine(' Or run `/revpack-review` if installed')); console.log(''); - console.log(formatGuidanceLine('After new commits or review comments:')); - console.log(formatGuidanceLine(' revpack prepare')); + console.log(formatGuidanceLine('Later:')); + console.log(formatGuidanceLine(' After new commits or review comments, run `revpack prepare` again')); } catch (err) { handleError(err); } diff --git a/src/cli/commands/publish.test.ts b/src/cli/commands/publish.test.ts index 4941e46..37c572a 100644 --- a/src/cli/commands/publish.test.ts +++ b/src/cli/commands/publish.test.ts @@ -4,6 +4,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { __testing } from './publish.js'; import { createOrchestrator, getRepoFromGit } from '../helpers.js'; +import { computeContentHash } from '../../workspace/thread-digest.js'; vi.mock('../helpers.js', () => ({ createOrchestrator: vi.fn(), @@ -29,6 +30,62 @@ describe('publish command internals', () => { await fs.rm(tmpDir, { recursive: true, force: true }); }); + async function writeBundleState(provider = 'gitlab', options?: { summaryHash?: string }): Promise { + await fs.mkdir(path.join(tmpDir, '.revpack'), { recursive: true }); + await fs.writeFile( + path.join(tmpDir, '.revpack', 'bundle.json'), + JSON.stringify( + { + target: { provider, diffRefs: { headSha: 'head-sha' } }, + outputs: { + review: { path: '.revpack/outputs/review.md' }, + summary: { + path: '.revpack/outputs/summary.md', + ...(options?.summaryHash ? { lastPublishedHash: options.summaryHash } : {}), + }, + }, + publishedActions: [], + }, + null, + 2, + ), + 'utf-8', + ); + } + + async function writeValidFindingBundle(): Promise { + await fs.mkdir(path.join(tmpDir, '.revpack', 'diffs'), { recursive: true }); + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await fs.writeFile( + path.join(tmpDir, '.revpack', 'diffs', 'latest.patch'), + [ + 'diff --git a/src/app.ts b/src/app.ts', + 'index 1111111..2222222 100644', + '--- a/src/app.ts', + '+++ b/src/app.ts', + '@@ -1 +1,2 @@', + ' const value = read();', + '+audit(value);', + '', + ].join('\n'), + 'utf-8', + ); + await fs.writeFile( + path.join(tmpDir, '.revpack', 'outputs', 'new-findings.json'), + JSON.stringify([ + { + oldPath: 'src/app.ts', + newPath: 'src/app.ts', + newLine: 2, + body: 'Audit call can throw unexpectedly.', + severity: 'medium', + category: 'correctness', + }, + ]), + 'utf-8', + ); + } + it('matches T-NNN reply refs case-insensitively', () => { const entries = [{ threadId: 'T-001', body: 'reply' }]; @@ -106,6 +163,98 @@ describe('publish command internals', () => { expect(createOrchestrator).not.toHaveBeenCalled(); }); + it('clears the default review note after publishing it', async () => { + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await writeBundleState(); + const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); + await fs.writeFile(reviewPath, 'Review body', 'utf-8'); + + const orchestrator = { + publishReview: vi.fn().mockResolvedValue({ created: true, noteId: 'note-1' }), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishReviewCmd({})).resolves.toBe(1); + + await expect(fs.readFile(reviewPath, 'utf-8')).resolves.toBe(''); + const bundleState = JSON.parse(await fs.readFile(path.join(tmpDir, '.revpack', 'bundle.json'), 'utf-8')); + expect(bundleState.outputs.review.lastPublishedHash).toBeUndefined(); + expect(bundleState.outputs.review.providerNoteId).toBeUndefined(); + }); + + it('clears the default review note even when bundle state is unavailable', async () => { + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); + await fs.writeFile(reviewPath, 'Review body', 'utf-8'); + + const orchestrator = { + publishReview: vi.fn().mockResolvedValue({ created: true, noteId: 'note-1' }), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishReviewCmd({})).resolves.toBe(1); + + await expect(fs.readFile(reviewPath, 'utf-8')).resolves.toBe(''); + }); + + it('keeps the default review note when no review note is created', async () => { + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await writeBundleState(); + const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); + await fs.writeFile(reviewPath, 'Review body', 'utf-8'); + + const orchestrator = { + publishReview: vi.fn().mockResolvedValue({ created: false }), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishReviewCmd({})).resolves.toBe(0); + + await expect(fs.readFile(reviewPath, 'utf-8')).resolves.toBe('Review body'); + const bundleState = JSON.parse(await fs.readFile(path.join(tmpDir, '.revpack', 'bundle.json'), 'utf-8')); + expect(bundleState.outputs.review.lastPublishedHash).toBeUndefined(); + }); + + it('does not clear the default review note when publishing from a custom file', async () => { + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await writeBundleState(); + const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); + const customPath = path.join(tmpDir, 'custom-review.md'); + await fs.writeFile(reviewPath, 'Pending default review', 'utf-8'); + await fs.writeFile(customPath, 'Custom review body', 'utf-8'); + + const orchestrator = { + publishReview: vi.fn().mockResolvedValue({ created: true, noteId: 'note-1' }), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishReviewCmd({ from: customPath })).resolves.toBe(1); + + await expect(fs.readFile(reviewPath, 'utf-8')).resolves.toBe('Pending default review'); + await expect(fs.readFile(customPath, 'utf-8')).resolves.toBe('Custom review body'); + }); + + it('clears the default review note after including it in a GitHub review batch', async () => { + await writeBundleState('github'); + await writeValidFindingBundle(); + const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); + await fs.writeFile(reviewPath, 'Batch review body', 'utf-8'); + + const orchestrator = { + publishReviewBatch: vi.fn().mockResolvedValue({ created: true }), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishFindingsAndReviewBatch('Batch review body')).resolves.toBe(1); + + await expect(fs.readFile(reviewPath, 'utf-8')).resolves.toBe(''); + await expect(fs.readFile(path.join(tmpDir, '.revpack', 'outputs', 'new-findings.json'), 'utf-8')).resolves.toBe( + '[]', + ); + const bundleState = JSON.parse(await fs.readFile(path.join(tmpDir, '.revpack', 'bundle.json'), 'utf-8')); + expect(bundleState.outputs.review.lastPublishedHash).toBeUndefined(); + }); + it('uses summary.md as the default description source', async () => { await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); await fs.writeFile(path.join(tmpDir, '.revpack', 'outputs', 'summary.md'), 'Generated summary', 'utf-8'); @@ -126,6 +275,59 @@ describe('publish command internals', () => { ); }); + it('skips the default summary when it is already published', async () => { + const summary = 'Generated summary'; + await writeBundleState('gitlab', { summaryHash: computeContentHash(summary) }); + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await fs.writeFile(path.join(tmpDir, '.revpack', 'outputs', 'summary.md'), summary, 'utf-8'); + + await expect(__testing.publishDescription({})).resolves.toBe(0); + + expect(createOrchestrator).not.toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('summary already published')); + }); + + it('replaces the description from the default summary even when it is already published', async () => { + const summary = 'Generated summary'; + await writeBundleState('gitlab', { summaryHash: computeContentHash(summary) }); + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await fs.writeFile(path.join(tmpDir, '.revpack', 'outputs', 'summary.md'), summary, 'utf-8'); + + const orchestrator = { + open: vi.fn(), + updateDescription: vi.fn().mockResolvedValue(undefined), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishDescription({ replace: true })).resolves.toBe(1); + + expect(orchestrator.open).not.toHaveBeenCalled(); + expect(orchestrator.updateDescription).toHaveBeenCalledWith(undefined, summary, 'group/project'); + }); + + it('publishes a custom description file even when the default summary is already published', async () => { + const summary = 'Generated summary'; + await writeBundleState('gitlab', { summaryHash: computeContentHash(summary) }); + await fs.mkdir(path.join(tmpDir, '.revpack', 'outputs'), { recursive: true }); + await fs.writeFile(path.join(tmpDir, '.revpack', 'outputs', 'summary.md'), summary, 'utf-8'); + const customPath = path.join(tmpDir, 'custom-summary.md'); + await fs.writeFile(customPath, 'Custom summary', 'utf-8'); + + const orchestrator = { + open: vi.fn().mockResolvedValue({ description: 'Existing description' }), + updateDescription: vi.fn().mockResolvedValue(undefined), + }; + vi.mocked(createOrchestrator).mockResolvedValue(orchestrator as never); + + await expect(__testing.publishDescription({ from: customPath })).resolves.toBe(1); + + expect(orchestrator.updateDescription).toHaveBeenCalledWith( + undefined, + expect.stringContaining('Custom summary'), + 'group/project', + ); + }); + it('auto-refreshes after publish without mutating unrelated pending outputs', async () => { const orchestrator = { prepare: vi.fn().mockResolvedValue(undefined), diff --git a/src/cli/commands/publish.ts b/src/cli/commands/publish.ts index 0b855ca..2196e38 100644 --- a/src/cli/commands/publish.ts +++ b/src/cli/commands/publish.ts @@ -260,6 +260,12 @@ async function trackFindingActions(ws: WorkspaceManager, findings: NewFinding[], } } +async function clearDefaultReviewOutput(clearDefaultOutput = true): Promise { + if (clearDefaultOutput) { + await fs.writeFile(workspacePath(DEFAULT_REVIEW_FILE), '', 'utf-8'); + } +} + async function publishFindings(opts: { from?: string; dryRun?: boolean; noRefresh?: boolean }): Promise { const filePath = opts.from ?? DEFAULT_FINDINGS_FILE; const { findings, resolvedPath } = await loadAndValidateFindings(filePath); @@ -307,6 +313,7 @@ async function publishFindings(opts: { from?: string; dryRun?: boolean; noRefres async function publishDescription(opts: { from?: string; replace?: boolean; repo?: string }): Promise { let content: string; let usedSummary = false; + let ws: WorkspaceManager | undefined; if (opts.from) { content = await fs.readFile(workspacePath(opts.from), 'utf-8'); } else { @@ -319,6 +326,15 @@ async function publishDescription(opts: { from?: string; replace?: boolean; repo } requirePublishableContent(content, usedSummary ? DEFAULT_SUMMARY_FILE : (opts.from ?? 'description content')); + if (usedSummary && !opts.replace) { + ws = new WorkspaceManager(process.cwd()); + const summaryState = await ws.getOutputState('summary'); + if (summaryState === 'published') { + console.log(chalk.dim(' (summary already published)')); + return 0; + } + } + const orchestrator = await createOrchestrator(); const defaultRepo = opts.repo ?? (await getRepoFromGit()); @@ -335,7 +351,7 @@ async function publishDescription(opts: { from?: string; replace?: boolean; repo // Store publish hash for summary tracking if (usedSummary) { - const ws = new WorkspaceManager(process.cwd()); + ws ??= new WorkspaceManager(process.cwd()); const bundleState = await ws.loadBundleState(); if (bundleState) { const summaryContent = await fs.readFile(workspacePath(DEFAULT_SUMMARY_FILE), 'utf-8'); @@ -349,7 +365,7 @@ async function publishDescription(opts: { from?: string; replace?: boolean; repo /** * GitHub-specific: load, validate, and submit findings + review.md as a single PR review batch. - * Updates the stored review state. Returns the number of findings published. + * Clears successfully published outputs. Returns the number of findings published. */ async function publishFindingsAndReviewBatch(reviewContent: string): Promise { const { findings, resolvedPath } = await loadAndValidateFindings(DEFAULT_FINDINGS_FILE); @@ -372,11 +388,7 @@ async function publishFindingsAndReviewBatch(reviewContent: string): Promise { // Checkpoint guidance is intentionally omitted when exactly one content output is ready. @@ -97,6 +98,32 @@ describe('buildStatusNextLines', () => { ).toEqual(['Next:', ' revpack publish checkpoint']); }); + it('tells the user to push and prepare when the local checkout is ahead of the latest head', () => { + expect( + buildStatusNextLines({ + repliesReady: false, + findingsReady: false, + summaryReady: false, + reviewReady: false, + checkpointDue: true, + checkoutRelation: 'ahead', + }), + ).toEqual(['Next:', ' Push local commits, then run:', ' revpack prepare']); + }); + + it('keeps checkpoint guidance when the local checkout matches the latest head', () => { + expect( + buildStatusNextLines({ + repliesReady: false, + findingsReady: false, + summaryReady: false, + reviewReady: false, + checkpointDue: true, + checkoutRelation: 'current', + }), + ).toEqual(['Next:', ' revpack publish checkpoint']); + }); + it('shows no pending publish action when nothing is ready or due', () => { expect( buildStatusNextLines({ @@ -153,3 +180,46 @@ describe('buildPendingOlderBundleLines', () => { ).toEqual([]); }); }); + +describe('compareCheckoutToTargetHead', () => { + function gitDouble(options: { + hasCommit: (sha: string) => boolean | Promise; + isAncestor?: (ancestorSha: string, descendantRef?: string) => boolean | Promise; + }) { + return { + hasCommit: options.hasCommit, + isAncestor: options.isAncestor ?? (() => false), + } as unknown as GitHelper; + } + + it('reports unknown when the comparison target commit is not available locally', async () => { + const git = gitDouble({ + hasCommit: (sha) => sha === 'current-head', + isAncestor: () => { + throw new Error('ancestry should not be checked for missing commits'); + }, + }); + + await expect(compareCheckoutToTargetHead(git, 'missing-target-head', 'current-head')).resolves.toBe('unknown'); + }); + + it('reports unknown when the current commit cannot be inspected locally', async () => { + const git = gitDouble({ + hasCommit: (sha) => sha === 'target-head', + isAncestor: () => { + throw new Error('ancestry should not be checked for missing commits'); + }, + }); + + await expect(compareCheckoutToTargetHead(git, 'target-head', 'missing-current-head')).resolves.toBe('unknown'); + }); + + it('reports diverged only after both commits are available for ancestry checks', async () => { + const git = gitDouble({ + hasCommit: () => true, + isAncestor: () => false, + }); + + await expect(compareCheckoutToTargetHead(git, 'target-head', 'current-head')).resolves.toBe('diverged'); + }); +}); diff --git a/src/cli/commands/status.ts b/src/cli/commands/status.ts index e8c8c3d..9cfd668 100644 --- a/src/cli/commands/status.ts +++ b/src/cli/commands/status.ts @@ -27,7 +27,7 @@ export function registerStatusCommand(program: Command): void { // Compute output states const summaryState = await ws.getOutputState('summary'); - const reviewState = await ws.getOutputState('review'); + const reviewState = await ws.getPendingOutputState('review'); if (opts.json) { const target = ref @@ -77,9 +77,11 @@ export function registerStatusCommand(program: Command): void { console.log(chalk.bold(`${targetKind} ${targetDisplayId}: ${t.title}`)); console.log(` ${chalk.dim('Repository:')} ${t.repository}`); + console.log(` ${chalk.dim('State:')} ${stateColor(t.state)}`); console.log(` ${chalk.dim('Author:')} @${t.author}`); console.log(` ${chalk.dim('Branch:')} ${t.sourceBranch} → ${t.targetBranch}`); - console.log(` ${chalk.dim('State:')} ${stateColor(t.state)}`); + console.log(` ${chalk.dim(`Updated:`)} ${formatDate(t.updatedAt)}`); + if (t.webUrl) { console.log(` ${chalk.dim('URL:')} ${t.webUrl}`); } @@ -99,21 +101,15 @@ export function registerStatusCommand(program: Command): void { // Local checkout info const git = new GitHelper(process.cwd()); + let checkoutRelation: CheckoutRelation = 'unknown'; try { const [currentHead, currentBranch] = await Promise.all([git.headSha(), git.currentBranch()]); - const matchesTarget = currentHead === comparisonTargetHead; - - console.log(chalk.dim('─ Local checkout ─')); + console.log(chalk.dim('─ Checkout ─')); console.log(` ${chalk.dim('Branch:')} ${currentBranch}`); console.log(` ${chalk.dim('Current HEAD:')} ${currentHead.slice(0, 7)}`); - if (matchesTarget) { - console.log(` ${chalk.dim(`Matches ${targetKind}:`)} ${chalk.green('yes')}`); - } else { - const isAncestor = await git.isAncestor(comparisonTargetHead).catch(() => false); - const relation = isAncestor ? `ahead of ${targetKind} head` : `behind ${targetKind} head`; - console.log(` ${chalk.dim(`${targetKind} head:`)} ${comparisonTargetHead.slice(0, 7)}`); - console.log(` ${chalk.dim(`Matches ${targetKind}:`)} ${chalk.yellow(`no — ${relation}`)}`); - } + checkoutRelation = await compareCheckoutToTargetHead(git, comparisonTargetHead, currentHead); + const status = formatCheckoutState(checkoutRelation, comparisonTargetHead, targetKind); + console.log(` ${chalk.dim(`Status:`)} ${status}`); } catch { // Not a git repo — skip local checkout info } @@ -148,13 +144,6 @@ export function registerStatusCommand(program: Command): void { const checkpointState = getCheckpointState(bundleState); console.log(` ${chalk.dim('Checkpoint:')} ${formatCheckpointState(checkpointState)}`); - // Publish history - if (bundleState.publishedActions.length > 0) { - console.log(''); - console.log(chalk.dim('─ Publish history ─')); - console.log(` ${formatCount(bundleState.publishedActions.length, 'action')} previously published`); - } - // Next step console.log(''); const needsCheckpoint = checkpointState !== 'current'; @@ -181,6 +170,7 @@ export function registerStatusCommand(program: Command): void { summaryReady: hasPublishableSummary, reviewReady: hasPublishableReview, checkpointDue: needsCheckpoint, + checkoutRelation, })) { console.log(formatGuidanceLine(line)); } @@ -236,6 +226,7 @@ async function countJsonArray(filePath: string): Promise { } type CheckpointState = 'none' | 'current' | 'outdated' | 'unknown'; +type CheckoutRelation = 'current' | 'ahead' | 'behind' | 'diverged' | 'unknown'; function getCheckpointState(bundleState: { prepare: { @@ -280,7 +271,52 @@ function formatBundleFreshnessState( targetKind: string, ): string { if (!currentTargetHead) return chalk.yellow('unknown'); - return bundleIsOutdated ? chalk.yellow(`stale — prepared for older ${targetKind} head`) : chalk.green('current'); + return bundleIsOutdated + ? chalk.yellow(`stale — prepared for older ${targetKind} head`) + : chalk.green(`current — matches latest ${targetKind} head`); +} + +export async function compareCheckoutToTargetHead( + git: GitHelper, + comparisonTargetHead: string, + currentHead: string, +): Promise { + if (currentHead === comparisonTargetHead) { + return 'current'; + } + + const [targetExists, currentExists] = await Promise.all([ + git.hasCommit(comparisonTargetHead), + git.hasCommit(currentHead), + ]); + if (!targetExists || !currentExists) { + return 'unknown'; + } + + const targetIsAncestorOfCurrent = await git.isAncestor(comparisonTargetHead, currentHead); + const currentIsAncestorOfTarget = await git.isAncestor(currentHead, comparisonTargetHead); + if (targetIsAncestorOfCurrent) { + return 'ahead'; + } + if (currentIsAncestorOfTarget) { + return 'behind'; + } + return 'diverged'; +} + +function formatCheckoutState(relation: CheckoutRelation, comparisonTargetHead: string, targetKind: string): string { + switch (relation) { + case 'current': + return chalk.green(`current — matches latest ${targetKind} head`); + case 'ahead': + return chalk.yellow(`ahead — local HEAD is not in the ${targetKind} yet`); + case 'behind': + return chalk.yellow(`behind — latest ${targetKind} head is ${comparisonTargetHead.slice(0, 7)}`); + case 'diverged': + return chalk.yellow(`diverged — latest ${targetKind} head is ${comparisonTargetHead.slice(0, 7)}`); + case 'unknown': + return chalk.yellow('unknown'); + } } function isPublishableOutputState(state: string): boolean { @@ -293,7 +329,12 @@ export function buildStatusNextLines(options: { summaryReady: boolean; reviewReady: boolean; checkpointDue: boolean; + checkoutRelation?: CheckoutRelation; }): string[] { + if (options.checkoutRelation === 'ahead') { + return ['Next:', ' Push local commits, then run:', ' revpack prepare']; + } + const contentReady = [options.repliesReady, options.findingsReady, options.summaryReady, options.reviewReady].filter( Boolean, ).length; @@ -376,7 +417,3 @@ function formatDate(iso: string): string { return iso; } } - -function formatCount(count: number, singular: string, plural = `${singular}s`): string { - return `${count} ${count === 1 ? singular : plural}`; -} diff --git a/src/cli/helpers.test.ts b/src/cli/helpers.test.ts index 333d9ff..147be43 100644 --- a/src/cli/helpers.test.ts +++ b/src/cli/helpers.test.ts @@ -2,6 +2,7 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest'; +import { WorkspaceError } from '../core/errors.js'; import type { ReviewProvider } from '../providers/provider.js'; vi.mock('../config/index.js', () => ({ @@ -18,18 +19,33 @@ vi.mock('../providers/factory.js', () => ({ }), })); -const { createOrchestrator } = await import('./helpers.js'); +const { createOrchestrator, handleError } = await import('./helpers.js'); describe('cli helpers', () => { let tmpDir: string; let cwdSpy: MockInstance<() => string>; + let exitSpy: MockInstance; + let consoleErrorSpy: MockInstance; + let debugEnv: string | undefined; beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'revpack-helpers-')); cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmpDir); + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit: 1'); + }); + consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + debugEnv = process.env.DEBUG; }); afterEach(async () => { + if (debugEnv === undefined) { + delete process.env.DEBUG; + } else { + process.env.DEBUG = debugEnv; + } + consoleErrorSpy.mockRestore(); + exitSpy.mockRestore(); cwdSpy.mockRestore(); await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -52,4 +68,34 @@ describe('cli helpers', () => { const explicit = await createOrchestrator(undefined, undefined, { allowActiveLocal: false }); expect((explicit as unknown as { provider: ReviewProvider }).provider.providerType).toBe('github'); }); + + it('prints the user-facing error message and debug hint without DEBUG', () => { + delete process.env.DEBUG; + + expect(() => handleError(new WorkspaceError('Cannot prepare bundle'))).toThrow('process.exit: 1'); + + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('[WORKSPACE_ERROR] Cannot prepare bundle')); + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('Set DEBUG=1 for full stack trace')); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it('prints debug stack frames without repeating the error message', () => { + process.env.DEBUG = '1'; + const err = new Error('Cannot prepare bundle\n\nPush your commits, then run revpack prepare'); + err.stack = [ + 'Error: Cannot prepare bundle', + '', + 'Push your commits, then run revpack prepare', + ' at ReviewOrchestrator.prepare (orchestrator.js:126:23)', + ' at async Command. (prepare.js:20:28)', + ].join('\n'); + + expect(() => handleError(err)).toThrow('process.exit: 1'); + + const output = consoleErrorSpy.mock.calls.map(([message]) => String(message)).join('\n'); + expect(output.match(/Cannot prepare bundle/g)).toHaveLength(1); + expect(output).toContain('Stack trace:'); + expect(output).toContain(' at ReviewOrchestrator.prepare (orchestrator.js:126:23)'); + expect(output).toContain(' at async Command. (prepare.js:20:28)'); + }); }); diff --git a/src/cli/helpers.ts b/src/cli/helpers.ts index d02ae0d..c0e57bd 100644 --- a/src/cli/helpers.ts +++ b/src/cli/helpers.ts @@ -88,9 +88,20 @@ export function handleError(err: unknown): never { } if (process.env.DEBUG) { - console.error(err); + console.error(''); + console.error(chalk.dim('Stack trace:')); + if (err instanceof Error && err.stack) { + const stackFrames = err.stack + .split('\n') + .filter((line) => /^\s+at\s/.test(line)) + .join('\n'); + console.error(chalk.dim(stackFrames || err.stack)); + } else { + console.error(err); + } } else { - console.error(chalk.dim(' Set DEBUG=1 for full stack trace')); + console.error(''); + console.error(chalk.dim('Set DEBUG=1 for full stack trace')); } process.exit(1); diff --git a/src/core/types.ts b/src/core/types.ts index 6622a6b..616c27f 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -288,14 +288,16 @@ export type OutputState = 'empty' | 'pending' | 'published' | 'modified since pu export interface BundleOutputEntry { path: string; +} + +export interface HashTrackedBundleOutputEntry extends BundleOutputEntry { lastPublishedHash?: string; lastPublishedAt?: string; lastPublishedTargetHeadSha?: string; - providerNoteId?: string; } export interface BundleOutputs { - summary: BundleOutputEntry; + summary: HashTrackedBundleOutputEntry; review: BundleOutputEntry; } diff --git a/src/orchestration/orchestrator.test.ts b/src/orchestration/orchestrator.test.ts index e373789..b265302 100644 --- a/src/orchestration/orchestrator.test.ts +++ b/src/orchestration/orchestrator.test.ts @@ -1359,7 +1359,7 @@ describe('ReviewOrchestrator', () => { // ─── Output publish state ────────────────────────────── describe('output publish state', () => { - it('tracks pending, published, modified, and empty output states', async () => { + it('tracks summary publish hashes and review note pending state separately', async () => { const orchestrator = new ReviewOrchestrator({ provider: mockProvider, workingDir: tmpDir }); await orchestrator.prepare('!42', 'group/project'); const ws = new WorkspaceManager(tmpDir); @@ -1368,7 +1368,7 @@ describe('ReviewOrchestrator', () => { const reviewPath = path.join(tmpDir, '.revpack', 'outputs', 'review.md'); expect(await ws.getOutputState('summary')).toBe('empty'); - expect(await ws.getOutputState('review')).toBe('empty'); + expect(await ws.getPendingOutputState('review')).toBe('empty'); const summary = '# Summary\nThis is a test'; await fs.writeFile(summaryPath, summary, 'utf-8'); @@ -1382,13 +1382,7 @@ describe('ReviewOrchestrator', () => { const review = '## Notes\nReview notes'; await fs.writeFile(reviewPath, review, 'utf-8'); - expect(await ws.getOutputState('review')).toBe('pending'); - - await ws.updateOutputPublishState('review', computeContentHash(review), 'bbb'); - expect(await ws.getOutputState('review')).toBe('published'); - - await fs.writeFile(reviewPath, '## Notes\nEdited', 'utf-8'); - expect(await ws.getOutputState('review')).toBe('modified since publish'); + expect(await ws.getPendingOutputState('review')).toBe('pending'); }); it('prefills summary from published description marker and marks it as published', async () => { diff --git a/src/orchestration/orchestrator.ts b/src/orchestration/orchestrator.ts index 7627355..e22ef4b 100644 --- a/src/orchestration/orchestrator.ts +++ b/src/orchestration/orchestrator.ts @@ -203,7 +203,7 @@ export class ReviewOrchestrator { `Cannot prepare an agent-ready bundle because the local checkout is ahead of the ${formatTargetKind(target)} head.\n\n` + `${formatTargetKind(target)} head: ${mrHeadSha}\n` + `Local HEAD: ${localHeadSha}\n\n` + - `Push your commits or reset/switch to the ${formatTargetKind(target)} head, then run:\n\n` + + `Push your commits or reset/switch to the ${formatTargetKind(target)} head, then run:\n` + ` revpack prepare`, ); } else { diff --git a/src/providers/local/local-git-provider.test.ts b/src/providers/local/local-git-provider.test.ts index b428a86..987a7e6 100644 --- a/src/providers/local/local-git-provider.test.ts +++ b/src/providers/local/local-git-provider.test.ts @@ -900,7 +900,6 @@ describe('LocalGitProvider orchestrator boundary', () => { }, review: { path: '.revpack/outputs/review.md', - lastPublishedHash: 'sha256:old-review', }, }, publishedActions: [ @@ -945,6 +944,6 @@ describe('LocalGitProvider orchestrator boundary', () => { expect(result.bundle.target.diffRefs.headSha).toBe('head-sha'); expect(result.bundleState.publishedActions).toEqual([]); expect(result.bundleState.outputs.summary.lastPublishedHash).toBeUndefined(); - expect(result.bundleState.outputs.review.lastPublishedHash).toBeUndefined(); + expect(result.bundleState.outputs.review).toEqual({ path: '.revpack/outputs/review.md' }); }); }); diff --git a/src/workspace/git-helper.ts b/src/workspace/git-helper.ts index 97ce178..d63b242 100644 --- a/src/workspace/git-helper.ts +++ b/src/workspace/git-helper.ts @@ -280,10 +280,10 @@ export class GitHelper { return stdout.trim(); } - /** Check if a commit is an ancestor of HEAD (i.e. HEAD includes that commit). */ - async isAncestor(commitSha: string): Promise { + /** Check if `ancestorSha` is an ancestor of `descendantRef`. */ + async isAncestor(ancestorSha: string, descendantRef = 'HEAD'): Promise { try { - await execGit(['merge-base', '--is-ancestor', commitSha, 'HEAD'], this.cwd); + await execGit(['merge-base', '--is-ancestor', ancestorSha, descendantRef], this.cwd); return true; } catch { return false; diff --git a/src/workspace/workspace-manager.test.ts b/src/workspace/workspace-manager.test.ts index 2ca5433..6a876a0 100644 --- a/src/workspace/workspace-manager.test.ts +++ b/src/workspace/workspace-manager.test.ts @@ -1736,43 +1736,53 @@ describe('WorkspaceManager', () => { }); }); - describe('updateOutputPublishState', () => { - it('returns false when no bundle state exists', async () => { - const result = await manager.updateOutputPublishState('summary', 'hash', 'sha'); - expect(result).toBe(false); + describe('getPendingOutputState', () => { + it('returns empty when no bundle state exists', async () => { + const state = await manager.getPendingOutputState('review'); + expect(state).toBe('empty'); }); - it('stores hash, timestamp, and targetHeadSha', async () => { + it('returns empty when output file does not exist', async () => { await createBundleWithState(manager); - const result = await manager.updateOutputPublishState('review', 'abc123', 'head-sha'); - expect(result).toBe(true); - - const state = await manager.loadBundleState(); - expect(state!.outputs.review.lastPublishedHash).toBe('abc123'); - expect(state!.outputs.review.lastPublishedTargetHeadSha).toBe('head-sha'); - expect(state!.outputs.review.lastPublishedAt).toBeTruthy(); + const state = await manager.getPendingOutputState('review'); + expect(state).toBe('empty'); }); - it('stores providerNoteId when provided', async () => { + it('returns empty when output file is whitespace only', async () => { await createBundleWithState(manager); - await manager.updateOutputPublishState('summary', 'hash', 'sha', 'note-42'); - const state = await manager.loadBundleState(); - expect(state!.outputs.summary.providerNoteId).toBe('note-42'); + await manager.writeOutput('review.md', ' \n '); + const state = await manager.getPendingOutputState('review'); + expect(state).toBe('empty'); }); - it('does not set providerNoteId when not provided', async () => { + it('returns pending when review note content exists regardless of legacy publish hash', async () => { await createBundleWithState(manager); - await manager.updateOutputPublishState('summary', 'hash', 'sha'); const state = await manager.loadBundleState(); - expect(state!.outputs.summary.providerNoteId).toBeUndefined(); + (state!.outputs.review as { lastPublishedHash?: string }).lastPublishedHash = + computeContentHash('## Notes\nReview notes'); + await manager.saveBundleState(state!); + + await manager.writeOutput('review.md', '## Notes\nReview notes'); + + await expect(manager.getPendingOutputState('review')).resolves.toBe('pending'); }); + }); - it('preserves existing providerNoteId when not provided in subsequent call', async () => { + describe('updateOutputPublishState', () => { + it('returns false when no bundle state exists', async () => { + const result = await manager.updateOutputPublishState('summary', 'hash', 'sha'); + expect(result).toBe(false); + }); + + it('stores hash, timestamp, and targetHeadSha', async () => { await createBundleWithState(manager); - await manager.updateOutputPublishState('summary', 'h1', 'sha1', 'note-42'); - await manager.updateOutputPublishState('summary', 'h2', 'sha2'); + const result = await manager.updateOutputPublishState('summary', 'abc123', 'head-sha'); + expect(result).toBe(true); + const state = await manager.loadBundleState(); - expect(state!.outputs.summary.providerNoteId).toBe('note-42'); + expect(state!.outputs.summary.lastPublishedHash).toBe('abc123'); + expect(state!.outputs.summary.lastPublishedTargetHeadSha).toBe('head-sha'); + expect(state!.outputs.summary.lastPublishedAt).toBeTruthy(); }); }); diff --git a/src/workspace/workspace-manager.ts b/src/workspace/workspace-manager.ts index 8a2ecec..742744d 100644 --- a/src/workspace/workspace-manager.ts +++ b/src/workspace/workspace-manager.ts @@ -46,7 +46,6 @@ const OUTPUT_DEFAULTS: readonly [filename: string, content: string][] = [ const OUTPUT_STATE_KEYS = { 'summary.md': 'summary', - 'review.md': 'review', } as const; interface InstructionRoute { @@ -365,9 +364,9 @@ export class WorkspaceManager { digest: prepareSummary.current.threadsDigest, items: threadItems, }, - outputs: previousOutputs ?? { - summary: { path: '.revpack/outputs/summary.md' }, - review: { path: '.revpack/outputs/review.md' }, + outputs: { + summary: previousOutputs?.summary ?? { path: '.revpack/outputs/summary.md' }, + review: { path: previousOutputs?.review.path ?? '.revpack/outputs/review.md' }, }, publishedActions: previousActions ?? [], paths: { @@ -400,23 +399,15 @@ export class WorkspaceManager { } /** - * Update the publish hash for an output file in bundle.json. + * Update the publish hash for a hash-tracked output file in bundle.json. */ - async updateOutputPublishState( - outputKey: 'summary' | 'review', - hash: string, - targetHeadSha: string, - providerNoteId?: string, - ): Promise { + async updateOutputPublishState(outputKey: 'summary', hash: string, targetHeadSha: string): Promise { const state = await this.loadBundleState(); if (!state) return false; const entry = state.outputs[outputKey]; entry.lastPublishedHash = hash; entry.lastPublishedAt = new Date().toISOString(); entry.lastPublishedTargetHeadSha = targetHeadSha; - if (providerNoteId !== undefined) { - entry.providerNoteId = providerNoteId; - } await this.saveBundleState(state); return true; } @@ -424,7 +415,7 @@ export class WorkspaceManager { /** * Compute the current state of an output file relative to its publish hash. */ - async getOutputState(outputKey: 'summary' | 'review'): Promise { + async getOutputState(outputKey: 'summary'): Promise { const state = await this.loadBundleState(); if (!state) return 'empty'; const entry = state.outputs[outputKey]; @@ -441,6 +432,22 @@ export class WorkspaceManager { return currentHash === entry.lastPublishedHash ? 'published' : 'modified since publish'; } + /** + * Compute whether a queue-style output file currently has content to publish. + */ + async getPendingOutputState(outputKey: 'review'): Promise> { + const state = await this.loadBundleState(); + if (!state) return 'empty'; + const entry = state.outputs[outputKey]; + const filePath = path.resolve(this.workingDir, entry.path); + try { + const content = await fs.readFile(filePath, 'utf-8'); + return content.trim() ? 'pending' : 'empty'; + } catch { + return 'empty'; + } + } + /** * Remove the entire bundle directory (.revpack/). */ @@ -468,10 +475,10 @@ export class WorkspaceManager { } /** - * Prefill an output file with content from the last published review note, - * but only if the file is currently empty. This lets agents see and update - * existing content in incremental mode without triggering a "changed" state - * on the next status check (the publish hash stays matched). + * Prefill an output file with published remote content, but only if the file + * is currently empty. This lets agents see and update existing content in + * incremental mode without triggering a "changed" state on the next status + * check (the publish hash stays matched). */ async prefillOutputIfEmpty(filename: string, content: string): Promise { const filePath = path.join(this.baseDir, 'outputs', filename);