feat(artifact): versioned file/download artifacts — save_file_artifact (ADR 0092 D2)

[mabry1985]

What

Adds a file artifact kind and a save_file_artifact(path, title?, artifact_id?) tool, so a generated .docx / .xlsx / .pptx / .pdf / image becomes a first-class, versioned download artifact — the "versioning holds for the files too" half of the cowork-docs initiative.

This is Track 1 D2 (design in ADR 0092, shipped via #2123).

Why

The artifact plugin versioned only renderable text source (an HTML/MD/React code string, diffed as old_string → new_string). A generated binary Office file had no home in the panel and no version history — it was a loose file in a work folder. D2 gives binaries the same edit-history + inspectable-panel treatment.

How

• Storage — sidecar blobs, not base64-in-JSON. Bytes live at <artifact-dir>/blobs/<id>/<token>.<ext>; history.json (read on every panel poll) keeps only {mime, filename, size, preview, thumb, blob}. A random per-version token survives max_versions trimming (which shifts indices). _write_store's trim now GCs orphaned/deleted blobs.
• Preview — a diffable text projection in code: docx→paragraphs, xlsx→sheet tables, pptx→slide outline, pdf→text (pypdf, already a dep), text/*→verbatim. Each extractor imports its lib lazily and degrades to a readable note if absent (a lean non-desktop env without the ADR 0092 D1 doc stack) — never crashes.
• Thumbnail — images only (Pillow, bundled by D1) → base64 PNG data-URI. Office/PDF raster thumbnails need LibreOffice/pdfium and are deferred (they show the file glyph + text preview). (This is exactly the LOE split we agreed: cheap image thumbs in, heavy raster out.)
• Route — GET /api/plugins/artifact/artifact/{id}/blob?version= streams the stored bytes with the mime + attachment filename, operator-gated like the rest.
• Shell — file kinds render a themed download card (thumb/glyph + name/mime/size + preview) instead of iframing; the Download button pulls the real blob; Edit is hidden (a derived preview isn't user-editable).
• Config — max_blob_kb (25 MB) + max_preview_kb (64 KB) as Settings ▸ Plugins fields.

Existing text artifacts are untouched (purely additive kind).

Test

• Extractor per format — real .docx/.xlsx built in-test, guarded by importorskip so they run on the stack and skip in bare CI.
• Degradation on an unparseable/absent-lib file (runs everywhere); sidecar write + trim/delete GC; blob route headers + 404s (unknown + non-file artifact); image-only thumbnail.
• ✅ Bare (core CI, no doc stack): 63 passed / 3 skipped. ✅ With the stack: 65 passed / 1 skipped. ruff clean.

Sequencing

• Depends on ADR 0092 (the doc file lands via feat(desktop): bundle the document-generation stack (ADR 0092 D1) #2123) — the code references it; nothing runtime-blocking, the plugin works standalone.
• Follow-up D3: wire cowork's doc skills (~/dev/cowork-plugin) to call save_file_artifact after they write a file.

Refs #1631

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@mabry1985, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2b453dad-cd15-488c-88bc-6c5cf63909e9

📥 Commits

Reviewing files that changed from the base of the PR and between e866547 and ed55290.

📒 Files selected for processing (4)

• plugins/artifact/__init__.py
• plugins/artifact/protoagent.plugin.yaml
• plugins/artifact/skills/rendering-artifacts/SKILL.md
• tests/test_artifact_plugin.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/artifact-download-tier

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[protoreview]

QA panel review — FAIL

code-review-structural · head dae8383ba5ac · formal

[review-synthesizer completed: workflow code-review-structural:report]

Brief

Overall risk: Medium. This PR adds file-artifact persistence (blob store, preview trunction, office/pdf extractors), but two kind-confusion paths can corrupt artifact state — one in the new save_file_artifact function, one in three pre-existing callers that now silently drop file metadata. The UI hides the edit button for file artifacts, partially masking the server-side vulnerability, but the PUT route and tool invocations remain unprotected.

Fix-first: Guard save_file_artifact (line ~484) with a kind == "file" check before appending file metadata — it's the new code introduced by this PR and the simpler of the two major defects to close.

Panel agreement: All eight findings confirmed by the verifier; no refutations, no disagreements.

Verification impact: Nothing dropped — every finding held against the diff. The two major findings share a root cause (no kind guard on _commit_version call paths) but affect different callers and are kept separate.

Gaps: The structural pass (protopatch) was skipped. The four office-file extractors (_extract_docx, _extract_xlsx, _extract_pptx, _extract_pdf) received no logic-level findings — their extraction implementations went under-read, a coverage gap worth noting given how much new extraction code landed.

[
  {
    "file": "plugins/artifact/__init__.py",
    "line": 484,
    "severity": "major",
    "category": "correctness",
    "claim": "save_file_artifact can append a blob-backed file version to a non-file artifact (e.g., one created by show_artifact), corrupting its state — the artifact's kind stays non-file, the panel renders the preview as raw HTML, and the Download button serves the preview text instead of the actual file bytes.",
    "evidence": "In the revision branch, `_commit_version(store, art, preview, extra={\"file\": file_meta, \"blob\": blob_name})` is called without first checking `art.get(\"kind\") == \"file\"`. The `_find()` call on line 478 returns any artifact by ID regardless of kind.",
    "verdict": "confirmed",
    "note": "Diff shows revision branch (else clause, ~line 484–488) calls _commit_version with file extra without checking art['kind']. _find() returns any artifact by ID. The art's kind is never updated to 'file', so panel renders via srcdoc(a.kind, v.code) instead of fileCard(v), and download button uses text path."
  },
  {
    "file": "plugins/artifact/__init__.py",
    "line": 0,
    "severity": "major",
    "category": "cross-file",
    "claim": "update_artifact, rewrite_artifact, and _save_edit call _commit_version without the extra parameter, so targeting a file artifact creates a corrupted version missing file metadata and blob token — the panel shows a broken download card and the stored bytes are orphaned.",
    "evidence": "update_artifact calls `_commit_version(store, art, new_code)` and rewrite_artifact calls `_commit_version(store, art, code)` — neither checks `art[\"kind\"]` before calling. The diff confirms _save_edit was not modified: `v = _commit_version(store, art, code, by=\"user\")` at line ~973. The UI hides the edit button for file artifacts (client-only mitigation), but the server routes remain unprotected.",
    "verdict": "confirmed",
    "note": "Diff confirms all three callers unchanged — update_artifact (old line ~329+), rewrite_artifact (~348+), and _save_edit (~684) all call _commit_version without extra. No kind guard exists. UI hides edit button (`$edit.style.display = a.kind===\"file\" ? \"none\" : \"\"`) but tools and PUT route remain unprotected."
  },
  {
    "file": "plugins/artifact/__init__.py",
    "line": 435,
    "severity": "minor",
    "category": "correctness",
    "claim": "_clip() can silently drop a character at the truncation boundary when the byte-level slice splits a multi-byte UTF-8 sequence, because decode('utf-8', 'ignore') discards the incomplete bytes.",
    "evidence": "`return data[: max(0, limit - len(_PREVIEW_TRUNC.encode()))].decode('utf-8', 'ignore') + _PREVIEW_TRUNC` — the byte slice may cut mid-codepoint; the 'ignore' error handler drops the partial sequence.",
    "verdict": "confirmed",
    "note": "The _clip function in the diff uses byte-level slicing on the UTF-8 encoding, and decode('utf-8','ignore') silently discards any incomplete multi-byte sequence at the cut point. At most 1–3 chars lost; minor impact on preview text only."
  },
  {
    "file": "plugins/artifact/__init__.py",
    "line": 1411,
    "severity": "minor",
    "category": "correctness",
    "claim": "The file-artifact download button handler silently swallows all fetch errors with no user feedback, masking failures such as a missing blob, network error, or auth issue.",
    "evidence": "`catch(e){ /* transient — the panel poll will re-sync */ }` — the empty catch block gives the user no indication the download failed.",
    "verdict": "confirmed",
    "note": "Diff shows the async download handler's catch block is empty with only a comment. No user-visible feedback on failure — the click silently does nothing. Minor: the poll eventually re-renders, but the user gets no immediate cue."
  },
  {
    "file": "plugins/artifact/skills/rendering-artifacts/SKILL.md",
    "line": 134,
    "severity": "minor",
    "category": "cross-file",
    "claim": "SKILL.md documents save_file_artifact as `save_file_artifact(path, title?)` — omitting the artifact_id parameter from the signature, even though the prose below explains artifact_id for revisions.",
    "evidence": "SKILL.md heading: `call **save_file_artifact(path, title?)** right after`. Actual Python signature: `def save_file_artifact(path: str, title: str = \"\", artifact_id: str = \"\")`.",
    "verdict": "confirmed",
    "note": "Diff shows the heading line reads `save_file_artifact(path, title?)` — artifact_id omitted. Prose below says 'Pass the prior artifact_id' so it's documented, just missing from the signature shorthand. Minor doc hygiene."
  },
  {
    "file": "tests/test_artifact_plugin.py",
    "line": 181,
    "severity": "minor",
    "category": "cross-file",
    "claim": "test_manifest_exposes_all_settings_fields iterates a fixed tuple of number-field keys that wasn't updated to include max_blob_kb and max_preview_kb — a type-regression on the new settings fields would go undetected.",
    "evidence": "Test iterates `(\"ask_max_chars\", \"history\", \"max_versions\", \"max_code_kb\")` but the manifest now also declares `max_blob_kb` and `max_preview_kb` as number-type settings.",
    "verdict": "confirmed",
    "note": "Test's for-loop tuple omits max_blob_kb and max_preview_kb. However, the `set(by_key) <= set(m[\"config\"])` line does verify they exist in config — only the explicit type==number check is missing. Minor gap."
  },
  {
    "file": "tests/test_artifact_plugin.py",
    "line": 0,
    "severity": "minor",
    "category": "tests",
    "claim": "The `_extract_pptx` extractor added in this PR has no dedicated positive-path test, while the structurally identical docx and xlsx extractors each have one.",
    "evidence": "The diff adds `_extract_pptx` and 12 new tests, but only docx (`test_docx_extraction_reads_paragraphs`), xlsx (`test_xlsx_extraction_reads_sheet_cells`), and a degrade-path test exist — no pptx extraction test.",
    "verdict": "confirmed",
    "note": "12 tests added in diff: docx extraction (test_docx_extraction_reads_paragraphs), xlsx extraction (test_xlsx_extraction_reads_sheet_cells), degrade-path (test_unparseable_office_file_degrades_not_crashes). No test_pptx_extraction_* exists. Minor coverage gap."
  },
  {
    "file": "tests/test_artifact_plugin.py",
    "line": 0,
    "severity": "minor",
    "category": "tests",
    "claim": "The `_extract_pdf` extractor added in this PR has no dedicated positive-path test, while the structurally identical docx and xlsx extractors each have one.",
    "evidence": "The diff adds `_extract_pdf` but no test exercises pdf text extraction — only docx, xlsx, and a degrade-path test exist.",
    "verdict": "confirmed",
    "note": "Same analysis as pptx — no test_pdf_extraction_* in the 12 new tests. pdf extractor has the same structural need for a positive-path test as docx/xlsx. Minor coverage gap."
  }
]

