diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index cff0a3696..37afa5418 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -161,6 +161,10 @@ import { validateCodeNavRequest, extractChangedFiles, } from "../generated/code-nav.ts"; +import { + REPO_FILE_ERROR_STATUS, + readRepoFile, +} from "../generated/repo-file.ts"; import { createDefaultSemanticDiffRuntime, getSemanticDiffAvailability, @@ -779,6 +783,28 @@ export async function startReviewServer(options: { } return options.agentCwd && existsSync(options.agentCwd) ? options.agentCwd : null; } + /** + * Local working-tree root for /api/code-nav/file and /api/review-file. + * Mirrors resolveLocalFileRoot in packages/server/review.ts. Synchronous + * here because this server's cwd resolution is synchronous (resolveAgentCwd), + * matching how the Pi code-nav routes already behaved. + */ + function resolveLocalFileRoot( + surface: "Code navigation" | "File viewing", + ): { ok: true; root: string } | { ok: false; error: string; status: number } { + if (isGitButlerCommittedView()) { + return { + ok: false, + status: 400, + error: `${surface} is unavailable for committed GitButler views`, + }; + } + const hasAccess = !!workspace || !!options.gitContext || !!options.agentCwd || !!options.worktreePool; + if (!hasAccess) { + return { ok: false, status: 400, error: `${surface} requires local access` }; + } + return { ok: true, root: resolveAgentCwd() }; + } async function ensurePRCallFlowCwd(): Promise { if (options.worktreePool && prMeta) { try { @@ -2975,31 +3001,41 @@ export async function startReviewServer(options: { json(res, { error: err instanceof Error ? err.message : "Code navigation failed" }, 500); } } else if (url.pathname === "/api/code-nav/file" && req.method === "GET") { - if (isGitButlerCommittedView()) { - json(res, { error: "Code navigation is unavailable for committed GitButler views" }, 400); + // Hardened to go through readRepoFile — see the Bun mirror in + // packages/server/review.ts. This route previously had no size cap. + const rootResult = resolveLocalFileRoot("Code navigation"); + if (!rootResult.ok) { + json(res, { error: rootResult.error }, rootResult.status); return; } - const hasCodeNavAccess = !!workspace || !!options.gitContext || !!options.agentCwd || !!options.worktreePool; - if (!hasCodeNavAccess) { - json(res, { error: "Code navigation requires local access" }, 400); + const result = readRepoFile(rootResult.root, url.searchParams.get("path")); + if (!result.ok) { + json(res, { error: result.message }, REPO_FILE_ERROR_STATUS[result.reason]); return; } - const filePath = url.searchParams.get("path"); - if (!filePath) { - json(res, { error: "Missing path" }, 400); + json(res, { content: result.content }); + } else if (url.pathname === "/api/review-file" && req.method === "GET") { + // Full file content for the full-file review viewer. Serves the live + // working tree with no snapshot guard, by design — see the Bun mirror. + const rootResult = resolveLocalFileRoot("File viewing"); + if (!rootResult.ok) { + json(res, { error: rootResult.error }, rootResult.status); return; } - try { validateFilePath(filePath); } catch { - json(res, { error: "Invalid path" }, 400); + const result = readRepoFile(rootResult.root, url.searchParams.get("path")); + if (!result.ok) { + json( + res, + { error: result.message, reason: result.reason, size: result.size }, + REPO_FILE_ERROR_STATUS[result.reason], + ); return; } - try { - const navCwd = resolveAgentCwd(); - const content = readFileSync(`${navCwd}/${filePath}`, "utf-8"); - json(res, { content }); - } catch { - json(res, { error: "File not found" }, 404); - } + json(res, { + filePath: result.filePath, + content: result.content, + size: result.size, + }); } else if (url.pathname === "/api/config" && req.method === "POST") { try { const body = (await parseBody(req)) as { displayName?: string; diffOptions?: Record; theme?: Record; favicon?: FaviconStyle; reviewAnalysis?: Record; conventionalComments?: boolean }; diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index f2f0a0efd..f6b63a8fd 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -29,7 +29,7 @@ for f in config-types storage-types workspace-status-types; do done # Everything else in the original flat list stays sourced from packages/shared. -for f in prompts review-core generated-files cli-pagination jj-core gitbutler-core vcs-core review-args draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common markdown-extensions resolve-file file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node; do +for f in prompts review-core generated-files cli-pagination jj-core gitbutler-core vcs-core review-args draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common markdown-extensions resolve-file file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav repo-file data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node; do src="../../packages/shared/$f.ts" # Shared modules that import browser-safe siblings from @plannotator/core # (e.g. guide-store → core/guide-format): generated/ is flat and vendors the diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index d5a023d52..ec3f761a0 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -106,6 +106,8 @@ import { REVIEW_CALL_FLOW_PANEL_ID, REVIEW_ALL_FILES_PANEL_ID, REVIEW_CODE_NAV_PANEL_ID, + REVIEW_FULL_FILE_PANEL_ID, + getReviewFullFilePanelFilePath, } from './dock/reviewPanelTypes'; import type { DiffFile, AnnotationScrollTarget } from './types'; import { annotationMatchesPrScope, proseAnnotationMatchesPr } from './utils/annotationScope'; @@ -337,6 +339,9 @@ const ReviewApp: React.FC = () => { const apiModeRef = useRef(false); const analysisSettingsInitialized = useRef(false); const [isDiffPanelActive, setIsDiffPanelActive] = useState(false); + const [isFullFileActive, setIsFullFileActive] = useState(false); + /** Path the full-file panel currently holds — feeds diff-panel focus arbitration. */ + const [fullFileActivePath, setFullFileActivePath] = useState(null); const [allFilesVisibleFile, setAllFilesVisibleFile] = useState(null); const [pendingSelection, setPendingSelection] = useState(null); const [lineAnnotationComposeRequest, setLineAnnotationComposeRequest] = @@ -713,6 +718,39 @@ const ReviewApp: React.FC = () => { needsInitialDiffPanel.current = false; }, [dockApi, files, clearPendingSelection]); + /** + * Open a file whole in the full-file panel (design doc phase 1). + * + * One reused panel retargeted per file, exactly like openDiffFile — the + * recommended model over tab-per-file, which cannot easily be walked back. + * Unlike openDiffFile this does NOT require the path to be in `files`: the + * point of the feature is opening files the patch never mentions (code-nav + * results, and later the repo tree). + */ + const openFullFile = useCallback((filePath: string, line?: number) => { + if (!dockApi) return; + const title = filePath.split('/').pop() || filePath; + const existing = dockApi.getPanel(REVIEW_FULL_FILE_PANEL_ID); + if (existing) { + const existingFilePath = getReviewFullFilePanelFilePath(existing.params); + if (existingFilePath !== filePath || line != null) { + existing.api.updateParameters({ filePath, ...(line != null && { line }) }); + existing.api.setTitle(title); + } + setFullFileActivePath(filePath); + existing.api.setActive(); + return; + } + clearPendingSelection(); + dockApi.addPanel({ + id: REVIEW_FULL_FILE_PANEL_ID, + component: REVIEW_PANEL_TYPES.FULL_FILE, + title, + params: { filePath, ...(line != null && { line }) }, + }); + setFullFileActivePath(filePath); + }, [dockApi, clearPendingSelection]); + const isCallFlowNodeInPatch = useCallback((node: CallFlowNode): boolean => { if (!node.file || !node.line) return false; const file = files.find((candidate) => candidate.path === node.file || candidate.oldPath === node.file); @@ -1138,6 +1176,7 @@ const ReviewApp: React.FC = () => { setIsPROverviewActive(false); setIsPRArtifactsActive(false); setIsDiffPanelActive(false); + setIsFullFileActive(false); return; } setIsAllFilesActive(panel.id === REVIEW_ALL_FILES_PANEL_ID); @@ -1146,6 +1185,10 @@ const ReviewApp: React.FC = () => { setIsPROverviewActive(panel.id === REVIEW_PR_OVERVIEW_PANEL_ID); setIsPRArtifactsActive(panel.id === REVIEW_PR_ARTIFACTS_PANEL_ID); setIsDiffPanelActive(isReviewDiffPanelId(panel.id)); + setIsFullFileActive(panel.id === REVIEW_FULL_FILE_PANEL_ID); + if (panel.id === REVIEW_FULL_FILE_PANEL_ID) { + setFullFileActivePath(getReviewFullFilePanelFilePath(panel.params)); + } if (!isReviewDiffPanelId(panel.id)) return; const filePath = getReviewDiffPanelFilePath(panel.params); if (!filePath) return; @@ -1841,11 +1884,19 @@ const ReviewApp: React.FC = () => { originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], - tokenMeta?: TokenAnnotationMeta + tokenMeta?: TokenAnnotationMeta, + selectionSnippet?: string ) => { if (!pendingSelection) return; const lineStart = Math.min(pendingSelection.start, pendingSelection.end); const lineEnd = Math.max(pendingSelection.start, pendingSelection.end); + const side = pendingSelection.side === 'additions' ? 'new' : 'old'; + // Stamp annotations whose lines the agent will NOT find in the patch: + // authored in the full-file viewer (file absent from the diff, or a range + // past every hunk) or on expanded diff context. Computed here rather than + // at export time because only this side of the app holds the patch. + const filePatch = files.find(candidate => candidate.path === filePath)?.patch ?? ''; + const outsideDiff = !filePatch || !isLineRangeInPatch(filePatch, lineStart, lineEnd, side); const newAnnotation: CodeAnnotation = { id: generateId(), type, @@ -1853,10 +1904,15 @@ const ReviewApp: React.FC = () => { filePath, lineStart, lineEnd, - side: pendingSelection.side === 'additions' ? 'new' : 'old', + side, text, suggestedCode, - originalCode, + // `originalCode` stays suggestion-only in-diff (it renders as + // "Replaces:"). For an out-of-diff annotation the selected lines are + // attached regardless, because the patch does not contain them and the + // export has to quote them for the agent to see anything at all. + originalCode: originalCode ?? (outsideDiff ? selectionSnippet : undefined), + ...(outsideDiff && { outsideDiff: true }), ...(tokenMeta && { charStart: tokenMeta.charStart, charEnd: tokenMeta.charEnd, @@ -1869,7 +1925,7 @@ const ReviewApp: React.FC = () => { }; setAnnotations(prev => [...prev, withPRContext(newAnnotation)]); clearPendingSelection(); - }, [pendingSelection, identity, withPRContext, clearPendingSelection]); + }, [pendingSelection, identity, withPRContext, clearPendingSelection, files]); const handleAddCallFlowAnnotation = useCallback(( targets: readonly CallFlowAnnotationTarget[], @@ -2906,6 +2962,8 @@ const ReviewApp: React.FC = () => { // focus claim at the source instead of threading `guideOpen` through every // dock panel. Guide-side DiffViewers arbitrate focus among themselves. focusedFilePath: guideOpen ? null : (files[activeFileIndex]?.path ?? null), + fullFileFocusPath: isFullFileActive ? fullFileActivePath : null, + onOpenFullFile: openFullFile, diffStyle: effectiveDiffStyle, onDiffStyleChange: handleDiffStyleChange, isCompactTouchLayout, @@ -4116,6 +4174,7 @@ const ReviewApp: React.FC = () => { > { > completeNavigatorSelection(openPROverviewPanel)} isPROverviewActive={isPROverviewActive} diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index fb551d421..0bdacdac8 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -856,6 +856,12 @@ export const DiffViewer: React.FC = ({ void; onDoubleClickFile?: (index: number) => void; + /** Open a file whole in the full-file viewer (design doc phase 1). */ + onOpenFile?: (filePath: string) => void; annotations: CodeAnnotation[]; viewedFiles: Set; onToggleViewed?: (filePath: string) => void; @@ -127,6 +129,7 @@ export const FileTree: React.FC = ({ activeFileIndex, onSelectFile, onDoubleClickFile, + onOpenFile, annotations, viewedFiles, onToggleViewed, @@ -565,6 +568,7 @@ export const FileTree: React.FC = ({ scrollHighlightIndex={isAllFilesActive ? scrollHighlightIndex : undefined} onSelectFile={onSelectFile} onDoubleClickFile={onDoubleClickFile} + onOpenFile={onOpenFile} viewedFiles={viewedFiles} onToggleViewed={onToggleViewed} showViewedControls={showViewedControls} diff --git a/packages/review-editor/components/FileTreeNode.tsx b/packages/review-editor/components/FileTreeNode.tsx index 8af20109c..49ed41a82 100644 --- a/packages/review-editor/components/FileTreeNode.tsx +++ b/packages/review-editor/components/FileTreeNode.tsx @@ -11,6 +11,7 @@ interface FileTreeNodeProps { activeFileIndex: number; onSelectFile: (index: number) => void; onDoubleClickFile?: (index: number) => void; + onOpenFile?: (filePath: string) => void; viewedFiles: Set; onToggleViewed?: (filePath: string) => void; showViewedControls?: boolean; @@ -55,6 +56,7 @@ export const FileTreeNodeItem: React.FC = ({ activeFileIndex, onSelectFile, onDoubleClickFile, + onOpenFile, viewedFiles, onToggleViewed, showViewedControls = true, @@ -114,6 +116,7 @@ export const FileTreeNodeItem: React.FC = ({ activeFileIndex={activeFileIndex} onSelectFile={onSelectFile} onDoubleClickFile={onDoubleClickFile} + onOpenFile={onOpenFile} viewedFiles={viewedFiles} onToggleViewed={onToggleViewed} showViewedControls={showViewedControls} @@ -191,11 +194,38 @@ export const FileTreeNodeItem: React.FC = ({ {node.name} + {onOpenFile && ( + { + // The row button owns the click; this affordance opens the + // whole file instead of the diff. + e.stopPropagation(); + e.preventDefault(); + onOpenFile(node.path); + }} + > + ⤢ + + )} + {onOpenFile && ( + onOpenFile(node.path)} + className="flex items-center gap-2 mx-1 px-2 py-1.5 text-xs rounded cursor-pointer outline-none text-foreground/80 data-[highlighted]:bg-muted data-[highlighted]:text-foreground" + > + Open whole file + + )} { void copyTextToClipboard(node.path); }} className="flex items-center gap-2 mx-1 px-2 py-1.5 text-xs rounded cursor-pointer outline-none text-foreground/80 data-[highlighted]:bg-muted data-[highlighted]:text-foreground" diff --git a/packages/review-editor/components/SectionsPanel.tsx b/packages/review-editor/components/SectionsPanel.tsx index 44c0a8097..fa3dd1369 100644 --- a/packages/review-editor/components/SectionsPanel.tsx +++ b/packages/review-editor/components/SectionsPanel.tsx @@ -53,6 +53,8 @@ interface SectionsPanelProps { scrollHighlightIndex?: number; onSelectFile: (index: number) => void; onDoubleClickFile?: (index: number) => void; + /** Open a file whole in the full-file viewer (design doc phase 1). */ + onOpenFile?: (filePath: string) => void; /** j/k/arrows/Home/End file navigation (disabled while modals are open). */ enableKeyboardNav?: boolean; annotations: CodeAnnotation[]; @@ -145,6 +147,8 @@ const SectionRow: React.FC<{ isStaged: boolean; isStaging: boolean; onStage?: () => void; + /** Open this file whole in the full-file viewer (design doc phase 1). */ + onOpenFile?: (filePath: string) => void; }> = ({ item, isActive, @@ -161,6 +165,7 @@ const SectionRow: React.FC<{ isStaged, isStaging, onStage, + onOpenFile, }) => { const { file } = item; @@ -195,6 +200,25 @@ const SectionRow: React.FC<{ + {onOpenFile && ( + { + // The row button owns the click; this affordance opens the whole + // file instead of the diff. + e.stopPropagation(); + e.preventDefault(); + onOpenFile(file.path); + }} + > + ⤢ + + )} ); @@ -208,6 +232,7 @@ export const SectionsPanel: React.FC = ({ scrollHighlightIndex, onSelectFile, onDoubleClickFile, + onOpenFile, enableKeyboardNav, annotations, viewedFiles, @@ -456,6 +481,7 @@ export const SectionsPanel: React.FC = ({ isStaged={item.staged} isStaging={stagingFile === item.file.path} onStage={onStageFile ? () => onStageFile(item.file.path) : undefined} + onOpenFile={onOpenFile} /> )); diff --git a/packages/review-editor/components/ToolbarHost.tsx b/packages/review-editor/components/ToolbarHost.tsx index e53283af5..0ce1485f2 100644 --- a/packages/review-editor/components/ToolbarHost.tsx +++ b/packages/review-editor/components/ToolbarHost.tsx @@ -26,6 +26,8 @@ export interface ToolbarHostHandle { interface ToolbarHostProps { patch: string; + /** Whole new-side file contents, when the surface has them (see useAnnotationToolbar). */ + fileContent?: string; filePath: string; isFocused: boolean; onLineSelection: (range: SelectedLineRange | null) => void; @@ -37,6 +39,7 @@ interface ToolbarHostProps { conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta, + selectionSnippet?: string, ) => void; onEditAnnotation: ( id: string, @@ -61,6 +64,7 @@ interface ToolbarHostProps { export const ToolbarHost = forwardRef(function ToolbarHost( { patch, + fileContent, filePath, isFocused, onLineSelection, @@ -76,6 +80,7 @@ export const ToolbarHost = forwardRef(funct ) { const toolbar = useAnnotationToolbar({ patch, + fileContent, filePath, isFocused, onLineSelection, diff --git a/packages/review-editor/dock/ReviewStateContext.tsx b/packages/review-editor/dock/ReviewStateContext.tsx index 8abe00072..b8f84a6e2 100644 --- a/packages/review-editor/dock/ReviewStateContext.tsx +++ b/packages/review-editor/dock/ReviewStateContext.tsx @@ -35,6 +35,18 @@ export interface ReviewState { rawPatch: string; focusedFileIndex: number; focusedFilePath: string | null; + /** + * Path currently claimed by an ACTIVE full-file panel, else null. + * + * Focus arbitration (design doc risk 1): ToolbarHost annotation drafts live + * in module-level maps keyed by filePath, so if a full-file panel and a diff + * panel both reported isFocused for the same path, draft handoff would + * corrupt on a last-write-wins basis. The diff panel yields while the + * full-file panel holds the same file. + */ + fullFileFocusPath: string | null; + /** Open a path whole in the full-file panel, optionally revealing a line. */ + onOpenFullFile?: (filePath: string, line?: number) => void; diffStyle: 'split' | 'unified'; /** Compact touch shells use a session-only style so desktop preferences stay untouched. */ onDiffStyleChange: (style: 'split' | 'unified') => void; @@ -82,8 +94,8 @@ export interface ReviewState { targets: readonly CallFlowAnnotationTarget[], text: string, ) => boolean; - onAddAnnotation: (type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta) => void; - onAddAnnotationForFile: (filePath: string, type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta) => void; + onAddAnnotation: (type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta, selectionSnippet?: string) => void; + onAddAnnotationForFile: (filePath: string, type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta, selectionSnippet?: string) => void; /** EXPERIMENTAL edit-to-suggestion flag (cookie setting, default OFF). Only * the plain all-files panel consumes it — Guided Review surfaces stay off. */ editSuggestionsEnabled: boolean; diff --git a/packages/review-editor/dock/panels/ReviewCodeNavPanel.tsx b/packages/review-editor/dock/panels/ReviewCodeNavPanel.tsx index e6fe5d3c4..726b0ef86 100644 --- a/packages/review-editor/dock/panels/ReviewCodeNavPanel.tsx +++ b/packages/review-editor/dock/panels/ReviewCodeNavPanel.tsx @@ -250,8 +250,27 @@ export const ReviewCodeNavPanel: React.FC = (props) => { className="h-full flex flex-col border-t border-border/50" >
-
- +
+ {selectedLocation && state.onOpenFullFile && ( +
+ + {selectedLocation.filePath}:{selectedLocation.line} + + +
+ )} +
+ +
= (props) => { const file = filePath ? state.files.find(candidate => candidate.path === filePath) : undefined; - const isFocusedFile = !!file && state.focusedFilePath === file.path; + const isFocusedFile = + !!file && + state.focusedFilePath === file.path && + state.fullFileFocusPath !== file.path; const fileAnnotations = useMemo( () => { diff --git a/packages/review-editor/dock/panels/ReviewFullFilePanel.tsx b/packages/review-editor/dock/panels/ReviewFullFilePanel.tsx new file mode 100644 index 000000000..5397eef65 --- /dev/null +++ b/packages/review-editor/dock/panels/ReviewFullFilePanel.tsx @@ -0,0 +1,370 @@ +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import type { IDockviewPanelProps } from 'dockview-react'; +import { CodeView, type CodeViewHandle, type CodeViewItem, type LineAnnotation } from '@pierre/diffs/react'; +import type { CodeViewLineSelection } from '@pierre/diffs'; +import type { SelectedLineRange as PierreSelectedLineRange } from '@pierre/diffs'; + +import { useReviewState } from '../ReviewStateContext'; +import { + getReviewFullFilePanelFilePath, + getReviewFullFilePanelLine, + type ReviewFullFilePanelParams, +} from '../reviewPanelTypes'; +import { usePierreTheme } from '../../hooks/usePierreTheme'; +import { useWorkerPoolThemeSync } from '../../workerPool'; +import { ToolbarHost, type ToolbarHostHandle } from '../../components/ToolbarHost'; +import { InlineAnnotation } from '../../components/InlineAnnotation'; +import { lineAnnotationMetadata } from '../../utils/annotationDisplay'; +import { annotationMatchesPrScope } from '../../utils/annotationScope'; +import { hashString } from '../../utils/hashString'; +import { detectLanguage } from '../../utils/detectLanguage'; +import type { DiffAnnotationMetadata } from '@plannotator/ui/types'; + + +/** + * Map a DOM node inside Pierre's rendered code to its 1-based line number. + * Ported from CodeFilePopout, which solves the same problem on the annotate + * side: Pierre's own drag gesture does not surface a range on a CodeView file + * item, so a multi-line selection has to be read back off the DOM selection. + */ +function lineNumberFromNode(node: Node | null): number | null { + let current: Node | null = node; + if (current?.nodeType === Node.TEXT_NODE) current = current.parentNode; + while (current) { + if (current instanceof HTMLElement) { + const line = current.closest('[data-line]')?.getAttribute('data-line'); + if (line) { + const parsed = Number(line); + return Number.isFinite(parsed) ? parsed : null; + } + } + current = current.parentNode; + } + return null; +} + +/** Pierre renders into a shadow root, which owns its own selection. */ +function pierreSelection(root: HTMLElement | null): Selection | null { + const shadowRoot = root?.querySelector('diffs-container')?.shadowRoot; + const shadowSelection = ( + shadowRoot as (ShadowRoot & { getSelection?: () => Selection | null }) | null + )?.getSelection?.(); + return shadowSelection && !shadowSelection.isCollapsed ? shadowSelection : window.getSelection(); +} + +/** + * Full-file viewer panel (design doc phase 1). + * + * Renders ONE Pierre `CodeViewFileItem`. That is what buys virtualization, + * shared worker-pool highlighting, line selection and line annotations for + * free — the same machinery the diff surfaces use — so a 10k-line file costs + * the viewport, not the file. + * + * Content comes from /api/review-file: the live working tree, deliberately + * without a snapshot guard (see the endpoint comment). Annotations authored + * here are ordinary CodeAnnotations, so they join the one review feedback + * stream rather than a second channel. + */ +export const ReviewFullFilePanel: React.FC = (props) => { + const state = useReviewState(); + // Double read: updateParameters does not always flow into props.params + // synchronously (the same reason ReviewDiffPanel reads both). + const filePath = + getReviewFullFilePanelFilePath(props.params) ?? + getReviewFullFilePanelFilePath(props.api.getParameters()); + const targetLine = + getReviewFullFilePanelLine(props.params) ?? + getReviewFullFilePanelLine(props.api.getParameters()); + + const [content, setContent] = useState<{ forPath: string; text: string } | null>(null); + const [error, setError] = useState(null); + const [loading, setLoading] = useState(false); + const [isActive, setIsActive] = useState(props.api.isActive); + const toolbarHostRef = useRef(null); + // A multi-line drag ends with a selection event AND a click on the release + // line. Without this window the click reopens the composer on that single + // line and the range the user just dragged is silently lost. Same guard + // CodeFilePopout uses on the annotate side. + const suppressLineClickUntilRef = useRef(0); + // Pierre repaints a drag-selection only in CONTROLLED mode: a defined + // `selectedLines` plus a change handler. Uncontrolled, a range drag paints + // nothing and never reaches the composer. + const [selectedLines, setSelectedLines] = useState(null); + const codeViewRef = useRef | null>(null); + const surfaceRef = useRef(null); + + const pierreTheme = usePierreTheme(); + useWorkerPoolThemeSync(pierreTheme.syntaxTheme); + + // Focus arbitration (design doc risk 1): ToolbarHost drafts live in + // module-level maps keyed by filePath, so a full-file panel and a diff panel + // both claiming isFocused for one path would corrupt draft handoff on a + // last-write-wins basis. Only the ACTIVE dock panel ever claims it. + useEffect(() => { + setIsActive(props.api.isActive); + const disposable = props.api.onDidActiveChange((event) => setIsActive(event.isActive)); + return () => disposable.dispose(); + }, [props.api]); + + // --- content ------------------------------------------------------------ + useEffect(() => { + if (!filePath) return; + const controller = new AbortController(); + setLoading(true); + setError(null); + fetch(`/api/review-file?path=${encodeURIComponent(filePath)}`, { + signal: controller.signal, + }) + .then(async (res) => { + const data = (await res.json().catch(() => null)) as + | { content?: string; error?: string } + | null; + if (!res.ok) throw new Error(data?.error || `Could not open ${filePath}`); + return data?.content ?? ''; + }) + .then((text) => { + setContent({ forPath: filePath, text }); + setLoading(false); + }) + .catch((err: unknown) => { + if (controller.signal.aborted) return; + setError(err instanceof Error ? err.message : 'Could not open file'); + setLoading(false); + }); + return () => controller.abort(); + }, [filePath]); + + const contents = content?.forPath === filePath ? content.text : ''; + + // --- annotations -------------------------------------------------------- + const fileAnnotations = useMemo(() => { + if (!filePath) return []; + return state.allAnnotations.filter( + (a) => + a.filePath === filePath && + (a.scope ?? 'line') === 'line' && + annotationMatchesPrScope(a, state.prMetadata?.url, state.prDiffScope), + ); + }, [state.allAnnotations, filePath, state.prMetadata, state.prDiffScope]); + + const lineAnnotations = useMemo( + (): LineAnnotation[] => + fileAnnotations.map((ann) => ({ + lineNumber: ann.lineEnd, + metadata: lineAnnotationMetadata(ann), + })), + [fileAnnotations], + ); + + // Pierre 1.3.2 identity hazard: it compares nothing but cacheKey, so the key + // must be content-derived or a second file rendered at the same path is + // silently served the first one's cached render. + const items = useMemo((): CodeViewItem[] => { + if (!filePath || !contents) return []; + return [ + { + id: filePath, + type: 'file', + file: { + name: filePath, + contents, + cacheKey: `${filePath}#file#${hashString(contents)}`, + }, + annotations: lineAnnotations, + }, + ]; + }, [filePath, contents, lineAnnotations]); + + // Reveal the requested line once the file has rendered. + useEffect(() => { + if (!targetLine || !filePath || !contents) return; + const timer = setTimeout(() => { + codeViewRef.current?.scrollTo({ + type: 'line', + id: filePath, + lineNumber: targetLine, + align: 'center', + }); + }, 50); + return () => clearTimeout(timer); + }, [targetLine, filePath, contents]); + + // --- selection gestures ------------------------------------------------- + // Everything here is the "new" side: this surface shows the working tree. + const handleSelectedLinesChange = useCallback( + (selection: { id: string; range: PierreSelectedLineRange } | null) => { + setSelectedLines(selection); + if (!selection) { + toolbarHostRef.current?.handleLineSelectionEnd(null); + return; + } + const start = Math.min(selection.range.start, selection.range.end); + const end = Math.max(selection.range.start, selection.range.end); + if (start !== end) { + suppressLineClickUntilRef.current = Date.now() + 300; + } + toolbarHostRef.current?.handleLineSelectionEnd({ start, end, side: 'additions' }); + }, + [], + ); + + // CodeView's callback type is an overload spanning file AND diff items, so + // the handler is typed on the narrow shape both overloads supply. + const handleLineClick = useCallback( + (lineProps: { lineNumber: number }) => { + if (Date.now() < suppressLineClickUntilRef.current) return; + toolbarHostRef.current?.openLineAnnotation({ + start: lineProps.lineNumber, + end: lineProps.lineNumber, + side: 'additions', + }); + }, + [], + ); + + /** + * Turn a browser text selection spanning several lines into a line-range + * annotation. Without this, dragging across code selects text and nothing + * else happens — the reviewer's range is lost. + */ + const openRangeFromTextSelection = useCallback(() => { + const selection = pierreSelection(surfaceRef.current); + if (!selection || selection.isCollapsed) return; + const anchor = lineNumberFromNode(selection.anchorNode); + const focus = lineNumberFromNode(selection.focusNode); + if (anchor == null || focus == null || anchor === focus) return; + const start = Math.min(anchor, focus); + const end = Math.max(anchor, focus); + suppressLineClickUntilRef.current = Date.now() + 300; + setSelectedLines({ id: filePath ?? '', range: { start, end } }); + toolbarHostRef.current?.handleLineSelectionEnd({ start, end, side: 'additions' }); + selection.removeAllRanges?.(); + }, [filePath]); + + const handleAddAnnotation = useCallback( + (type, text, suggestedCode, originalCode, conventionalLabel, decorations, tokenMeta, selectionSnippet) => { + if (!filePath) return; + state.onAddAnnotationForFile( + filePath, + type, + text, + suggestedCode, + originalCode, + conventionalLabel, + decorations, + tokenMeta, + selectionSnippet, + ); + }, + [filePath, state.onAddAnnotationForFile], + ); + + const language = filePath ? detectLanguage(filePath) : undefined; + + const renderAnnotation = useCallback( + (annotation: { metadata?: DiffAnnotationMetadata }) => { + const metadata = annotation.metadata; + if (!metadata) return null; + return ( + { + const ann = state.allAnnotations.find((a) => a.id === id); + if (ann) toolbarHostRef.current?.startEdit(ann); + }} + onDelete={state.onDeleteAnnotation} + /> + ); + }, + [ + language, + state.selectedAnnotationId, + state.onSelectAnnotation, + state.onDeleteAnnotation, + state.allAnnotations, + ], + ); + + if (!filePath) { + return ( +
+ No file selected +
+ ); + } + + return ( +
+
+ + {filePath} + + + Full file + + {contents && ( + + {contents.split('\n').length} lines + + )} +
+ +
requestAnimationFrame(openRangeFromTextSelection)} + > + {error ? ( +
+ {error} +
+ ) : loading && !contents ? ( +
+ Loading {filePath}… +
+ ) : ( + + ref={codeViewRef} + items={items} + // Containment mirrors AllFilesCodeView (and Pierre's own production + // wrapper): CodeView virtualizes against ITS OWN scroll container, + // so without overflow-y-auto here nothing scrolls and the virtual + // window never advances past the first screen of lines. + className="relative h-full overflow-y-auto overflow-x-clip overscroll-contain [contain:strict] [overflow-anchor:none] [will-change:scroll-position] [&_diffs-container]:overflow-clip [&_diffs-container]:[contain:layout_paint_style]" + selectedLines={selectedLines} + onSelectedLinesChange={handleSelectedLinesChange} + renderAnnotation={renderAnnotation} + options={{ + themeType: pierreTheme.type, + unsafeCSS: pierreTheme.css, + ...(pierreTheme.syntaxTheme && { theme: pierreTheme.syntaxTheme }), + disableFileHeader: true, + overflow: 'scroll', + enableLineSelection: true, + lineHoverHighlight: 'line', + onLineClick: handleLineClick, + }} + /> + )} +
+ + +
+ ); +}; diff --git a/packages/review-editor/dock/reviewPanelComponents.ts b/packages/review-editor/dock/reviewPanelComponents.ts index 16a50b3da..fc564c244 100644 --- a/packages/review-editor/dock/reviewPanelComponents.ts +++ b/packages/review-editor/dock/reviewPanelComponents.ts @@ -7,6 +7,7 @@ import { ReviewAllFilesDiffPanel } from './panels/ReviewAllFilesDiffPanel'; import { ReviewCodeNavPanel } from './panels/ReviewCodeNavPanel'; import { ReviewSemanticDiffPanel } from './panels/ReviewSemanticDiffPanel'; import { ReviewCallFlowPanel } from './panels/ReviewCallFlowPanel'; +import { ReviewFullFilePanel } from './panels/ReviewFullFilePanel'; /** * Component registry for dockview — maps panel type strings to React components. @@ -21,4 +22,5 @@ export const reviewPanelComponents = { [REVIEW_PANEL_TYPES.CODE_NAV]: ReviewCodeNavPanel, [REVIEW_PANEL_TYPES.SEMANTIC_DIFF]: ReviewSemanticDiffPanel, [REVIEW_PANEL_TYPES.CALL_FLOW]: ReviewCallFlowPanel, + [REVIEW_PANEL_TYPES.FULL_FILE]: ReviewFullFilePanel, } as const; diff --git a/packages/review-editor/dock/reviewPanelTypes.ts b/packages/review-editor/dock/reviewPanelTypes.ts index b0a8ad767..5b291b140 100644 --- a/packages/review-editor/dock/reviewPanelTypes.ts +++ b/packages/review-editor/dock/reviewPanelTypes.ts @@ -14,6 +14,7 @@ export const REVIEW_PANEL_TYPES = { CODE_NAV: 'review-code-nav', SEMANTIC_DIFF: 'review-semantic-diff', CALL_FLOW: 'review-call-flow', + FULL_FILE: 'review-full-file', } as const; export const REVIEW_DIFF_PANEL_ID = 'review-diff'; @@ -31,6 +32,30 @@ export const REVIEW_ALL_FILES_PANEL_ID = 'review-all-files'; export const REVIEW_CODE_NAV_PANEL_ID = 'review-code-nav'; export const REVIEW_SEMANTIC_DIFF_PANEL_ID = 'review-semantic-diff'; export const REVIEW_CALL_FLOW_PANEL_ID = 'review-call-flow'; +/** + * One reused full-file panel, retargeted per file exactly like the diff panel + * (REVIEW_DIFF_PANEL_ID). Tab-per-file would be more editor-like but cannot + * easily be walked back; a retargeted panel can grow a "pin as new tab" later. + */ +export const REVIEW_FULL_FILE_PANEL_ID = 'review-full-file'; + +export interface ReviewFullFilePanelParams { + filePath: string; + /** Line to reveal on open, when the caller has one (code-nav results). */ + line?: number; +} + +export function getReviewFullFilePanelFilePath(params: unknown): string | null { + if (!params || typeof params !== 'object') return null; + const filePath = (params as { filePath?: unknown }).filePath; + return typeof filePath === 'string' ? filePath : null; +} + +export function getReviewFullFilePanelLine(params: unknown): number | null { + if (!params || typeof params !== 'object') return null; + const line = (params as { line?: unknown }).line; + return typeof line === 'number' && Number.isFinite(line) ? line : null; +} export function isReviewDiffPanelId(panelId: string): boolean { return panelId === REVIEW_DIFF_PANEL_ID; diff --git a/packages/review-editor/hooks/useAnnotationToolbar.ts b/packages/review-editor/hooks/useAnnotationToolbar.ts index 72ce8b7d2..8f869c99a 100644 --- a/packages/review-editor/hooks/useAnnotationToolbar.ts +++ b/packages/review-editor/hooks/useAnnotationToolbar.ts @@ -6,7 +6,7 @@ import { shouldUseExpandedComposer, useVisibleViewportBounds, } from '@plannotator/ui/hooks/useViewportEnvironment'; -import { extractLinesFromPatch } from '../utils/patchParser'; +import { resolveAnnotationSnippet } from '../utils/patchParser'; import type { DiffTokenEventBaseProps } from '@pierre/diffs'; export interface TokenMeta { @@ -30,10 +30,16 @@ export interface ToolbarState { interface UseAnnotationToolbarArgs { patch: string; + /** + * Whole new-side file contents, when the hosting surface has them. Lets a + * range with no hunk coverage (expanded context, or any line of the + * full-file viewer) still carry real code instead of an empty snippet. + */ + fileContent?: string; filePath: string; isFocused: boolean; onLineSelection: (range: SelectedLineRange | null) => void; - onAddAnnotation: (type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta) => void; + onAddAnnotation: (type: CodeAnnotationType, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel, decorations?: ConventionalDecoration[], tokenMeta?: TokenAnnotationMeta, selectionSnippet?: string) => void; onEditAnnotation: (id: string, text?: string, suggestedCode?: string, originalCode?: string, conventionalLabel?: ConventionalLabel | null, decorations?: ConventionalDecoration[]) => void; } @@ -58,7 +64,7 @@ function draftKey(filePath: string, range: SelectedLineRange): string { return `${filePath}:${range.side}:${start}-${end}`; } -export function useAnnotationToolbar({ patch, filePath, isFocused, onLineSelection, onAddAnnotation, onEditAnnotation }: UseAnnotationToolbarArgs) { +export function useAnnotationToolbar({ patch, fileContent, filePath, isFocused, onLineSelection, onAddAnnotation, onEditAnnotation }: UseAnnotationToolbarArgs) { const visibleBounds = useVisibleViewportBounds(16); const expandedComposerRequired = shouldUseExpandedComposer({ bounds: visibleBounds, @@ -188,10 +194,10 @@ export function useAnnotationToolbar({ patch, filePath, isFocused, onLineSelecti const side = range.side === 'additions' ? 'new' : 'old'; const start = Math.min(range.start, range.end); const end = Math.max(range.start, range.end); - setSelectedOriginalCode(extractLinesFromPatch(patch, start, end, side as 'old' | 'new')); + setSelectedOriginalCode(resolveAnnotationSnippet(patch, fileContent, start, end, side as 'old' | 'new')); onLineSelection(range); - }, [expandedComposerRequired, patch, filePath, onLineSelection, saveDraft]); + }, [expandedComposerRequired, patch, fileContent, filePath, onLineSelection, saveDraft]); // Handle line selection end (gutter clicks) const handleLineSelectionEnd = useCallback((range: SelectedLineRange | null) => { @@ -251,6 +257,11 @@ export function useAnnotationToolbar({ patch, filePath, isFocused, onLineSelecti conventionalLabel ?? undefined, decorations.length > 0 ? decorations : undefined, tokenMeta, + // The selected lines, ALWAYS — independent of `original`, which stays + // suggestion-only so in-diff export is unchanged. The consumer + // attaches it only when the lines are outside the diff, where the + // agent has no other way to see them. + selectedOriginalCode || undefined, ); } @@ -352,10 +363,10 @@ export function useAnnotationToolbar({ patch, filePath, isFocused, onLineSelecti const side = draft.range.side === 'additions' ? 'new' : 'old'; const start = Math.min(draft.range.start, draft.range.end); const end = Math.max(draft.range.start, draft.range.end); - setSelectedOriginalCode(extractLinesFromPatch(patch, start, end, side as 'old' | 'new')); + setSelectedOriginalCode(resolveAnnotationSnippet(patch, fileContent, start, end, side as 'old' | 'new')); onLineSelection(draft.range); } - }, [expandedComposerRequired, filePath, isFocused, onLineSelection, patch]); + }, [expandedComposerRequired, fileContent, filePath, isFocused, onLineSelection, patch]); // Handle single token click — opens toolbar for one token const handleTokenClick = useCallback((props: DiffTokenEventBaseProps, event: MouseEvent) => { diff --git a/packages/review-editor/utils/exportFeedback.ts b/packages/review-editor/utils/exportFeedback.ts index c5ac24067..2f388aa15 100644 --- a/packages/review-editor/utils/exportFeedback.ts +++ b/packages/review-editor/utils/exportFeedback.ts @@ -188,9 +188,12 @@ function formatFileAnnotations(fileAnnotations: CodeAnnotation[], headingLevel = const tokenSuffix = ann.tokenText ? ` — \`\`${ann.tokenText.replace(/`/g, '\\`')}\`\`${ann.charStart != null ? ` (chars ${ann.charStart}-${ann.charEnd})` : ''}` : ''; - output += `${headingLevel} ${lineRange} (${ann.side})${tokenSuffix}\n`; + // Bracketed, matching the design doc's proposed "[Outside diff]" shape. + const outsideSuffix = ann.outsideDiff ? ' [Outside diff]' : ''; + output += `${headingLevel} ${lineRange} (${ann.side})${tokenSuffix}${outsideSuffix}\n`; output += commitMismatchNote(ann, commitShaFromMode(currentDiff?.mode)); output += gitButlerMismatchNote(ann, currentDiff); + output += outsideDiffNote(ann); if (ann.text) { output += `${prefix}${ann.text}\n`; @@ -202,6 +205,7 @@ function formatFileAnnotations(fileAnnotations: CodeAnnotation[], headingLevel = } output += formatCallFlowAnnotationTargets(ann); output += formatSelectedTextBlock(ann); + output += formatOutsideDiffCode(ann); output += formatSuggestionBlocks(ann); output += '\n'; } @@ -209,6 +213,35 @@ function formatFileAnnotations(fileAnnotations: CodeAnnotation[], headingLevel = return output; } +/** + * Tells the agent that an annotation's lines are not in the patch it was + * given, so it stops hunting for them there. + * + * Without this an out-of-diff line comment reads exactly like an in-diff one, + * and an agent asked to "address the review" looks up `src/foo.ts:500` in a + * diff whose only hunk is at line 3. + */ +function outsideDiffNote(ann: CodeAnnotation): string { + if (!ann.outsideDiff) return ''; + return `_These lines are not part of the diff under review. The file content at review time is quoted below._\n`; +} + +/** + * The code an out-of-diff annotation points at. + * + * The agent cannot recover these lines from the patch, so the comment is + * close to useless without them. Suppressed when a suggestion is present, + * because `formatSuggestionBlocks` already prints the same lines under + * "Replaces:" and printing them twice invites the agent to apply them twice. + */ +function formatOutsideDiffCode(ann: CodeAnnotation): string { + if (!ann.outsideDiff || !ann.originalCode) return ''; + // A suggestion still prints these lines under "Replaces:", which is the + // right label there — don't print them twice. + if (ann.suggestedCode) return ''; + return `\n**Code at these lines:**\n\`\`\`\n${ann.originalCode}\n\`\`\`\n`; +} + /** * The highlighted-text payload for a comment created inside an edit session: * the exact text the reviewer had selected in the editor. When the selection @@ -239,6 +272,9 @@ function formatSelectedTextBlock(ann: CodeAnnotation): string { */ function formatSuggestionBlocks(ann: CodeAnnotation): string { let output = ''; + // An out-of-diff comment with no suggestion replaces nothing — its lines + // are printed by formatOutsideDiffCode under an honest heading instead. + if (ann.outsideDiff && !ann.suggestedCode) return output; if ((ann.suggestedCode || ann.text) && ann.originalCode) { output += `\n**Replaces:**\n\`\`\`\n${ann.originalCode}\n\`\`\`\n`; } diff --git a/packages/review-editor/utils/outsideDiffAnnotation.test.ts b/packages/review-editor/utils/outsideDiffAnnotation.test.ts new file mode 100644 index 000000000..577a76ce8 --- /dev/null +++ b/packages/review-editor/utils/outsideDiffAnnotation.test.ts @@ -0,0 +1,154 @@ +/** + * Out-of-diff annotation round-trip: snippet resolution → export. + * + * The regression these guard is the one the design doc names as the single + * integration fix full-file review needs. `extractLinesFromPatch` only walks + * hunk lines, so before this an annotation on expanded context — or on any + * line of a full-file view — reached the agent with an EMPTY snippet and no + * indication its lines were absent from the patch. The agent then hunted for + * `file.ts:500` in a diff whose only hunk is at line 3. + */ +import { describe, expect, it } from 'bun:test'; +import { + extractLinesFromContent, + extractLinesFromPatch, + resolveAnnotationSnippet, +} from './patchParser'; +import { exportReviewFeedback } from './exportFeedback'; +import type { CodeAnnotation } from '@plannotator/ui/types'; + +// A patch touching only lines 1-3 of a much longer file. +const PATCH = [ + 'diff --git a/src/alpha.ts b/src/alpha.ts', + '--- a/src/alpha.ts', + '+++ b/src/alpha.ts', + '@@ -1,3 +1,3 @@', + '-const a = 0;', + '+const a = 1;', + ' const b = 2;', + ' const c = 3;', +].join('\n'); + +const FILE_CONTENT = [ + 'const a = 1;', // 1 + 'const b = 2;', // 2 + 'const c = 3;', // 3 + 'const d = 4;', // 4 + 'const e = 5;', // 5 + 'const f = 6;', // 6 +].join('\n'); + +const ann = (overrides: Partial = {}): CodeAnnotation => ({ + id: '1', + type: 'comment', + filePath: 'src/alpha.ts', + lineStart: 5, + lineEnd: 6, + side: 'new', + text: 'This helper is dead code', + createdAt: 1, + ...overrides, +}); + +describe('extractLinesFromContent', () => { + it('slices a 1-based inclusive range', () => { + expect(extractLinesFromContent(FILE_CONTENT, 5, 6)).toBe('const e = 5;\nconst f = 6;'); + expect(extractLinesFromContent(FILE_CONTENT, 1, 1)).toBe('const a = 1;'); + }); + + it('returns empty for nonsense ranges rather than throwing', () => { + expect(extractLinesFromContent(FILE_CONTENT, 0, 2)).toBe(''); + expect(extractLinesFromContent(FILE_CONTENT, 4, 2)).toBe(''); + expect(extractLinesFromContent('', 1, 2)).toBe(''); + }); +}); + +describe('resolveAnnotationSnippet', () => { + it('returns nothing from the patch alone for lines outside every hunk', () => { + // This is the pre-existing bug, pinned so the fallback below is meaningful. + expect(extractLinesFromPatch(PATCH, 5, 6, 'new')).toBe(''); + }); + + it('falls back to file content for lines outside every hunk', () => { + expect(resolveAnnotationSnippet(PATCH, FILE_CONTENT, 5, 6, 'new')).toBe( + 'const e = 5;\nconst f = 6;', + ); + }); + + it('still prefers the patch for lines inside a hunk', () => { + // The patch is the authority where it has an answer: it distinguishes + // sides, which raw file content cannot. + expect(resolveAnnotationSnippet(PATCH, FILE_CONTENT, 1, 1, 'new')).toBe('const a = 1;'); + expect(resolveAnnotationSnippet(PATCH, FILE_CONTENT, 1, 1, 'old')).toBe('const a = 0;'); + }); + + it('works with no patch at all (a file absent from the diff)', () => { + expect(resolveAnnotationSnippet('', FILE_CONTENT, 2, 3, 'new')).toBe( + 'const b = 2;\nconst c = 3;', + ); + }); + + it('never quotes working-tree content for an old-side range', () => { + // The working tree is the NEW side; using it for an old-side range would + // quote text that never existed at the base. + expect(resolveAnnotationSnippet(PATCH, FILE_CONTENT, 5, 6, 'old')).toBe(''); + }); +}); + +describe('exportReviewFeedback with out-of-diff annotations', () => { + it('labels the annotation and fences the code the patch does not contain', () => { + const output = exportReviewFeedback([ + ann({ outsideDiff: true, originalCode: 'const e = 5;\nconst f = 6;' }), + ]); + + expect(output).toContain('src/alpha.ts'); + expect(output).toContain('Lines 5-6'); + // The label — deliberate wording so the agent stops looking in the patch. + expect(output).toContain('Outside diff'); + expect(output).toContain('not part of the diff under review'); + // The code itself, fenced. + expect(output).toContain('**Code at these lines:**'); + expect(output).toContain('const e = 5;\nconst f = 6;'); + // Not mislabeled as a suggestion: nothing is being replaced. + expect(output).not.toContain('**Replaces:**'); + }); + + it('leaves ordinary in-diff annotations untouched', () => { + const output = exportReviewFeedback([ann({ lineStart: 1, lineEnd: 1 })]); + expect(output).not.toContain('Outside diff'); + expect(output).not.toContain('**Code at these lines:**'); + }); + + it('keeps the Replaces block when an out-of-diff comment carries a suggestion', () => { + // A suggestion still needs its verifiable anchor, and printing the same + // lines twice would invite the agent to apply them twice. + const output = exportReviewFeedback([ + ann({ + outsideDiff: true, + originalCode: 'const e = 5;', + suggestedCode: 'const e = 50;', + }), + ]); + expect(output).toContain('Outside diff'); + expect(output).toContain('**Replaces:**'); + expect(output).toContain('**Suggested code:**'); + expect(output).not.toContain('**Code at these lines:**'); + }); + + it('carries the label through a mixed in-diff and out-of-diff review', () => { + const output = exportReviewFeedback([ + ann({ id: 'a', lineStart: 1, lineEnd: 1, text: 'in the diff' }), + ann({ + id: 'b', + lineStart: 5, + lineEnd: 6, + text: 'outside the diff', + outsideDiff: true, + originalCode: 'const e = 5;\nconst f = 6;', + }), + ]); + expect(output).toContain('in the diff'); + expect(output).toContain('outside the diff'); + expect(output.match(/Outside diff/g)?.length).toBe(1); + }); +}); diff --git a/packages/review-editor/utils/patchParser.ts b/packages/review-editor/utils/patchParser.ts index 1d39193e7..4e83914b4 100644 --- a/packages/review-editor/utils/patchParser.ts +++ b/packages/review-editor/utils/patchParser.ts @@ -77,3 +77,51 @@ export function isLineRangeInPatch( } return false; } + +/** + * Slice a 1-based, inclusive line range out of whole file contents. + * + * The patch-free counterpart of `extractLinesFromPatch`, for surfaces that + * hold the file rather than a diff of it (the full-file viewer) and for lines + * that exist in the file but not in any hunk (expanded diff context). + */ +export function extractLinesFromContent( + content: string, + lineStart: number, + lineEnd: number, +): string { + if (!content) return ''; + if (!Number.isInteger(lineStart) || !Number.isInteger(lineEnd)) return ''; + if (lineStart < 1 || lineEnd < lineStart) return ''; + return content + .split('\n') + .slice(lineStart - 1, lineEnd) + .join('\n'); +} + +/** + * The snippet an annotation should carry, preferring the patch and falling + * back to file contents. + * + * Why the fallback exists: `extractLinesFromPatch` only walks hunk lines, so + * annotating expanded context (or any line of a full-file view) produced an + * EMPTY `originalCode`. The annotation then reached the agent as a bare line + * number with no code attached, which is exactly the case where the agent + * most needs the code — the lines are not in the diff it was given. + * + * Only the new side falls back: `fileContent` is the working tree, so using + * it for an old-side range would quote the wrong text. An old-side range with + * no hunk coverage correctly yields nothing. + */ +export function resolveAnnotationSnippet( + patch: string, + fileContent: string | undefined, + lineStart: number, + lineEnd: number, + side: 'old' | 'new', +): string { + const fromPatch = patch ? extractLinesFromPatch(patch, lineStart, lineEnd, side) : ''; + if (fromPatch) return fromPatch; + if (side !== 'new' || !fileContent) return fromPatch; + return extractLinesFromContent(fileContent, lineStart, lineEnd); +} diff --git a/packages/server/review-file-endpoint.test.ts b/packages/server/review-file-endpoint.test.ts new file mode 100644 index 000000000..21a61c75d --- /dev/null +++ b/packages/server/review-file-endpoint.test.ts @@ -0,0 +1,269 @@ +/** + * GET /api/review-file — full-file serving for the code-review file viewer, + * plus the hardening retrofit on GET /api/code-nav/file. Dual-runtime. + * + * What can regress here: + * 1. The endpoint hands out a file it must not. The review side's only + * traversal defense was a lexical `..`/leading-slash check, which a + * symlink walks straight past. These tests put a real escaping symlink in + * a real repo and assert the bytes never come back. + * 2. The 5 MiB cap stops applying, making one request a memory bomb. + * 3. Bun and Pi drift — every case runs against both servers. + * 4. The code-nav retrofit gets reverted. `/api/code-nav/file` had NO size + * cap and the same lexical-only guard; the last block asserts it now + * answers like the new endpoint. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { createServer } from 'node:net'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { startReviewServer as startBunReviewServer } from './review'; +import { startReviewServer as startPiReviewServer } from '../../apps/pi-extension/server'; +import { getVcsContext } from './vcs'; +import { MAX_REPO_FILE_BYTES } from '@plannotator/shared/repo-file'; + +const originalDataDir = process.env.PLANNOTATOR_DATA_DIR; +const originalPort = process.env.PLANNOTATOR_PORT; +const tempDirs: string[] = []; + +function makeTempDir(prefix: string): string { + // realpath so containment comparisons see the same canonical prefix the + // server does (/tmp is a symlink to /private/tmp on macOS). + const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix))); + tempDirs.push(dir); + return dir; +} + +function git(cwd: string, args: string[]): void { + const result = spawnSync('git', args, { cwd, encoding: 'utf-8' }); + if (result.status !== 0) { + throw new Error(result.stderr || `git ${args.join(' ')} failed`); + } +} + +const SECRET = 'SUPER-SECRET-OUTSIDE-THE-REPO'; + +/** + * A repo with the shapes that matter: an ordinary file, a symlink that leaves + * the repo, a directory symlink that leaves it, and a symlink that stays in. + */ +function initRepo(): { repoDir: string; outsideDir: string } { + const base = makeTempDir('plannotator-review-file-'); + const repoDir = join(base, 'repo'); + const outsideDir = join(base, 'outside'); + mkdirSync(join(repoDir, 'src'), { recursive: true }); + mkdirSync(outsideDir, { recursive: true }); + + writeFileSync(join(outsideDir, 'secret.txt'), `${SECRET}\n`); + writeFileSync(join(repoDir, 'src', 'app.ts'), 'export const app = 1;\n'); + writeFileSync(join(repoDir, 'README.md'), '# repo\n'); + symlinkSync(join(outsideDir, 'secret.txt'), join(repoDir, 'escape.txt')); + symlinkSync(outsideDir, join(repoDir, 'escape-dir')); + symlinkSync(join(repoDir, 'src', 'app.ts'), join(repoDir, 'inside-link.ts')); + + git(repoDir, ['init', '-q']); + git(repoDir, ['branch', '-M', 'main']); + git(repoDir, ['config', 'user.email', 'test@example.com']); + git(repoDir, ['config', 'user.name', 'Test']); + git(repoDir, ['add', '-A']); + git(repoDir, ['commit', '-q', '-m', 'initial']); + return { repoDir, outsideDir }; +} + +async function reservePort(): Promise { + const server = createServer(); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve()); + }); + const address = server.address(); + const port = typeof address === 'object' && address ? address.port : 0; + await new Promise((resolve) => server.close(() => resolve())); + return port; +} + +const RAW_PATCH = [ + 'diff --git a/src/app.ts b/src/app.ts', + '--- a/src/app.ts', + '+++ b/src/app.ts', + '@@ -1 +1 @@', + '-export const app = 0;', + '+export const app = 1;', +].join('\n'); + +afterEach(() => { + if (originalDataDir === undefined) delete process.env.PLANNOTATOR_DATA_DIR; + else process.env.PLANNOTATOR_DATA_DIR = originalDataDir; + if (originalPort === undefined) delete process.env.PLANNOTATOR_PORT; + else process.env.PLANNOTATOR_PORT = originalPort; + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe('GET /api/review-file', () => { + for (const [runtime, startServer] of [ + ['Bun', startBunReviewServer], + ['Pi', startPiReviewServer], + ] as const) { + async function withServer( + repoDir: string, + body: (url: string) => Promise, + ): Promise { + process.env.PLANNOTATOR_DATA_DIR = makeTempDir('plannotator-review-file-data-'); + if (runtime === 'Pi') process.env.PLANNOTATOR_PORT = String(await reservePort()); + const gitContext = await getVcsContext(repoDir, 'git'); + const server = await startServer({ + rawPatch: RAW_PATCH, + gitRef: 'Working tree', + diffType: 'uncommitted', + gitContext, + agentCwd: repoDir, + origin: runtime === 'Pi' ? 'pi' : 'claude-code', + htmlContent: 'review', + }); + try { + await body(server.url); + } finally { + server.stop(); + } + } + + test(`${runtime} serves a file from the review root`, async () => { + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/review-file?path=src/app.ts`); + expect(response.status).toBe(200); + const data = (await response.json()) as { + content: string; + filePath: string; + size: number; + }; + expect(data.content).toBe('export const app = 1;\n'); + expect(data.filePath).toBe('src/app.ts'); + expect(data.size).toBe('export const app = 1;\n'.length); + }); + }); + + test(`${runtime} serves a file that is not in the diff at all`, async () => { + // The whole point of the feature: open a file the patch never mentions. + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/review-file?path=README.md`); + expect(response.status).toBe(200); + expect(((await response.json()) as { content: string }).content).toBe('# repo\n'); + }); + }); + + test(`${runtime} rejects path traversal`, async () => { + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + for (const candidate of [ + '../outside/secret.txt', + 'src/../../outside/secret.txt', + '/etc/passwd', + ]) { + const response = await fetch( + `${url}/api/review-file?path=${encodeURIComponent(candidate)}`, + ); + expect(response.status).toBe(400); + expect(await response.text()).not.toContain(SECRET); + } + }); + }); + + test(`${runtime} refuses a symlink that escapes the review root`, async () => { + // Lexically clean (no ".."), so the old validateFilePath allowed it. + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + for (const candidate of ['escape.txt', 'escape-dir/secret.txt']) { + const response = await fetch( + `${url}/api/review-file?path=${encodeURIComponent(candidate)}`, + ); + expect(response.status).toBe(403); + expect(await response.text()).not.toContain(SECRET); + } + }); + }); + + test(`${runtime} still follows a symlink that stays inside the root`, async () => { + // Containment must not be so blunt that ordinary in-repo links break. + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/review-file?path=inside-link.ts`); + expect(response.status).toBe(200); + expect(((await response.json()) as { content: string }).content).toBe( + 'export const app = 1;\n', + ); + }); + }); + + test(`${runtime} answers 404 for a missing file and 400 for a directory`, async () => { + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + expect((await fetch(`${url}/api/review-file?path=src/nope.ts`)).status).toBe(404); + expect((await fetch(`${url}/api/review-file?path=src`)).status).toBe(400); + expect((await fetch(`${url}/api/review-file`)).status).toBe(400); + }); + }); + + test(`${runtime} caps the response at ${MAX_REPO_FILE_BYTES} bytes`, async () => { + const { repoDir } = initRepo(); + writeFileSync( + join(repoDir, 'huge.txt'), + Buffer.alloc(MAX_REPO_FILE_BYTES + 1, 0x61), + ); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/review-file?path=huge.txt`); + expect(response.status).toBe(413); + const data = (await response.json()) as { reason: string; size: number }; + expect(data.reason).toBe('too-large'); + expect(data.size).toBe(MAX_REPO_FILE_BYTES + 1); + }); + }); + + // --- The retrofit ----------------------------------------------------- + // /api/code-nav/file shipped with no size cap and the same lexical-only + // guard. It must now behave exactly like the new endpoint. + + test(`${runtime} /api/code-nav/file refuses an escaping symlink`, async () => { + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/code-nav/file?path=escape.txt`); + expect(response.status).toBe(403); + expect(await response.text()).not.toContain(SECRET); + }); + }); + + test(`${runtime} /api/code-nav/file now enforces the size cap`, async () => { + const { repoDir } = initRepo(); + writeFileSync( + join(repoDir, 'huge.txt'), + Buffer.alloc(MAX_REPO_FILE_BYTES + 1, 0x61), + ); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/code-nav/file?path=huge.txt`); + expect(response.status).toBe(413); + }); + }); + + test(`${runtime} /api/code-nav/file still serves ordinary files`, async () => { + // The retrofit must not break the peek preview it guards. + const { repoDir } = initRepo(); + await withServer(repoDir, async (url) => { + const response = await fetch(`${url}/api/code-nav/file?path=src/app.ts`); + expect(response.status).toBe(200); + expect(((await response.json()) as { content: string }).content).toBe( + 'export const app = 1;\n', + ); + }); + }); + } +}); diff --git a/packages/server/review.ts b/packages/server/review.ts index f504a23de..43c79e098 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -65,6 +65,7 @@ import { import { type AgentJobInfo, REVIEW_OUTPUT_FAILED, getAgentJobAnnotationContext, markJobReviewFailed } from "@plannotator/shared/agent-jobs"; import { createCommitAvatarResolver } from "@plannotator/shared/commit-avatars"; import { detectGeneratedFiles, detectGeneratedFilesByName } from "@plannotator/shared/generated-files"; +import { REPO_FILE_ERROR_STATUS, readRepoFile } from "@plannotator/shared/repo-file"; import { getRepoInfo } from "./repo"; import { handleImage, handleUpload, handleAgents, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleApiNotFound, handleFavicon, readDraftGenerationFromBody, readDraftGenerationFromUrl, type OpencodeClient } from "./shared-handlers"; import { contentHash, deleteDraft } from "./draft"; @@ -804,6 +805,39 @@ export async function startReviewServer( } return resolveAgentCwd(); }; + /** + * Local working-tree root for the surfaces that serve whole files: + * /api/code-nav/file and /api/review-file. Both need the identical gate + * (no committed GitButler views, local access required, PR checkouts must + * actually be warm), so it lives in one place rather than being copied and + * drifting. `surface` only picks the user-facing noun in the error text. + */ + const resolveLocalFileRoot = async ( + surface: "Code navigation" | "File viewing", + ): Promise< + { ok: true; root: string } | { ok: false; error: string; status: number } + > => { + if (isGitButlerCommittedView()) { + return { + ok: false, + status: 400, + error: `${surface} is unavailable for committed GitButler views`, + }; + } + const hasAccess = !!workspace || !!gitContext || !!options.agentCwd || !!options.worktreePool; + if (!hasAccess) { + return { ok: false, status: 400, error: `${surface} requires local access` }; + } + // PR mode: the checkout must actually exist — reading through a fallback + // directory returns confidently-wrong file contents. + const root = options.worktreePool && prMetadata + ? await ensurePRLocalCwd() + : await resolveAgentCwdReady(); + if (!root) { + return { ok: false, status: 400, error: "Local checkout unavailable" }; + } + return { ok: true, root }; + }; const getWorkspacePromptContext = (): WorkspaceReviewPromptContext | undefined => { if (!workspace) return undefined; return workspace.getPromptContext(); @@ -2985,36 +3019,48 @@ export async function startReviewServer( } // API: Code navigation file preview (read file from working tree) + // + // Hardened to go through readRepoFile: this route previously read + // `${navCwd}/${filePath}` behind nothing but the lexical + // validateFilePath, with NO size cap at all. if (url.pathname === "/api/code-nav/file" && req.method === "GET") { - if (isGitButlerCommittedView()) { + const rootResult = await resolveLocalFileRoot("Code navigation"); + if (!rootResult.ok) { + return Response.json({ error: rootResult.error }, { status: rootResult.status }); + } + const result = readRepoFile(rootResult.root, url.searchParams.get("path")); + if (!result.ok) { return Response.json( - { error: "Code navigation is unavailable for committed GitButler views" }, - { status: 400 }, + { error: result.message }, + { status: REPO_FILE_ERROR_STATUS[result.reason] }, ); } - const hasCodeNavAccess = !!workspace || !!gitContext || !!options.agentCwd || !!options.worktreePool; - if (!hasCodeNavAccess) { - return Response.json({ error: "Code navigation requires local access" }, { status: 400 }); - } - const filePath = url.searchParams.get("path"); - if (!filePath) { - return Response.json({ error: "Missing path" }, { status: 400 }); - } - try { validateFilePath(filePath); } catch { - return Response.json({ error: "Invalid path" }, { status: 400 }); - } - try { - const navCwd = options.worktreePool && prMetadata - ? await ensurePRLocalCwd() - : await resolveAgentCwdReady(); - if (!navCwd) { - return Response.json({ error: "Local checkout unavailable" }, { status: 400 }); - } - const content = await Bun.file(`${navCwd}/${filePath}`).text(); - return Response.json({ content }); - } catch { - return Response.json({ error: "File not found" }, { status: 404 }); + return Response.json({ content: result.content }); + } + + // API: Full file content for the full-file review viewer. + // + // Deliberately NOT snapshot-guarded: this serves the live working + // tree (the "new" side), same as the code-nav preview. The diff has + // its own staleness notice; a file panel showing the file as it is + // right now is the correct behavior for "open the file". + if (url.pathname === "/api/review-file" && req.method === "GET") { + const rootResult = await resolveLocalFileRoot("File viewing"); + if (!rootResult.ok) { + return Response.json({ error: rootResult.error }, { status: rootResult.status }); + } + const result = readRepoFile(rootResult.root, url.searchParams.get("path")); + if (!result.ok) { + return Response.json( + { error: result.message, reason: result.reason, size: result.size }, + { status: REPO_FILE_ERROR_STATUS[result.reason] }, + ); } + return Response.json({ + filePath: result.filePath, + content: result.content, + size: result.size, + }); } // API: Stage / unstage a file (disabled when VCS doesn't support it) diff --git a/packages/shared/package.json b/packages/shared/package.json index a2c0885e6..e721e9b9c 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -60,6 +60,7 @@ "./annotate-target": "./annotate-target.ts", "./at-reference": "./at-reference.ts", "./code-nav": "./code-nav.ts", + "./repo-file": "./repo-file.ts", "./goal-setup": "./goal-setup.ts", "./semantic-diff": "./semantic-diff.ts", "./semantic-diff-types": "./semantic-diff-types.ts", diff --git a/packages/shared/repo-file.test.ts b/packages/shared/repo-file.test.ts new file mode 100644 index 000000000..e6b117fcb --- /dev/null +++ b/packages/shared/repo-file.test.ts @@ -0,0 +1,229 @@ +import { describe, expect, test, beforeAll, afterAll } from "bun:test"; +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + MAX_REPO_FILE_BYTES, + isContainedPath, + readRepoFile, + resolveRepoFilePath, + validateRepoFilePath, +} from "./repo-file"; + +/** + * Every test here guards a way the review side could hand out a file it must + * not, or read one it cannot afford. The lexical `validateFilePath` these + * replace passed the symlink cases below. + */ + +let root = ""; +let outside = ""; + +beforeAll(() => { + // realpath the temp dir: on macOS /tmp is itself a symlink to /private/tmp, + // so an un-resolved root would make every containment check fail. + const base = realpathSync(mkdtempSync(join(tmpdir(), "pn-repo-file-"))); + root = join(base, "repo"); + outside = join(base, "outside"); + mkdirSync(join(root, "src"), { recursive: true }); + mkdirSync(outside, { recursive: true }); + + writeFileSync(join(root, "src", "alpha.ts"), "export const a = 1;\n"); + writeFileSync(join(root, "README.md"), "# repo\n"); + writeFileSync(join(outside, "secret.txt"), "TOP SECRET\n"); + + // A symlink whose destination escapes the root. Lexically it is a perfectly + // ordinary relative path with no "..". + symlinkSync(join(outside, "secret.txt"), join(root, "escape.txt")); + // A directory symlink pointing out, so "escape-dir/secret.txt" also has no + // ".." anywhere in it. + symlinkSync(outside, join(root, "escape-dir")); + // A symlink that stays inside: this one must keep working. + symlinkSync(join(root, "src", "alpha.ts"), join(root, "inside-link.ts")); + // A dangling symlink. + symlinkSync(join(outside, "nope.txt"), join(root, "dangling.txt")); +}); + +afterAll(() => { + if (root) rmSync(join(root, ".."), { recursive: true, force: true }); +}); + +describe("validateRepoFilePath", () => { + test("accepts and normalizes an ordinary relative path", () => { + const result = validateRepoFilePath("./src//alpha.ts"); + expect(result.ok).toBe(true); + if (result.ok) expect(result.path).toBe("src/alpha.ts"); + }); + + test("rejects absolute paths", () => { + const result = validateRepoFilePath("/etc/passwd"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("invalid-path"); + }); + + test("rejects parent-directory segments", () => { + for (const candidate of ["../secret", "src/../../secret", "a/b/.."]) { + const result = validateRepoFilePath(candidate); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("invalid-path"); + } + }); + + test("rejects backslash-spelled parent segments", () => { + // Guards the normalization step: a Windows-style path must not smuggle + // a ".." past a check that only splits on "/". + const result = validateRepoFilePath("..\\secret"); + expect(result.ok).toBe(false); + }); + + test("does not treat a filename merely containing dots as traversal", () => { + // The old substring check rejected this legitimate filename. + const result = validateRepoFilePath("src/foo..bar.ts"); + expect(result.ok).toBe(true); + if (result.ok) expect(result.path).toBe("src/foo..bar.ts"); + }); + + test("rejects NUL bytes and empty input", () => { + expect(validateRepoFilePath("src/a\0.ts").ok).toBe(false); + expect(validateRepoFilePath("").ok).toBe(false); + expect(validateRepoFilePath(undefined).ok).toBe(false); + expect(validateRepoFilePath(".").ok).toBe(false); + }); + + test("rejects Windows drive-qualified paths", () => { + expect(validateRepoFilePath("C:/Windows/win.ini").ok).toBe(false); + }); +}); + +describe("isContainedPath", () => { + test("a sibling sharing a name prefix is not contained", () => { + // The bug a naive startsWith() containment check has. + expect(isContainedPath("/repo-evil/x", "/repo")).toBe(false); + expect(isContainedPath("/repo/x", "/repo")).toBe(true); + expect(isContainedPath("/repo", "/repo")).toBe(true); + }); +}); + +describe("readRepoFile containment", () => { + test("reads a file inside the root", () => { + const result = readRepoFile(root, "src/alpha.ts"); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.content).toBe("export const a = 1;\n"); + expect(result.filePath).toBe("src/alpha.ts"); + expect(result.size).toBe("export const a = 1;\n".length); + } + }); + + test("refuses a symlink that escapes the root", () => { + // The headline case: no ".." anywhere, lexically clean, still denied. + const result = readRepoFile(root, "escape.txt"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("outside-root"); + }); + + test("refuses a path through a directory symlink that escapes", () => { + const result = readRepoFile(root, "escape-dir/secret.txt"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("outside-root"); + }); + + test("never returns content from outside the root", () => { + for (const candidate of ["escape.txt", "escape-dir/secret.txt"]) { + const result = readRepoFile(root, candidate); + const body = result.ok ? result.content : ""; + expect(body).not.toContain("TOP SECRET"); + } + }); + + test("allows a symlink that stays inside the root", () => { + const result = readRepoFile(root, "inside-link.ts"); + expect(result.ok).toBe(true); + if (result.ok) expect(result.content).toBe("export const a = 1;\n"); + }); + + test("reports a dangling symlink as not-found, not as an escape", () => { + const result = readRepoFile(root, "dangling.txt"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("not-found"); + }); + + test("rejects traversal attempts before touching the filesystem", () => { + for (const candidate of [ + "../outside/secret.txt", + "src/../../outside/secret.txt", + "/etc/passwd", + ]) { + const result = readRepoFile(root, candidate); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("invalid-path"); + } + }); + + test("reports a directory as not-a-file", () => { + const result = readRepoFile(root, "src"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("not-a-file"); + }); + + test("reports a missing file as not-found", () => { + const result = readRepoFile(root, "src/nope.ts"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("not-found"); + }); +}); + +describe("readRepoFile size cap", () => { + test("refuses a file over the cap without reading it into memory", () => { + const big = join(root, "big.bin"); + // One byte over the cap is the boundary that matters. + writeFileSync(big, Buffer.alloc(MAX_REPO_FILE_BYTES + 1, 0x61)); + try { + const result = readRepoFile(root, "big.bin"); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.reason).toBe("too-large"); + expect(result.size).toBe(MAX_REPO_FILE_BYTES + 1); + } + } finally { + rmSync(big, { force: true }); + } + }); + + test("accepts a file exactly at the cap", () => { + const atCap = join(root, "atcap.bin"); + writeFileSync(atCap, Buffer.alloc(MAX_REPO_FILE_BYTES, 0x62)); + try { + const result = readRepoFile(root, "atcap.bin"); + expect(result.ok).toBe(true); + if (result.ok) expect(result.size).toBe(MAX_REPO_FILE_BYTES); + } finally { + rmSync(atCap, { force: true }); + } + }); +}); + +describe("resolveRepoFilePath", () => { + test("returns the canonical path for an inside symlink", () => { + const result = resolveRepoFilePath(root, "inside-link.ts"); + expect(result.ok).toBe(true); + if (result.ok) { + // Canonical, i.e. the link is resolved to its target. + expect(result.absolutePath).toBe(realpathSync(join(root, "src", "alpha.ts"))); + } + }); + + test("fails closed when the root itself does not exist", () => { + const result = resolveRepoFilePath(join(root, "no-such-root"), "a.ts"); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("not-found"); + }); +}); diff --git a/packages/shared/repo-file.ts b/packages/shared/repo-file.ts new file mode 100644 index 000000000..3ef0657a8 --- /dev/null +++ b/packages/shared/repo-file.ts @@ -0,0 +1,235 @@ +/** + * Hardened repo file reading for the code-review surfaces. + * + * Runtime-agnostic (node:fs + node:path only) so both the Bun review server + * and the hand-mirrored Pi review server route every full-file read through + * exactly one guard. Vendored to Pi by `apps/pi-extension/vendor.sh`. + * + * Why this exists: the review side's entire traversal defense was + * `validateFilePath` in review-core.ts, a lexical check that rejects `..` + * substrings and leading `/`. Lexical checks cannot see a symlink. A repo + * containing `link -> /etc` passes every lexical test and then reads + * `link/passwd` straight out of the filesystem. `/api/code-nav/file` also had + * no size cap at all, so a multi-gigabyte file in the tree was a + * one-request memory bomb. + * + * The guard here is realpath containment: resolve the review root AND the + * candidate through the filesystem, then require the canonical candidate to + * live under the canonical root. That is symlink-proof by construction + * because realpath is what decides, not string shape. + */ + +import { readFileSync, realpathSync, statSync } from "node:fs"; +import { isAbsolute, resolve, sep } from "node:path"; + +/** + * Serve cap for full-file reads, deliberately identical to + * MAX_REVIEW_FILE_CONTENT_BYTES (packages/shared/review-core.ts:21) so a file + * that can be context-expanded in the diff can also be opened whole, and vice + * versa. Kept as its own named constant so this module stays importable by + * the Pi vendor copy without dragging in all of review-core. + */ +export const MAX_REPO_FILE_BYTES = 5 * 1024 * 1024; + +export type RepoFileFailureReason = + | "invalid-path" + | "outside-root" + | "not-found" + | "not-a-file" + | "too-large"; + +export interface RepoFileSuccess { + ok: true; + /** The request path, normalized to repo-relative POSIX form. */ + filePath: string; + /** Canonical on-disk path, after symlink resolution. */ + absolutePath: string; + content: string; + size: number; +} + +export interface RepoFileFailure { + ok: false; + reason: RepoFileFailureReason; + message: string; + /** Present on "too-large" so the client can render a real number. */ + size?: number; +} + +export type RepoFileResult = RepoFileSuccess | RepoFileFailure; + +/** HTTP status each failure maps to. Shared so both runtimes answer alike. */ +export const REPO_FILE_ERROR_STATUS: Record = { + "invalid-path": 400, + "outside-root": 403, + "not-found": 404, + "not-a-file": 400, + "too-large": 413, +}; + +function fail( + reason: RepoFileFailureReason, + message: string, + size?: number, +): RepoFileFailure { + return size === undefined + ? { ok: false, reason, message } + : { ok: false, reason, message, size }; +} + +/** + * Shape validation for a client-supplied repo-relative path. + * + * This is the cheap pre-filter, NOT the security boundary — containment is. + * It exists so obviously hostile input is rejected before it ever touches the + * filesystem, and so error messages stay honest ("invalid path" vs "not + * found", which would otherwise leak whether a path outside the repo exists). + */ +export function validateRepoFilePath( + rawPath: unknown, +): { ok: true; path: string } | RepoFileFailure { + if (typeof rawPath !== "string" || rawPath.length === 0) { + return fail("invalid-path", "Missing path"); + } + // A NUL truncates the path at the syscall boundary on some platforms. + if (rawPath.includes("\0")) { + return fail("invalid-path", "Invalid path"); + } + // Normalize Windows separators up front so the segment checks below see + // every segment, however the client spelled them. + const normalized = rawPath.replace(/\\/g, "/"); + if (isAbsolute(rawPath) || normalized.startsWith("/")) { + return fail("invalid-path", "Path must be relative to the review root"); + } + // Windows drive-qualified paths ("C:foo", "C:/foo") are absolute in intent + // even when node's isAbsolute disagrees on a POSIX host. + if (/^[a-zA-Z]:/.test(normalized)) { + return fail("invalid-path", "Path must be relative to the review root"); + } + const segments = normalized.split("/"); + if (segments.some((segment) => segment === "..")) { + return fail("invalid-path", "Invalid path"); + } + // Strip "." and empty segments so "./a//b" normalizes to "a/b". + const cleaned = segments.filter( + (segment) => segment.length > 0 && segment !== ".", + ); + if (cleaned.length === 0) { + return fail("invalid-path", "Invalid path"); + } + return { ok: true, path: cleaned.join("/") }; +} + +/** + * True when `candidate` is the canonical root itself or lives beneath it. + * + * Both inputs must already be realpath-resolved by the caller; comparing + * un-resolved paths is what makes lexical containment checks defeatable. + * The trailing-separator form is what stops the classic `/repo-evil` prefix + * match against root `/repo`. + */ +export function isContainedPath(candidate: string, root: string): boolean { + if (candidate === root) return true; + const rootWithSep = root.endsWith(sep) ? root : `${root}${sep}`; + return candidate.startsWith(rootWithSep); +} + +function realpathOrNull(target: string): string | null { + try { + return realpathSync(target); + } catch { + return null; + } +} + +/** + * Resolve a repo-relative request path to a canonical, contained absolute path + * without reading it. Split out from `readRepoFile` so callers that only need + * the location (existence probes, editor hand-off) share the same guard. + */ +export function resolveRepoFilePath( + root: string, + rawPath: unknown, +): + | { ok: true; filePath: string; absolutePath: string } + | RepoFileFailure { + const validated = validateRepoFilePath(rawPath); + if (!validated.ok) return validated; + + const canonicalRoot = realpathOrNull(root); + if (canonicalRoot === null) { + return fail("not-found", "Review root is unavailable"); + } + + const candidate = resolve(canonicalRoot, validated.path); + // Lexical pre-check. Redundant with the realpath check below, but it keeps + // a path that is out of bounds on its face from being stat'ed at all. + if (!isContainedPath(candidate, canonicalRoot)) { + return fail("outside-root", "Path is outside the review root"); + } + + const canonicalCandidate = realpathOrNull(candidate); + if (canonicalCandidate === null) { + // Covers both "does not exist" and "dangling symlink". Reported as + // not-found either way so the response never confirms what lives + // outside the repo. + return fail("not-found", "File not found"); + } + + // THE security boundary: the canonical, symlink-resolved destination must + // still be inside the canonical root. + if (!isContainedPath(canonicalCandidate, canonicalRoot)) { + return fail("outside-root", "Path is outside the review root"); + } + + return { + ok: true, + filePath: validated.path, + absolutePath: canonicalCandidate, + }; +} + +/** + * Read one file from the review working tree, contained and capped. + * + * The size check reads `stat` before `readFileSync` on purpose: checking after + * the read would mean the oversized file was already resident in memory, which + * is the thing the cap exists to prevent. + */ +export function readRepoFile(root: string, rawPath: unknown): RepoFileResult { + const resolved = resolveRepoFilePath(root, rawPath); + if (!resolved.ok) return resolved; + + let stats: ReturnType; + try { + stats = statSync(resolved.absolutePath); + } catch { + return fail("not-found", "File not found"); + } + + if (!stats.isFile()) { + return fail("not-a-file", "Path is not a file"); + } + if (stats.size > MAX_REPO_FILE_BYTES) { + return fail( + "too-large", + `File is too large to open (max ${MAX_REPO_FILE_BYTES} bytes)`, + stats.size, + ); + } + + let content: string; + try { + content = readFileSync(resolved.absolutePath, "utf8"); + } catch { + return fail("not-found", "File could not be read"); + } + + return { + ok: true, + filePath: resolved.filePath, + absolutePath: resolved.absolutePath, + content, + size: stats.size, + }; +} diff --git a/packages/ui/types.ts b/packages/ui/types.ts index f40c074af..73bd98a88 100644 --- a/packages/ui/types.ts +++ b/packages/ui/types.ts @@ -276,6 +276,15 @@ export interface CodeAnnotation { gitButlerBase?: string; /** Exact server snapshot that supplied the GitButler line coordinates. */ gitButlerSnapshotId?: string; + /** + * Set when the annotated lines are NOT in the diff the agent was given: + * authored in the full-file viewer, or on a changed file but outside every + * hunk. Stamped at creation (the surface knows; the exporter cannot), the + * way plan-diff annotations stamp `diffContext`. The export uses it to say + * so and to fence the code, because the agent has no other way to see + * lines the patch never contained. + */ + outsideDiff?: boolean; } /** Token-level metadata passed from selection to annotation creation. */