Skip to content

feat(website): inline text preview for .docx / .pptx in the file viewer - #2716

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
harpreetmultani1994:feat/office-file-preview
Aug 30, 2026
Merged

feat(website): inline text preview for .docx / .pptx in the file viewer#2716
iamwhatever merged 1 commit into
kirodotdev:mainfrom
harpreetmultani1994:feat/office-file-preview

Conversation

@harpreetmultani1994

@harpreetmultani1994 harpreetmultani1994 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

#2615 stopped 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 .docx or .pptx in 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_parser already parses OOXML .docx/.pptx into 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, no python-pptx, no openpyxl.

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:

  • The security envelope is shared, not written. files.py ships _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.
  • One worker-thread hop, and the checked fd never returns to the loop. Everything blocking or CPU-bound — realpath validation, the sensitive-path screen, the open, the fstat, ZIP decompression, XML parsing, redaction — runs inside a single asyncio.to_thread callback. 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 in routes/taskrunner.py, exported from handlers/__init__.py):

  • Format gate: .docx and .pptx only. Anything else answers 415 unsupported_preview_format — not 400 — 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, mirroring api_file_sheet's _SheetRefusal, rather than adding an endpoint-specific code to _OpenDenied's shared vocabulary.
  • Size policy: the shared prefix's fstat_cap=_MAX_UPLOAD_BYTES (50 MB, same ceiling as uploads), enforced on the fd before zipfile materializes 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-audited 413 file_too_large.
  • Text cap: 512 KB (_OFFICE_PREVIEW_CAP), mirroring api_file_read. Extraction is bounded at cap + 1 so truncation stays detectable, and redact() 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=1 goes through the shared _resolve_project_relative helper (its Windows-absolute/UNC pass-through matters: the validator's network-path gate sits before its realpath). Both failure modes record an SEL denied.
  • asyncio.CancelledError records an SEL cancelled outcome and re-raises, so a shutdown or client disconnect mid-parse still leaves the access in the audit trail.
  • Response is {text, truncated}. No format / supported / empty fields — they had no consumer, and doc_parser returns "" for both a blank document and a parse failure, so "empty" cannot be reported honestly.

Backend — doc_parser gains 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-opening path, 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.
  • Consequently _vet_archive_inventory is now handle-aware: when a fileobj is passed it reads the EOCD tail from that handle via the vet module's existing vet_zip_inventory_bytes and rewinds, because vetting path while zipfile parses the fd would bound a different archive than the one opened.

Frontend (FileRenderers.tsx, utils/fileReadUrl.ts):

  • OfficeViewer fetches through useQuery (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.
  • Both card states are one shared OfficeCard component so the compact and full presentations cannot drift.
  • fileOfficePreviewUrl is derived from fileDownloadUrl rather than restated — the two endpoints take an identical query shape, so the endpoint segment is swapped and the construction keeps one owner.
  • OFFICE_PREVIEWABLE_EXTS short-circuits known-unsupported extensions client-side, following the existing SHEET_EXTS precedent, 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_hint said 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 (no openpyxl dependency declared here — those already have their own inline path via /api/file-sheet), .doc/.ppt (legacy OLE, outside doc_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 — the cap + 1 budget keeps truncation detectable while the returned text is cut to exactly the cap.
  • test_redaction_runs_before_truncation — an AKIA token 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_file is what runs (not an inline copy), it runs off the event loop, and it carries the 50 MB fstat_cap.
  • test_extraction_reads_through_the_prefix_fd_and_closes_itextract_text receives 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_auditedresolve=1 routes through the shared helper and audits the denial.
  • test_cancellation_is_sel_audited_and_reraised — the access survives in the audit trail and CancelledError still propagates.

Backend — test/test_doc_parser.py (7 new tests, two classes): TestMaxCharsBudget locks 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); TestFileobjExtraction locks 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): eight OfficeViewer cases 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 the detectFileType routing cases that keep .pdf on the PDF path and OOXML spreadsheets on the sheet path.

Mechanical, per sibling convention: test/windows-collect-ignore.txt lists the new HTTP handler suite because FILE_READ_SCHEMA's path pattern is POSIX-only (same treatment as test_file_download.py / test_file_raw.py), with the matching entry in test_ci_surface_tests.py which pins that file's exact contents; error-code-baseline.json regenerated with the official python test/test_error_code_contract.py --update, never hand-edited.

Manual verification

Opened a .docx and a .pptx in 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 .doc and an .xls still 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

OfficeViewer preview state

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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@harpreetmultani1994
harpreetmultani1994 requested a review from a team August 11, 2026 01:34
@harpreetmultani1994
harpreetmultani1994 requested a review from a team as a code owner August 11, 2026 01:34
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 11, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 11, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 11, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 11, 2026
@harpreetmultani1994
harpreetmultani1994 force-pushed the feat/office-file-preview branch 2 times, most recently from 6914299 to 371206a Compare August 11, 2026 16:35
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 11, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 11, 2026
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 4e8e5c295243852bb16b99e27e7161cc40e9798e via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- src/kiro_crew/doc_parser.py:314 -- Text-only budget permits decompression exhaustion
if max_chars is not None and collected >= max_chars:
A sub-50MB PPTX containing thousands of highly compressed, text-free slides -> preview parses every expanded XML entry because collected remains zero -> requests exhaust CPU and worker threads.
Anchor: residual/security
Fix: Enforce a cumulative decompressed-byte or processed-slide budget independent of extracted text.

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:2408 -- Path resolution blocks the event loop (origin: validation)
raw_path, _resolve_err = _resolve_project_relative(raw_path)
A relative preview path on NFS/FUSE -> the handler calls os.path.realpath synchronously -> the gateway loop and heartbeat stall.
Anchor: no-blocking-call-on-event-loop
Fix: Run _resolve_project_relative through await asyncio.to_thread(...).

[BLOCK-MERGE] 4e8e5c2
[GPT-REVIEWED] 4e8e5c2

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 4e8e5c295243852bb16b99e27e7161cc40e9798e via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The design mirrors the existing sheet-preview endpoint precisely: shared _open_checked_file prefix, worker-thread hop, typed refusals, SEL audit, and a fail-safe frontend fallback. All base symbols the diff relies on (vet_zip_inventory_bytes, TAIL_WINDOW, _OpenDenied codes, redact, _resolve_project_relative) exist and are used as the description claims. The doc_parser extensions (max_chars, fileobj) are backward-compatible keyword additions, and .xlsx already has its own preview path via SheetViewer, so the format exclusions leave no gap. The frontend's duplicated extension list is a deliberate, acknowledged optimization whose drift mode fails safe (415 → download card). No one-way doors: the new endpoint is internal dashboard surface with a minimal {text, truncated} contract.

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

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed db99e5ad0 — fixes the Windows CI failure (5 failed tests in the new test_file_office_preview.py, everything else green). Root cause: FILE_READ_SCHEMA's path pattern is POSIX-only (^[~/]...) and rejects Windows C:\ paths with 400 — a pre-existing platform behavior on main that the sibling handler test files (test_file_download.py, test_file_raw.py) already handle by being listed in test/windows-collect-ignore.txt. Fix follows the same convention: the HTTP-level handler tests are added to the collect-ignore list, and the 3 platform-independent doc_parser budget tests moved into test_doc_parser.py so they keep running on Windows. No production code changed in this push. The earlier CI-run cancellation (Windows shard at 41 min) was a runner-side cancel unrelated to the diff; the rerun surfaced this real finding.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 18, 2026
@bolichen97
bolichen97 force-pushed the feat/office-file-preview branch 2 times, most recently from a1d1f84 to a16359b Compare August 18, 2026 06:08
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 18, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed a16359bd1 — rebased onto main again. Root cause of the missing CI on the previous two pushes: main advanced (PR #4049 and siblings) and the PR went CONFLICTING, and GitHub does not create pull_request workflow runs when the merge ref can't be built (only the pull_request_target guard/readiness workflows fired). Conflicts were with the newly-merged SheetViewer work in FileRenderers.tsx: kept main's SheetViewer intact, preserved its <OfficeViewer hideHint /> fallback contract by threading the new hideHint prop through the preview-capable OfficeViewer and its card, and wrapped SheetViewer's two failure-path tests in the React Query provider its OfficeViewer fallback now requires. error-code baseline re-synced (1 line, main-side drift). All local gates green (tsc, eslint, vitest 24/24 in the merged test file, isort/flake8/mypy, error-code + route-table + handler + doc_parser suites).

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 18, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed 531f7b3fa — one-line fix for the two red backend shards (3.10/1 and Windows/1, same root cause): test_ci_surface_tests.py::test_ignore_list_matches_the_names_conftest_previously_inlined pins the exact contents of test/windows-collect-ignore.txt, so adding test_file_office_preview.py to the list requires adding it to the pinned set too. Contract suite now 41/41 locally. No production code changed.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed b84aa1c58 — round-4 review fixes for GPT's findings on 531f7b3 (Design and UX both moved to ✅ PASS; First Principles moved from BLOCK to 🟡 CONCERNS):

BLOCKING — unbounded ZIP central directory: added an on-disk size gate BEFORE any ZIP parsing — os.path.getsize checked against the existing _MAX_UPLOAD_BYTES (50 MB, same ceiling as file uploads) with an SEL-audited 413 file_too_large denial. zipfile.ZipFile materializes the central directory in memory bounded only by the file itself, so the gate must run before extract_text opens the archive; the per-entry (50 MB) and aggregate (max_chars) budgets then bound everything past the open. New backend test pins the 413 + SEL record with the gate firing before any parsing.

FINDING — staleTime serves stale preview: adopted; staleTime: 0 so a reopened file always refetches (the document may have been edited since the last preview). Within-mount dedupe still applies.

error-code baseline regenerated (new file_too_large code). All local gates green: isort/flake8/mypy, error-code contract, handler suite 8/8, tsc/eslint, vitest 24/24.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed 62e0f2fdd — GPT round-4's single remaining BLOCKING (finding counts per round: 5→3→2→1; Opus is now ✅ no-blocking, Design/UX ✅ PASS): a CancelledError during the offloaded extraction (gateway shutdown / client disconnect mid-parse) is a BaseException and bypassed the except Exception guard, leaving an already-performed file access with no SEL outcome. Added an except asyncio.CancelledError branch that records an SEL cancelled outcome and re-raises — per the repo's established cancellation semantics, the exception still propagates. New test pins record-then-reraise. Handler suite 9/9, isort/flake8/mypy and error-code contract green locally. Note per drive policy: this is push 9 of a 10-push budget.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]: Pushed 6549f3857 — GPT round-5's single BLOCKING, a genuine ordering bug introduced by the truncation logic: text was sliced to the 512K cap BEFORE redact(), so a credential straddling the cap boundary would be cut mid-token and the unmatched prefix could leak into the preview. Redaction now runs on the full extracted text first, then the slice; the truncated flag still reflects the raw extraction hitting the budget. New test pins the exact straddle scenario (fake AKIA ID crossing the boundary — neither the token nor its cut prefix may appear). Handler suite 10/10, all local gates green.

Design review 🟡 CONCERNS (advisory) — answered, deferred to follow-up: the _open_rb_nofollow parity point is fair — this endpoint lets doc_parser open the bare path while download/raw/sheet open through the O_NOFOLLOW helper. As the review itself notes, residual risk is narrow (redaction runs; non-ZIP content extracts to ""), and threading an fd through extract_text_extract_docx/_extract_pptx (and its other callers: attachments, knowledge readers) is a doc_parser API change that belongs in its own reviewed change, together with the suggested shared validate→sensitive→size→SEL envelope helper — the same drift already exists across the other four hand-copied envelopes on main. Filing a follow-up issue so it lands as one coherent refactor rather than a sixth divergent copy here.

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.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 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 6549f3857: CI is fully green (51 checks, 0 failures), MERGEABLE, Opus ✅ no-blocking, Design ✅ PASS, UX 🟡 advisory CONCERNS, First Principles 🟡 advisory. The one remaining blocker is GPT 5.6, which posted 2 new findings this round after 5 rounds of distinct findings (per-round counts: 5→3→2→1→1→2):

  1. getsize→open TOCTOU: the size gate stats the path, then extract_text reopens it — a swap in between bypasses the gate. This is the same open-once-fd discipline that the Design review raised as advisory and that was deferred to files.py: share one validate→sensitive→size→SEL envelope and O_NOFOLLOW open across file-content endpoints #4309, because threading an fd through doc_parser.extract_text changes an API shared with attachments and knowledge readers. GPT has now escalated the same root cause to blocking.
  2. docx budget off-by-one: collected += len(joined) + 1 counts a separator for the first paragraph too, so a cap-sized first paragraph can stop extraction with truncated reading false. Real but extreme edge case; one-line fix plus a test.

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.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Pushed b5814095 — adopted from the escalation at 10/10 push budget; fresh budget, same drive rules. This push carries three things:

Rebase onto current main (was CONFLICTING, 847 commits behind). Two conflicts: FileRenderers.tsx — main added a useLanguageGeneration() subscription to the memoized viewers; kept this PR's OfficeCard/preview structure and threaded that subscription into the preview-capable OfficeViewer (memo() bails out of the provider-level repaint, so the subscription must be direct). error-code-baseline.json — regenerated from the merged tree via the official --update.

GPT round-6 BLOCKING #1 — getsize→open TOCTOU: fixed with the open-once fd discipline the sibling endpoints already use: the handler opens via _open_rb_nofollow (atomic symlink refusal, SEL-audited 403), size-gates with fstat on that fd, and doc_parser parses through the SAME handle — extract_text gains an opt-in fileobj param (zipfile.ZipFile accepts file objects natively; all existing callers unchanged, path-based behavior untouched). The bytes parsed are now exactly the bytes measured. This also closes the Design review's deferred _open_rb_nofollow parity concern for THIS endpoint without the cross-caller doc_parser API migration tracked in #4309. Tests: fd-threading pin (extract_text must receive the gated handle), fileobj-vs-path equivalence for docx/pptx, and a decoy-path test proving the handle's bytes win; the 413 test now uses a real sparse 51 MB file since the gate no longer stats the path.

GPT round-6 BLOCKING #2 — docx budget off-by-one: the "\n" join separator is now charged only BETWEEN paragraphs, so collected tracks len("\n".join(paragraphs)) exactly. Previously a cap-sized opening paragraph stopped extraction while the caller's length check read the result as un-truncated, silently dropping the rest of the document. Regression test pins a cap-sized first paragraph + TAIL-MARKER second paragraph: the tail must survive and the result must read as truncated.

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.

@iamwhatever

Copy link
Copy Markdown
Collaborator

Dispositions for GPT 5.6's two blocking findings on 6549f385, addressed in b5814095:

  • getsize→open TOCTOU on the size gate — fixed

    The handler now opens the file exactly once via _open_rb_nofollow (the same atomic symlink-refusing helper api_file_raw / api_file_download use), runs the 50 MB gate with os.fstat on that fd, and hands the SAME open handle to doc_parser.extract_text via a new opt-in fileobj parameter. There is no second open of the path, so no swap window exists between measuring and parsing. Existing extract_text callers (attachments, knowledge readers) are untouched — the parameter defaults to None and path-based behavior is unchanged, which keeps the change inside this PR's scope while the cross-caller shared-envelope migration stays tracked in files.py: share one validate→sensitive→size→SEL envelope and O_NOFOLLOW open across file-content endpoints #4309. Pinned by three new tests: extract_text must receive the gated handle, fileobj-vs-path result equivalence (docx + pptx), and a decoy-path test proving the handle's bytes win over the path argument.

  • docx budget separator off-by-one (collected += len(joined) + 1) — fixed

    The separator is now charged only between paragraphs (if paragraphs: collected += 1 before append), so collected equals len("\n".join(paragraphs)) exactly and a cap-sized first paragraph no longer terminates extraction with the caller's length check reading the result as un-truncated. Regression test: first paragraph exactly cap-sized, second paragraph TAIL-MARKER, budget cap+1 — the tail must survive and the returned length must exceed the cap so truncation is detectable.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Pushed 990716d1 — black formatting on the new test/test_file_office_preview.py (the diff-baselined black gate covers the whole file since it is new vs main; a local-gate miss on my side, no logic change — git diff b5814095..990716d1 is whitespace/wrapping only). The other 6 reds on b5814095 reproduce on pristine main at the exact rebase base 6ed29f5cb (Backend shards 2+4: test_kiro_usage_api + test_trust_reads; frontend hiStyle ratchet + Coverage Merge) — main-side breakage, not this PR. Local gates re-run green: black check script, flake8/isort, handler suite 11/11.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Escalating — PR-side work complete on 990716d1; every remaining red is main-side breakage.

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 _open_rb_nofollow + fstat + fd-threaded extraction (also closing the Design review's deferred parity concern for this endpoint), and the docx budget separator off-by-one — each with mutation-style regression tests, plus black formatting on the new test file.

Remaining reds, all reproduced on pristine main at this PR's exact rebase base 6ed29f5cb (main's own latest CI run is red with the same jobs):

Why the review bots show "skipped" on this head: the fork review pipeline (workflow_run) only fires on CI success, so GPT/Opus/Design/UX/First-Principles stamps cannot refresh until main is repaired — mechanically un-greenable from this PR regardless of its contents. GPT's two blockers from 6549f385 are fixed and dispositioned above; a re-review will run automatically once main is green and CI re-passes.

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.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Pushed a40285d0 — re-rebased onto current main now that the main-side breakage blocking this PR has cleared (main CI green at 9cd848be4, 22:14 UTC; the shard-2/shard-4/hiStyle reds from the escalation comment are fixed on main). Zero code changes vs the reviewed 990716d1: the only conflict was error-code-baseline.json main-side drift, regenerated via the official --update. Local gates re-run green: 94 backend tests (doc_parser + office-preview + error-code contract + surface guards), black gate script, isort/flake8/mypy, tsc, vitest FileRenderers 24/24. This should un-strand the fork review lanes (they gate on CI success). Removing needs-human since the blocker is gone; monitoring CI.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Review-ready at a40285d0 — all 63 checks green, PR Readiness passed, MERGEABLE, all 5 review bots PASS (GPT 5.6 / Opus 4.8 / Design / UX / First Principles), 0 unresolved threads. The GPT round-6 blockers (TOCTOU, docx off-by-one) are fixed and dispositioned; the Design fd-parity concern is closed by the same fix (follow-up #4309 remains for the cross-caller migration). Awaiting human review/merge — no further automated work pending.

bolichen97
bolichen97 previously approved these changes Aug 26, 2026
bolichen97
bolichen97 previously approved these changes Aug 26, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved after conflict resolution against the latest main; focused validation passed.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Pushed 0c6eefff — restored the branch to a single clean commit rebased onto current main, replacing the merge commit 84d8a22c.

Why: 84d8a22c merged origin/main into the branch rather than rebasing, which left the PR carrying 2 commits and re-armed the fork review lanes. Both re-armed lanes then returned blocking verdicts (GPT 5.6 Review — changes requested (blocking), First Principles Review — BLOCK — premise concern (advisory)) but published no finding text anywhere — no PR comment, no review, no thread, no annotation — and stayed that way for ~10 hours. A blocking verdict with no reachable finding is not actionable, so this push re-runs the lanes against a clean single-commit head.

Zero feature-code changes. The commit is the previously-reviewed a40285d0 replayed onto main (58 commits ahead). The only conflict was error-code-baseline.json main-side drift, regenerated via the official test/test_error_code_contract.py --update. PR diff is unchanged in shape: +992/-72 across 25 files (files.py, doc_parser.py, the two test files, FileRenderers.tsx, fileReadUrl.ts, 13 locale catalogs, baseline).

The GPT round-6 blockers from 6549f385 remain fixed and dispositioned above (open-once fd + fstat size gate closing the getsize→open TOCTOU; docx budget separator charged only between paragraphs). No new code was written in this push.

Local gates re-run green on 0c6eefff: 94 backend tests (doc_parser + office-preview + error-code contract + surface guards), black gate script, isort/flake8/mypy, tsc, eslint, lint:i18n, vitest FileRenderers 24/24. 84d8a22c remains recoverable by SHA if anyone needs it.

@iamwhatever

Copy link
Copy Markdown
Collaborator

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: _open_checked_file (src/kiro_crew/dashboard/handlers/files.py:2041) is the validate -> sensitive-path -> is-file -> _open_rb_nofollow -> fstat prefix, split from the whole-read _open_checked (:2140) precisely so an endpoint that must keep the open file object can still share it. Four sites use it, including api_file_sheet (:4052, opening at :4089).

That makes the standing First Principles BLOCK on this PR concrete and mechanical: api_file_office_preview should adopt _open_checked_file instead of carrying its own inlined prefix, and api_file_sheet's _open_and_load (:4075-4096) is the exact template - checked open plus parse in ONE worker-thread hop, with the checked file object never crossing back to the event loop and the parser's with f: closing it on the same thread.

The cross-caller extract_text migration that #4309 also proposed is NOT required here. The opt-in fileobj parameter already added on this branch is the right scope: attachments and knowledge readers keep path-based behaviour, and there is no second caller asking for fd-based opening today.

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>
@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]: Pushed 4e8e5c29 — rebased onto current main (0468a40a, single commit, authorship unchanged) and answered both blocking review lanes with one change.

The fix (GPT BLOCKING + First Principles BLOCK were the same root cause). api_file_office_preview no longer hand-rolls the file-serving security prefix. The inline _validate_dashboard_path → is_sensitive_path → isfile → _open_rb_nofollow → fdopen → fstat-cap sequence is deleted and replaced by the shared _open_checked_file(...), called inside one asyncio.to_thread hop — api_file_sheet's _open_and_load is the template that was followed:

  • One spelling of the boundary again (First Principles): this endpoint becomes the 5th adopter instead of the first hand-rolled copy since consolidation, so a future hardening change to that prefix lands here too.
  • Nothing blocking runs on the event loop (GPT, anchor no-blocking-call-on-event-loop): realpath validation, the sensitive-path screen, the open, the fstat, ZIP+XML parsing and redaction now all run on the worker thread. An NFS/FUSE-backed .docx can no longer stall gateway tasks or the heartbeat.
  • The checked file object never crosses back to the loop: every path that opens it closes it on that thread (refusals close inside the prefix; a with block covers the parse and the format refusal), so a cancellation cannot strand an open fd in a discarded future.

Everything the earlier rounds earned is preserved, deliberately:

  • The 50 MB gate is now expressed as the prefix's fstat_cap=_MAX_UPLOAD_BYTES — still enforced on the fd, still before zipfile materializes the central directory, still an SEL-audited 413 file_too_large.
  • redact() still runs on the FULL extracted text before the 512 K _OFFICE_PREVIEW_CAP slice (the AKIA-straddling-the-boundary test still pins it).
  • except asyncio.CancelledError still records an SEL cancelled outcome and re-raises.
  • SEL denied still records on both cannot_resolve and outside_project via the shared _resolve_project_relative(); every response code is unchanged, so the error-code baseline shows no new codes.
  • The 415 format gate keeps its SEL audit and still reads the validated path's extension — it moved inside the worker hop as an endpoint-local refusal (_PreviewUnsupported, mirroring api_file_sheet's _SheetRefusal) rather than adding an endpoint-specific code to _OpenDenied's shared vocabulary.
  • extract_text's opt-in fileobj stays exactly as scoped — no cross-caller migration, per the note about files.py: share one validate→sensitive→size→SEL envelope and O_NOFOLLOW open across file-content endpoints #4309 above.