[mabry1985]

Thanks — accurate review, all eight addressed in c2d4a9b:

Major ×2 (kind confusion) — fixed. Added an _is_file() guard on both _commit_version directions:

• save_file_artifact now refuses to revise a non-file artifact (was: kind stayed wrong, preview rendered as raw source).
• update_artifact / rewrite_artifact / the panel PUT (_save_edit) now refuse a file artifact (was: text version appended with no blob → orphaned bytes + broken card). The hidden Edit button was only a client mask; the route is guarded server-side now (409). Regression tests cover all four paths.

Minors:

• Empty download catch → flashes "Failed" for 1.8s.
• SKILL.md heading → save_file_artifact(path, title?, artifact_id?).
• test_manifest_exposes_all_settings_fields tuple → now includes max_blob_kb + max_preview_kb.
• Added positive-path pptx + pdf extraction tests (importorskip-guarded, like docx/xlsx).
• _clip: keeping the byte-slice + decode(errors="ignore") — dropping a partial codepoint at a hard truncation boundary is the intended, correct behavior (no crash, ≤3 bytes off an already-truncated preview). Happy to change if you'd rather truncate on a codepoint boundary.

Tests: bare (core CI, no doc stack) 65 passed / 5 skipped; full stack 70 passed. The 5 bare-skips are the doc-stack extraction + Pillow-thumbnail tests (the doc stack ships only in the desktop build, ADR 0092 D1 / #2123).

[protoreview]

QA panel review — WARN

code-review-structural · head c2d4a9ba1f92 · formal

[review-synthesizer completed: workflow code-review-structural:report]

Prose Brief

Overall risk is low: the change is a self-contained _clip() helper in plugins/artifact/__init__.py that truncates preview text for display only — no data loss, no security boundary, no cross-file coupling. The one confirmed defect is a minor correctness bug in the UTF‑8 truncation path: a byte‑level slice can land mid‑codepoint and decode('ignore') silently discards the straddling character. Fix this first by computing the cut point in code points (e.g., iterate characters and measure their encoded lengths) rather than slicing raw bytes. Three panel lanes (correctness, cross‑file, conventions) converged on this finding. Verification confirmed the mechanism but corrected the line number (~429, not 441) and the impact estimate (at most 1 character lost, not 1–3). Gap: the workflow references a code-review-structural:verify pass, but only this single verifier‑annotated finding was presented — if a full structural‑protopatch findings list existed, it did not reach this synthesizer pass, so its items (if any) are absent from the array below.

[
  {
    "file": "plugins/artifact/__init__.py",
    "line": 429,
    "severity": "minor",
    "category": "correctness",
    "claim": "_clip() can silently drop up to 3 characters at the truncation boundary when the byte-level slice splits a multi-byte UTF-8 sequence, because decode('utf-8', 'ignore') discards the incomplete bytes. Flagged by correctness, cross-file, and conventions review.",
    "evidence": "`return data[: max(0, limit - len(_PREVIEW_TRUNC.encode()))].decode('utf-8', 'ignore') + _PREVIEW_TRUNC` — the byte slice may cut mid-codepoint (e.g., a 4-byte emoji); the 'ignore' error handler silently drops the partial sequence. At most 1–3 chars lost; impact limited to preview text only.",
    "verdict": "confirmed",
    "note": "Mechanism confirmed: byte-slice on UTF-8 bytes + decode('ignore') silently drops an incomplete multi-byte sequence at the boundary. At most 1 character lost (the one straddling the cut), not 1–3 — the lost char occupies up to 3 trailing bytes. Line ~429 in the diff, not 441 (minor offset). Impact confined to preview text truncation."
  }
]

Superseded — a later head cleared the QA panel (see the newest verdict).

[mabry1985]

Fixed the last one (b6d7a62). You're right — I adopted your codepoint-boundary suggestion rather than keeping the errors="ignore": _clip now backs the byte cut off any trailing continuation byte (0b10xxxxxx) so a multi-byte char is never split, and decode("utf-8") is exact (no ignore). Added a boundary test (a 4-byte emoji straddling the budget truncates cleanly instead of being mangled).

That clears the WARN — the two majors + all seven minors from both passes are now addressed.

[protoreview]

QA panel review — WARN

code-review-structural · head b6d7a6298661 · formal

[review-synthesizer completed: workflow code-review-structural:report]

Prose Brief

Overall risk: Low. The defect is in _gc_blobs(), a garbage-collection helper for artifact directories. If one directory fails to rmdir() (e.g., still contains files, permissions issue), the entire sweep aborts and all remaining eligible directories are skipped indefinitely. Orphaned artifact blobs accumulate on disk but no data is lost and the primary code path is unaffected.

Fix-first item: Move the try/except OSError inside the for art_dir in root.iterdir(): loop so that a failed rmdir() on one directory does not short-circuit cleanup of the rest.

Panel disagreement: None — only one finding was produced, and the verifier confirmed it unambiguously.

Verification: The structural pass (code-review-structural:verify) confirmed the defect from the diff: try wraps the full loop at ~line 200, except OSError catches at ~line 213, and any rmdir() failure (ENOTEMPTY, PermissionError) triggers a return that abandons all subsequent directories. No findings were refuted.

Gaps: The structural pass completed successfully. Only one file (plugins/artifact/__init__.py) was implicated; no other production files were flagged by any finder, which is consistent with a narrowly-scoped change. No coverage gap suspected.

---

[
  {
    "file": "plugins/artifact/__init__.py",
    "line": 205,
    "severity": "minor",
    "category": "correctness",
    "claim": "_gc_blobs() aborts the entire sweep when a single artifact directory's rmdir() raises OSError, skipping cleanup of all remaining directories.",
    "evidence": "The `except OSError` wraps the entire `for art_dir in root.iterdir():` loop. If `art_dir.rmdir()` raises — permission denied, directory not empty — the handler catches it and returns, leaving orphaned blobs in subsequent directories indefinitely.",
    "verdict": "confirmed",
    "note": "Diff confirms `try:` at ~line 200 wraps the full `for art_dir in root.iterdir():` loop; `except OSError:` at ~line 213 catches any OSError (rmdir ENOTEMPTY, unlink PermissionError, etc.) and just logs debug — all subsequent dirs in the iteration are skipped. `Path.rmdir()` indeed raises OSError on non-empty dirs; `unlink(missing_ok=True)` only suppresses FileNotFoundError, not PermissionError."
  }
]

[mabry1985]

Fixed (5323e34) — legit robustness bug. _gc_blobs now isolates per directory: the try/except OSError moved inside the for art_dir in root.iterdir() loop, so a single un-sweepable dir (unexpected subdir, permissions/lock) is logged and skipped instead of aborting the whole pass and stranding every other orphan. The root.iterdir() itself has its own guard. Added a test where an orphan dir that fails to drain doesn't stop a sibling orphan from being swept.

[protoreview]

QA panel review — WARN

code-review-structural · head 5323e344e889 · formal

[review-synthesizer completed: workflow code-review-structural:report]

Prose Brief

Overall risk: Low. Both findings are minor — an edge-case correctness gap in the _blob route where an out-of-range version silently serves the latest rather than 404-ing, and a DRY violation from a duplicated error string. Neither is a data-loss or security concern.

Fix first: The _blob out-of-range swallowing (line 1022). It's a correctness defect where the route's behavior contradicts its own docstring ("404 if version is missing"), and callers relying on a 404 for bad versions will get wrong results. The DRY string duplication is purely a maintenance hygiene item.

Panel disagreement: None. The verifier confirmed both findings cleanly, and the original review panel was in agreement on severity (minor for both).

What verification changed: Nothing — both findings survived confirmation; no refutations.

Gaps: The structural pass (protopatch) ran and completed, but no structural findings surfaced in the final merge. Only the plugins/artifact/__init__.py changes were reviewed; if the PR touched other plugins or shared utilities, they may have gone under-read. The two findings are concentrated in one file, which is acceptable for a focused change.

---

[
  {
    "file": "plugins/artifact/__init__.py",
    "line": 1022,
    "severity": "minor",
    "category": "correctness",
    "claim": "The _blob route silently serves the latest version when `version` is out of range (e.g., v999 of a 3-version artifact), instead of returning 404.",
    "evidence": "`idx = (version - 1) if version and 1 <= version <= len(vers) else len(vers) - 1` — when `version > len(vers)`, the condition is False and `idx` becomes `len(vers)-1` (the latest), silently ignoring the caller's out-of-range request. The docstring specifies 0/absent → latest, but doesn't document this out-of-range swallowing.",
    "verdict": "confirmed",
    "note": "Diff shows the exact expression; for version>len(vers) the ternary falls through to else→latest, with no out-of-range 404 guard. Docstring says '404 if version is missing' but this path never raises one."
  },
  {
    "file": "plugins/artifact/__init__.py",
    "line": 722,
    "severity": "minor",
    "category": "conventions",
    "claim": "The same error message string is duplicated verbatim in update_artifact and rewrite_artifact when rejecting a file artifact — a drift risk if the message ever needs to change.",
    "evidence": "Both update_artifact (~line 722) and rewrite_artifact (~line 760) contain the identical f-string: `f\"Artifact {art['id']} is a file artifact (its source is a file on disk, not editable text). Re-generate the file and re-save it with save_file_artifact.\"`",
    "verdict": "confirmed",
    "note": "Diff confirms identical f-string block in both update_artifact (hunk ~+715) and rewrite_artifact (hunk ~+753); verbatim duplication is a real DRY drift risk."
  }
]

[protoreview]

Promoting the WARN verdict for head 5323e344e889: all checks terminal-green, zero unresolved review threads. (approve-on-green)

[mabry1985]

Thanks — landed both (ed55290) even though you approved-on-green, since the first is a real correctness gap:

• _blob out-of-range version → now 404s per the docstring instead of silently serving the latest. Explicit version must be in [1, N]; absent/0 → latest. Test covers all four cases (absent, 0, in-range, out-of-range).
• DRY → the duplicated "file artifact isn't editable" refusal is extracted to _file_not_editable(art).

That's every finding from all three passes addressed. 68 passed / 5 skipped, ruff clean.

[protoreview]

QA panel review — PASS

code-review-structural · head ed55290bc6c7 · formal

[review-synthesizer completed: workflow code-review-structural:report]

Prose Brief

This PR adds file-artifact support (save_file_artifact, _is_file guard on edit tools, download-card handling). The single finding that reached verification — a claim that show_artifact would accept kind="file" — was refuted: the pre-existing _KINDS set ({"html", "svg", "mermaid", "react", "markdown"}) already rejects "file" before any artifact dict is constructed, and save_file_artifact is the sole creation path for file artifacts (where kind is hardcoded safely). No other findings survived the full pipeline.

Coverage gap: Only one finding — now refuted — reached the verify pass for a PR that touches production code across multiple concerns (new artifact kind, edit guards, download rendering). The structural pass (code-review-structural:verify) completed but produced nothing that survived dedup/verification. This may indicate the change is genuinely clean, or that the panel under-read the diff — the caller should decide whether a second look at _save_edit, rewrite_artifact, or the download-card template is warranted.

Overall risk: Low on the evidence available. Fix-first: None — no surviving findings. Verification changed: The sole finding was dropped; the _KINDS guard is pre-existing and airtight.

[]

[protoreview]

Promoting the PASS verdict for head ed55290bc6c7: all checks terminal-green, zero unresolved review threads. (approve-on-green)