From d8b6f8cf3bc29be7c5f1887bb1d19dc664507350 Mon Sep 17 00:00:00 2001 From: Anso Date: Mon, 25 May 2026 01:38:04 -0400 Subject: [PATCH] feat(stack-files): force-text override for misidentified binary files (#1215) * feat(stack-files): force-text override for misidentified binary files The binary-detection heuristic (30% non-printable / NUL in the first 8 KB) sometimes flags UTF-8 files that happen to carry an embedded NUL or a high non-printable ratio, locking the user out of inline editing with only a Download fallback. readStackFile now accepts an optional { forceText: true } that bypasses isBinaryBuffer on the small-file path and returns the bytes as UTF-8 content. The route exposes this as ?force=text on GET /files/content. The oversized branch deliberately stays untouched: returning a multi-MB file as JSON-encoded text is wasteful regardless of the heuristic. SpecialFilePanel grows an optional extraAction slot. The viewer's binary branch wires Open as text anyway, which refetches with the new flag, clears isBinary, and routes the content through the existing Monaco editor path. A failed override surfaces both an inline error panel and a toast so the user knows why the click did nothing. Backend tests pin the heuristic-vs-override behaviour pair on a file with a literal NUL byte. The frontend test asserts that the second readStackFile call carries forceText: true and that Monaco mounts. Troubleshooting accordion entry updated to mention the new affordance. * fix(stack-files): guard the binary-override path against oversized files The override on the binary panel could open Monaco against an empty content buffer if the backend's oversized branch ran (files past the 2 MB inline-preview cap intentionally carry no content even when force=text is set). Saving that empty buffer would wipe the file on disk. Two reinforcing changes: - Initial load now checks result.oversized before result.binary, so a file that is both oversized and has binary bytes in the 8 KB probe shows the Download panel rather than the binary panel. The size signal stays in front of the operator and the override button never surfaces for a file that cannot be safely opened inline. - The handleForceText handler now respects result.oversized on the refetch and transitions to the Download panel instead of clearing isBinary and copying result.content ?? '' into Monaco. Same handler also gains a stale-request guard via a selectedPathRef: a slow override for file A no longer stomps on file B's state if the user navigated away while the request was in flight. Two regression tests pin the new behaviour: oversized+binary surfaces the Download panel on initial load, and an oversized refetch from the binary panel routes to the Download panel rather than Monaco. --- .../src/__tests__/stack-files-routes.test.ts | 34 +++++++ backend/src/routes/stacks.ts | 5 +- backend/src/services/FileSystemService.ts | 10 +- docs/features/stack-file-explorer.mdx | 2 +- frontend/src/components/files/FileViewer.tsx | 98 ++++++++++++++++--- .../files/__tests__/FileViewer.test.tsx | 60 ++++++++++++ frontend/src/lib/stackFilesApi.ts | 8 +- 7 files changed, 195 insertions(+), 22 deletions(-) diff --git a/backend/src/__tests__/stack-files-routes.test.ts b/backend/src/__tests__/stack-files-routes.test.ts index 1c81ae85..fab1e93a 100644 --- a/backend/src/__tests__/stack-files-routes.test.ts +++ b/backend/src/__tests__/stack-files-routes.test.ts @@ -241,6 +241,40 @@ describe('GET /api/stacks/:stackName/files/content', () => { await fs.unlink(bigPath); }, 15000); + + it('returns binary:true by default for a file with a NUL byte in the probe window', async () => { + const oddPath = path.join(stacksDir, STACK, 'force-text-default.txt'); + await fs.writeFile(oddPath, Buffer.from('hello\0world rest is utf8 text', 'utf-8')); + try { + const res = await request(app) + .get(`/api/stacks/${STACK}/files/content`) + .query({ path: 'force-text-default.txt' }) + .set('Cookie', adminCookie); + expect(res.status).toBe(200); + expect(res.body.binary).toBe(true); + expect(res.body.content).toBeUndefined(); + } finally { + await fs.unlink(oddPath); + } + }); + + it('returns the content as UTF-8 when ?force=text is set, even for files the binary heuristic rejects', async () => { + const oddPath = path.join(stacksDir, STACK, 'force-text-on.txt'); + const raw = Buffer.from('hello\0world rest is utf8 text', 'utf-8'); + await fs.writeFile(oddPath, raw); + try { + const res = await request(app) + .get(`/api/stacks/${STACK}/files/content`) + .query({ path: 'force-text-on.txt', force: 'text' }) + .set('Cookie', adminCookie); + expect(res.status).toBe(200); + expect(res.body.binary).toBe(false); + expect(typeof res.body.content).toBe('string'); + expect(res.body.content).toBe(raw.toString('utf-8')); + } finally { + await fs.unlink(oddPath); + } + }); }); // ── GET /:stackName/files/download ──────────────────────────────────────────── diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 4eef3bb3..a8b3ccf5 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1420,10 +1420,11 @@ stacksRouter.get('/:stackName/files/content', async (req: Request, res: Response if (!isValidRelativeStackPath(relPath)) { return res.status(400).json({ error: 'Invalid path', code: 'INVALID_PATH' }); } + const forceText = req.query.force === 'text'; const startedAt = Date.now(); - logFileDiag('read start', { stackName, relPath, nodeId: req.nodeId }); + logFileDiag('read start', { stackName, relPath, nodeId: req.nodeId, forceText }); try { - const result = await FileSystemService.getInstance(req.nodeId).readStackFile(stackName, relPath); + const result = await FileSystemService.getInstance(req.nodeId).readStackFile(stackName, relPath, undefined, { forceText }); // ETag is the integer mtimeMs the file was stat'd with, so the matching // PUT can compare millisecond-equal even though some filesystems return // float mtimeMs. diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index 90a82732..e9e9f3e6 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -675,7 +675,8 @@ export class FileSystemService { async readStackFile( stackName: string, relPath: string, - maxBytes: number = 2 * 1024 * 1024 + maxBytes: number = 2 * 1024 * 1024, + opts: { forceText?: boolean } = {}, ): Promise<{ content?: string; binary: boolean; oversized: boolean; size: number; mime: string; mtimeMs: number }> { const safePath = await this.resolveSafeStackPath(stackName, relPath); const mime = this.guessMime(safePath); @@ -701,7 +702,12 @@ export class FileSystemService { const buf = await fh.readFile(); - if (isBinaryBuffer(buf)) { + // forceText bypasses the binary-detection heuristic so callers can + // recover from false positives (a UTF-8 file that happens to carry a + // NUL or a high non-printable ratio in its first 8 KB). The oversized + // branch above still applies because returning a multi-megabyte file + // as JSON-encoded text is wasteful regardless of the heuristic. + if (!opts.forceText && isBinaryBuffer(buf)) { return { binary: true, oversized: false, size: stat.size, mime, mtimeMs }; } diff --git a/docs/features/stack-file-explorer.mdx b/docs/features/stack-file-explorer.mdx index 0ed35993..40a1e3c4 100644 --- a/docs/features/stack-file-explorer.mdx +++ b/docs/features/stack-file-explorer.mdx @@ -169,7 +169,7 @@ The Permissions dialog opens for everyone; only users with stack edit permission - The binary detection heuristic flagged null bytes or a high proportion of non-printable characters in the file's first kilobytes. This can happen with certain encodings, non-UTF-8 byte sequences, or unusual line-ending mixes. Download the file, edit it locally or via a host terminal, then re-upload. + The binary detection heuristic flagged null bytes or a high proportion of non-printable characters in the file's first kilobytes. This can happen with certain encodings, non-UTF-8 byte sequences, or unusual line-ending mixes. Click **Open as text anyway** on the binary panel to re-fetch the file as UTF-8 text and edit it in the inline viewer. Files above the 2 MB preview limit still need a Download. The file exceeds the 25 MB per-file upload limit. Compress or split the file before uploading, or transfer it via `scp` or `rsync` directly to the stack directory on the host. diff --git a/frontend/src/components/files/FileViewer.tsx b/frontend/src/components/files/FileViewer.tsx index 34d40ad4..bb301286 100644 --- a/frontend/src/components/files/FileViewer.tsx +++ b/frontend/src/components/files/FileViewer.tsx @@ -27,6 +27,7 @@ interface SpecialFilePanelProps { label: string; stackName: string; relPath: string; + extraAction?: { label: string; onClick: () => void; disabled?: boolean }; } function SpecialFilePanel({ @@ -35,6 +36,7 @@ function SpecialFilePanel({ label, stackName, relPath, + extraAction, }: SpecialFilePanelProps) { const [downloading, setDownloading] = useState(false); @@ -69,19 +71,31 @@ function SpecialFilePanel({

{filename}

{label} · {formatBytes(size)}

- + {extraAction && ( + )} - Download - + ); } @@ -115,6 +129,15 @@ export function FileViewer({ onDirtyChangeRef.current = onDirtyChange; }, [onDirtyChange]); + // Mirror selectedPath into a ref so async handlers fired from button clicks + // (handleForceText) can detect a stale response after the user has already + // navigated to a different file. A slow override for file A must not stomp + // on file B's state. + const selectedPathRef = useRef(selectedPath); + useEffect(() => { + selectedPathRef.current = selectedPath; + }, [selectedPath]); + useEffect(() => { onDirtyChangeRef.current?.(hasChanges); }, [hasChanges]); @@ -156,10 +179,17 @@ export function FileViewer({ if (cancelled) return; setSize(result.size); setLoadedMtimeMs(result.mtimeMs); - if (result.binary) { - setIsBinary(true); - } else if (result.oversized) { + // Check oversized BEFORE binary: the backend returns oversized:true + // for files past the 2 MB inline-preview cap regardless of the binary + // probe, and the body intentionally carries no content for those + // files. Showing the binary panel for an oversized file would hide + // the size signal and offer an override that resolves to an empty + // editor (the backend keeps the oversized-no-content contract even + // when force=text is set). + if (result.oversized) { setIsOversized(true); + } else if (result.binary) { + setIsBinary(true); } else { const text = result.content ?? ''; setContent(text); @@ -241,6 +271,40 @@ export function FileViewer({ const language = extensionToLanguage(filename); if (isBinary) { + const handleForceText = async () => { + const requestedPath = selectedPath; + setLoading(true); + setError(null); + try { + const result = await readStackFile(stackName, requestedPath, { forceText: true }); + // Stale-request guard: the user may have navigated to a different + // file while the override request was in flight. Drop the response + // rather than stomp on the new file's state. + if (selectedPathRef.current !== requestedPath) return; + setSize(result.size); + setLoadedMtimeMs(result.mtimeMs); + if (result.oversized) { + // Backend keeps oversized files out of the inline editor even with + // force=text set; the body has no content. Surface the Download + // panel so the user does not get an empty Monaco buffer that they + // could accidentally save back over the file. + setIsBinary(false); + setIsOversized(true); + } else { + const text = result.content ?? ''; + setContent(text); + setOriginalContent(text); + setIsBinary(false); + } + } catch (e) { + if (selectedPathRef.current !== requestedPath) return; + const message = e instanceof Error ? e.message : 'Failed to load file as text.'; + setError(message); + toast.error(message); + } finally { + if (selectedPathRef.current === requestedPath) setLoading(false); + } + }; return ( void handleForceText(), + }} /> ); } diff --git a/frontend/src/components/files/__tests__/FileViewer.test.tsx b/frontend/src/components/files/__tests__/FileViewer.test.tsx index c1873d78..9e1db843 100644 --- a/frontend/src/components/files/__tests__/FileViewer.test.tsx +++ b/frontend/src/components/files/__tests__/FileViewer.test.tsx @@ -238,6 +238,66 @@ describe('FileViewer', () => { expect(opts).toEqual({ ifMatchMtimeMs: 1_700_000_000_000 }); }); + it('binary panel offers "Open as text anyway"; click refetches with forceText and renders Monaco', async () => { + mockReadFile + .mockResolvedValueOnce(binaryResult()) + .mockResolvedValueOnce(textResult('rescued as text')); + + render(); + await screen.findByText(/binary file/i); + + const overrideBtn = screen.getByRole('button', { name: /open as text anyway/i }); + overrideBtn.click(); + + await waitFor(() => expect(screen.getByTestId('monaco-editor')).toBeInTheDocument()); + expect(mockReadFile).toHaveBeenCalledTimes(2); + expect(mockReadFile).toHaveBeenNthCalledWith(2, 'my-stack', 'quirky-utf8.txt', { forceText: true }); + }); + + it('renders the oversized panel (not the binary panel) when both flags are set', async () => { + // The backend can return `binary: true, oversized: true` when a >2 MB + // file's first 8 KB probe also looks binary. Surfacing the binary panel + // there would hide the size signal and offer an override that resolves + // to an empty editor, so the oversized panel must win. + mockReadFile.mockResolvedValue({ + binary: true, + oversized: true, + size: 5_000_000, + mime: 'application/octet-stream', + mtimeMs: 1_700_000_000_000, + }); + + render(); + + expect(await screen.findByText(/too large to preview/i)).toBeInTheDocument(); + expect(screen.queryByText(/binary file/i)).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /open as text anyway/i })).not.toBeInTheDocument(); + }); + + it('override on an oversized response falls back to the Download panel, never Monaco', async () => { + // Defence-in-depth: even if a future regression let the override button + // surface on an oversized file, the handler must not open Monaco against + // an empty content buffer. Save would then wipe the file on disk. + mockReadFile + .mockResolvedValueOnce(binaryResult()) + .mockResolvedValueOnce({ + binary: false, + oversized: true, + size: 5_000_000, + mime: 'text/plain', + mtimeMs: 1_700_000_000_000, + }); + + render(); + await screen.findByText(/binary file/i); + + const overrideBtn = screen.getByRole('button', { name: /open as text anyway/i }); + overrideBtn.click(); + + expect(await screen.findByText(/too large to preview/i)).toBeInTheDocument(); + expect(screen.queryByTestId('monaco-editor')).not.toBeInTheDocument(); + }); + it('updates baseline on FileConflictError without discarding the user buffer; follow-up save uses new mtime', async () => { mockReadFile.mockResolvedValue(textResult('stale local copy')); mockWriteFile diff --git a/frontend/src/lib/stackFilesApi.ts b/frontend/src/lib/stackFilesApi.ts index 5a033849..33531a82 100644 --- a/frontend/src/lib/stackFilesApi.ts +++ b/frontend/src/lib/stackFilesApi.ts @@ -86,10 +86,14 @@ export async function listStackDirectory( export async function readStackFile( stackName: string, - relPath: string + relPath: string, + options?: { forceText?: boolean } ): Promise { assertSafeRelPath(relPath); - const res = await apiFetch(stackFilesUrl(stackName, `/content?path=${encodeURIComponent(relPath)}`)); + const forceSuffix = options?.forceText ? '&force=text' : ''; + const res = await apiFetch( + stackFilesUrl(stackName, `/content?path=${encodeURIComponent(relPath)}${forceSuffix}`) + ); if (!res.ok) throw new Error(await parseApiError(res)); return res.json() as Promise; }