One extra fix the rebase surfaced. Main gained doc_parser._vet_archive_inventory (the shared zip-inventory vet from #6501) after this branch was last rebased. Auto-merged, it vetted the path while zipfile parsed the fd — a swapped path between the two reads would have let an over-cap inventory reach the allocation the vet exists to prevent. _vet_archive_inventory now reads the tail from the handle when one is passed, via the vet module's existing vet_zip_inventory_bytes, and rewinds; the path branch is untouched for the attachment and knowledge readers.

Tests. Rewrote test_extraction_reads_through_the_size_gated_fd (it pinned the old call's kwargs shape) as test_extraction_reads_through_the_prefix_fd_and_closes_it, which now also asserts the fd is closed by response time. Added test_open_envelope_is_the_shared_prefix_and_runs_off_the_loop, which pins both halves of the finding: the shared prefix is what runs, and it runs off the event loop. Red-before proven — it fails on the previous head with "the endpoint must use the shared _open_checked_file prefix, not an inline copy".

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, tsc -b, eslint (0 errors), lint:i18n, and vitest on FileRenderers (24 passed). The error-code baseline was regenerated with the official test/test_error_code_contract.py --update, never by hand.

On the First Principles Subtraction (not a Blocker): keeping OFFICE_PREVIEWABLE_EXTS client-side. It is retained deliberately — it avoids a guaranteed-415 round-trip and the loading flash on every .doc/.xls/ODF file in a browsed folder, and the UX round asked for that fallback card to render immediately. The backend 415 remains the authority; the client list is an optimisation over a static, rarely-changing set, and the 415 safety-net branch is tested precisely so list drift degrades correctly rather than silently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants