feat(website): inline text preview for .docx / .pptx in the file viewer - #2716
Conversation
9694b74 to
f67e809
Compare
f67e809 to
a9aeba6
Compare
a9aeba6 to
d83d1eb
Compare
6914299 to
371206a
Compare
371206a to
3e89125
Compare
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed BLOCKING -- src/kiro_crew/doc_parser.py:314 -- Text-only budget permits decompression exhaustion BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:2408 -- Path resolution blocks the event loop (origin: validation) |
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The design mirrors the existing sheet-preview endpoint precisely: shared Design-Verdict: PASS Solves a real gap with the proven sheet-endpoint shape — shared security prefix, no new deps, and every failure path degrades to the download card. [DESIGN-REVIEWED] 4e8e5c2 |
|
🤖 Kiro Crew [operator: bolichen97]: Pushed |
a1d1f84 to
a16359b
Compare
|
🤖 Kiro Crew [operator: bolichen97]: Pushed |
|
🤖 Kiro Crew [operator: bolichen97]: Pushed |
|
🤖 Kiro Crew [operator: bolichen97]: Pushed BLOCKING — unbounded ZIP central directory: added an on-disk size gate BEFORE any ZIP parsing — FINDING — staleTime serves stale preview: adopted; error-code baseline regenerated (new |
|
🤖 Kiro Crew [operator: bolichen97]: Pushed |
|
🤖 Kiro Crew [operator: bolichen97]: Pushed Design review 🟡 CONCERNS (advisory) — answered, deferred to follow-up: the Drive status: this was push 10 of the 10-push drive budget. GPT finding counts per round: 5→3→2→1→1, all distinct legitimate findings, none re-raised after rebuttal. If this head comes back green it is review-ready; any further blocking rounds go to the operator. |
|
🤖 Kiro Crew [operator: bolichen97]: Drive-to-green pausing here — push budget exhausted (10/10). Escalating to the operator rather than continuing. Where this PR stands on
Operator options: (a) grant additional push budget to fix both here (finding 2 is trivial; finding 1 means doing #4309's fd refactor inside this PR after all), (b) fix finding 2 only and dispute finding 1 as out-of-PR-scope per the #4309 deferral, or (c) leave the PR for the author with all six review rounds documented. Full drive history is in the comment chain above: rebase over the routes-refactor, PR Hygiene squash, review rounds fixing event-loop offload, SEL audit gaps, shared resolve helper, pptx aggregate budget, dead contract fields, 13-locale copy fixes, client short-circuit, Windows collect-ignore + pinned-set sync, ZIP size gate, staleTime, CancelledError audit, and redact-before-truncate. Original feature authorship (Harpreet Singh) preserved throughout. |
|
🤖 Kiro Crew [operator: zejiangg]: Pushed Rebase onto current main (was CONFLICTING, 847 commits behind). Two conflicts: GPT round-6 BLOCKING #1 — getsize→open TOCTOU: fixed with the open-once fd discipline the sibling endpoints already use: the handler opens via GPT round-6 BLOCKING #2 — docx budget off-by-one: the All local gates green: isort/flake8/mypy, error-code contract, doc_parser + office-preview + download/raw/surface suites (126 tests), tsc, eslint, vitest FileRenderers 24/24. |
|
Dispositions for GPT 5.6's two blocking findings on
|
|
🤖 Kiro Crew [operator: zejiangg]: Pushed |
|
🤖 Kiro Crew [operator: zejiangg]: Escalating — PR-side work complete on What this drive fixed (details in the two comments above): rebased onto current main (was CONFLICTING, 847 behind), fixed both GPT round-6 BLOCKING findings — the getsize→open TOCTOU via open-once Remaining reds, all reproduced on pristine main at this PR's exact rebase base
Why the review bots show "skipped" on this head: the fork review pipeline ( Maintainer actions needed: resolve #5846 (ruling) and land the #5850 hiStyle fix (or the pending reword), then this PR needs only a CI re-run — no further code work is expected. |
|
🤖 Kiro Crew [operator: zejiangg]: Pushed |
|
🤖 Kiro Crew [operator: zejiangg]: Review-ready at |
bolichen97
left a comment
There was a problem hiding this comment.
Re-approved after conflict resolution against the latest main; focused validation passed.
|
🤖 Kiro Crew [operator: zejiangg]: Pushed Why: Zero feature-code changes. The commit is the previously-reviewed The GPT round-6 blockers from Local gates re-run green on |
|
Note for whoever drives this PR next: follow-up issue #4309 has been closed as obsolete, and one requirement from it is carried forward here so it is not lost. #4309 asked for two things. The shared envelope half is already done on main: That makes the standing First Principles BLOCK on this PR concrete and mechanical: The cross-caller |
Extract plaintext from OOXML documents via kiro_crew.doc_parser and render a scrollable inline preview in the dashboard file viewer, replacing the download-only card for .docx / .pptx. Never-previewable office formats (.doc, .ppt, .xls, .xlsx, ODF) render the download card directly on the client; the backend 415 stays as the safety net. Feature by Harpreet Singh. Includes review fixes driven by Kiro Crew: - the endpoint adopts the SHARED open-and-check prefix _open_checked_file instead of hand-rolling a second spelling of it, and the whole envelope (validate -> sensitive-path -> is-file -> _open_rb_nofollow -> fstat -> ZIP+XML parse -> redact) now runs in ONE worker-thread hop, exactly as api_file_sheet does. Fixes two blocking findings with one change: the validation/open/fstat sequence no longer runs on the event loop (an NFS/FUSE-backed document stalled every session and the heartbeat), and the security boundary has one spelling again, so a future hardening change to it lands here too. The 50 MB ceiling is expressed as the prefix's fstat_cap; the checked file object never crosses back to the loop (GPT blocking, First Principles BLOCK) - extract_text offloaded via asyncio.to_thread (event-loop stall, GPT/Opus blocking) and bounded by a new opt-in max_chars aggregate budget in doc_parser (cap + 1) so a many-slide deck stops parsing at the preview cap instead of accumulating unbounded text (GPT blocking) - resolve=1 uses the shared _resolve_project_relative helper (Windows/UNC gate preserved) with SEL-audited denials, replacing a hand-inlined divergent copy (GPT blocking, Design/First Principles) - SEL audit record on the unsupported-format 415 branch (GPT blocking) - doc_parser import at module scope; useQuery instead of manual useEffect fetch; extension badge at text-[10px] (GPT findings) - zero-consumer response fields dropped: format, supported, empty - open-once fd discipline: the size gate fstats the fd the prefix opened and doc_parser parses through that SAME handle (new opt-in extract_text fileobj param) - closes the getsize->open TOCTOU and adds atomic symlink refusal, matching api_file_raw (GPT blocking; also the Design review's deferred _open_rb_nofollow parity ask) - docx budget separator accounting: the "\n" join separator is charged only BETWEEN paragraphs, so a cap-sized opening paragraph no longer stops extraction with the result reading as un-truncated (GPT blocking) - fallback-card copy no longer claims office files can't be previewed; truncation copy no longer hardcodes the 512 KB cap (UX, 13 locales) - truncation notice pinned in the always-visible footer; preview scroll region keyboard-focusable (UX) - error-code baseline regenerated via the official --update Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
|
🤖 Kiro Crew [operator: zejiangg]: Pushed The fix (GPT BLOCKING + First Principles BLOCK were the same root cause).
Everything the earlier rounds earned is preserved, deliberately:
One extra fix the rebase surfaced. Main gained Tests. Rewrote Local gates, all green before pushing: 174 backend tests (office-preview, doc_parser, zip_vet, error-code contract, CI surface guards, file-download / file-raw / file-sheet, knowledge-format parity), the black gate, isort, flake8, mypy, On the First Principles Subtraction (not a Blocker): keeping |
Problem / Motivation
#2615stopped the dashboard file viewer rendering raw ZIP bytes for Office documents, but it did so by replacing them with a download-only card. So a user browsing a shared.docxor.pptxin the file tree still cannot read it in place: the only way to see a one-paragraph memo or a three-slide deck is to download it and open it in another application.That is a gap rather than a bug, and specifically a gap in coverage rather than in capability —
kiro_crew.doc_parseralready parses OOXML.docx/.pptxinto plaintext for the attachment and knowledge-ingest readers. The file viewer simply never asked it.Why it matters
Reading a document is the common case; saving it is the exception. Anyone reviewing agent-produced reports, shared specs, or meeting decks in the file tree currently pays a download-open-close round trip per file, which is enough friction that documents go unread. Because the parser already exists, closing the gap costs an endpoint rather than a dependency — no
python-docx, nopython-pptx, noopenpyxl.What changed (motivation → approach → change)
Goal: show a document's text in the panel, and degrade to today's download card whenever that is not possible.
Approach. The parser already existed, so the work is a new read endpoint plus a viewer state — no new parsing code and no new dependency. Two design choices are worth stating because alternatives were available:
files.pyships_open_checked_file(files.py:2041) as the file-serving open-and-check prefix — validate → sensitive-path → is-file →_open_rb_nofollow→ fstat — split out precisely so an endpoint that must keep the open file object can still share it. This endpoint adopts it and becomes the fifth adopter, rather than carrying its own inlined copy of that sequence.api_file_sheet's_open_and_load(files.py:4052) is the template followed.asyncio.to_threadcallback. Every path that opens the file also closes it on that thread, so a cancelled request cannot strand an open fd in a discarded future or finalize one on the event loop. On an NFS/FUSE-backed document, doing any of this on the loop would stall every session's streaming and the liveness heartbeat.What was built.
Backend — new
GET /api/file-office-preview?path=…[&resolve=1](files.py, registered inroutes/taskrunner.py, exported fromhandlers/__init__.py):.docxand.pptxonly. Anything else answers415 unsupported_preview_format— not400— so the frontend can tell "wrong format, keep the download card" from "the request was broken". It is raised from inside the worker hop as an endpoint-local_PreviewUnsupported, mirroringapi_file_sheet's_SheetRefusal, rather than adding an endpoint-specific code to_OpenDenied's shared vocabulary.fstat_cap=_MAX_UPLOAD_BYTES(50 MB, same ceiling as uploads), enforced on the fd beforezipfilematerializes the archive's central directory — that allocation is bounded only by the file itself, so it must be gated ahead of any parsing. Refusal is an SEL-audited413 file_too_large._OFFICE_PREVIEW_CAP), mirroringapi_file_read. Extraction is bounded atcap + 1so truncation stays detectable, andredact()runs on the full extracted text before the slice — cutting first could sever a credential across the cap boundary and leave a prefix the redactor no longer matches.resolve=1goes through the shared_resolve_project_relativehelper (its Windows-absolute/UNC pass-through matters: the validator's network-path gate sits before its realpath). Both failure modes record an SELdenied.asyncio.CancelledErrorrecords an SELcancelledoutcome and re-raises, so a shutdown or client disconnect mid-parse still leaves the access in the audit trail.{text, truncated}. Noformat/supported/emptyfields — they had no consumer, anddoc_parserreturns""for both a blank document and a parse failure, so "empty" cannot be reported honestly.Backend —
doc_parsergains two opt-in parameters (both undeclared in the original description, both derived from review findings, both with exactly one consumer by design):max_chars: an aggregate extraction budget. Without it a deck with thousands of slides, each under the per-entry decompression cap, accumulates unbounded text. The docx path charges the"\n"join separator only between paragraphs, so a cap-sized opening paragraph no longer halts extraction while the result reads as un-truncated.fileobj: parse from an already-open handle instead of re-openingpath, which closes the stat→open TOCTOU window — the bytes parsed are exactly the bytes the size gate measured. Path behaviour is unchanged for the attachment and knowledge readers; the cross-caller migration is deliberately not in scope._vet_archive_inventoryis now handle-aware: when afileobjis passed it reads the EOCD tail from that handle via the vet module's existingvet_zip_inventory_bytesand rewinds, because vettingpathwhilezipfileparses the fd would bound a different archive than the one opened.Frontend (
FileRenderers.tsx,utils/fileReadUrl.ts):OfficeViewerfetches throughuseQuery(staleTime: 0) and renders one of three states: a "Loading preview…" placeholder; the extracted text in a keyboard-focusable scroll region with a compact "Download original" button and the truncation notice pinned in an always-visible footer; or the original download card on any non-2xx, empty text, or fetch error.OfficeCardcomponent so the compact and full presentations cannot drift.fileOfficePreviewUrlis derived fromfileDownloadUrlrather than restated — the two endpoints take an identical query shape, so the endpoint segment is swapped and the construction keeps one owner.OFFICE_PREVIEWABLE_EXTSshort-circuits known-unsupported extensions client-side, following the existingSHEET_EXTSprecedent, so a.doc/.xls/ODF file renders its card immediately instead of flashing a loading state through a guaranteed 415. The backend 415 remains the authority and is tested as the safety net for list drift.i18n — also undeclared originally: three new keys under
components.fileRenderers(office_preview_loading,office_preview_truncated,office_download_original) plus one reworded existing key —office_download_hintsaid Office documents "can't be previewed inline", which this PR makes false. 13 catalogs updated.Not previewable in this PR, unchanged from
#2615:.xls/.xlsx(noopenpyxldependency declared here — those already have their own inline path via/api/file-sheet),.doc/.ppt(legacy OLE, outsidedoc_parser's scope),.odt/.ods/.odp.Tests
Backend —
test/test_file_office_preview.py(12 tests, new file):test_docx_preview_returns_text_and_truncated_only— the response contract, including that the dropped zero-consumer fields stay absent.test_truncation_flag_set_and_text_capped— thecap + 1budget keeps truncation detectable while the returned text is cut to exactly the cap.test_redaction_runs_before_truncation— anAKIAtoken deliberately straddling the cap boundary; neither the secret nor its cap-cut prefix may appear.test_open_envelope_is_the_shared_prefix_and_runs_off_the_loop— pins both halves of the envelope finding:_open_checked_fileis what runs (not an inline copy), it runs off the event loop, and it carries the 50 MBfstat_cap.test_extraction_reads_through_the_prefix_fd_and_closes_it—extract_textreceives the same handle the prefix opened, and that handle is already closed by the time the response is built.test_oversized_file_413_before_any_parsing— a real sparse 51 MB file, so the gate is exercised on the fd rather than on a mocked path stat.test_unsupported_extension_415_with_sel_audit,test_sensitive_path_403,test_forbidden_path_400— the refusal statuses, each with its SEL record.test_resolve_uses_shared_helper,test_resolve_outside_project_denied_and_audited—resolve=1routes through the shared helper and audits the denial.test_cancellation_is_sel_audited_and_reraised— the access survives in the audit trail andCancelledErrorstill propagates.Backend —
test/test_doc_parser.py(7 new tests, two classes):TestMaxCharsBudgetlocks the aggregate budget (pptx slide iteration stops, docx paragraphs bound, no budget still extracts everything, and a cap-sized first paragraph does not halt extraction);TestFileobjExtractionlocks that handle-based and path-based extraction agree for both formats and that the handle is genuinely what gets read.Frontend —
website/src/test/FileRenderers.test.tsx(24 tests total): eightOfficeViewercases covering the preview happy path, the focusable scroll region, 415 fallback, no-fetch for never-previewable extensions, fetch-throw fallback, empty-text fallback, the pinned truncation notice, and Windows backslash basename extraction — plus thedetectFileTyperouting cases that keep.pdfon the PDF path and OOXML spreadsheets on the sheet path.Mechanical, per sibling convention:
test/windows-collect-ignore.txtlists the new HTTP handler suite becauseFILE_READ_SCHEMA's path pattern is POSIX-only (same treatment astest_file_download.py/test_file_raw.py), with the matching entry intest_ci_surface_tests.pywhich pins that file's exact contents;error-code-baseline.jsonregenerated with the officialpython test/test_error_code_contract.py --update, never hand-edited.Manual verification
Opened a
.docxand a.pptxin the dashboard file viewer and confirmed the preview renders with the download button and, on an over-cap document, the pinned truncation notice; confirmed a.docand an.xlsstill render the download card. Refusal paths (415 / 403 / 404 / 413 / cancellation) are covered by the endpoint tests above, which exercise the real handler against real files rather than mocks, so they need no separate manual pass.Screenshots / video
The fallback state is unchanged from
#2615— the same download card, with only the hint sentence reworded.Related Issues
Follows
#2615(merged), which introduced the download card this PR previews past. No issue is closed by this PR; it was filed directly as an enhancement.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement