From dae8383ba5ac4936d5c9b287d33fe47b2409a51e Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Tue, 21 Jul 2026 23:05:37 -0700 Subject: [PATCH 1/5] =?UTF-8?q?feat(artifact):=20versioned=20file/download?= =?UTF-8?q?=20artifacts=20=E2=80=94=20save=5Ffile=5Fartifact=20(ADR=200092?= =?UTF-8?q?=20D2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The artifact plugin versioned only renderable TEXT source (an HTML/MD/React `code` string) — so a generated .docx/.xlsx/.pptx/.pdf had no home in the panel and no version history. Add a `file` artifact kind and a save_file_artifact(path, title, artifact_id?) tool so a generated file becomes a first-class, versioned download artifact. - Storage: bytes live as SIDECAR blobs under /blobs//. (NOT base64-inlined — history.json is read on every panel poll). A random blob token per version 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 get a base64 PNG thumbnail via Pillow (bundled by D1); Office/PDF raster thumbnails need LibreOffice/pdfium and are deferred — they show the file glyph + text preview. - 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), Settings ▸ Plugins fields. Existing text artifacts are untouched (additive kind). Tests: extractor per format (real docx/xlsx guarded by importorskip), degradation on unparseable/absent-lib, sidecar write + trim/delete GC, blob route headers + 404s, image-only thumbnail. Suite passes bare (core CI, no doc stack) and with the stack installed. Depends on ADR 0092 (lands via #2123). Follow-up D3: wire cowork's doc skills to call save_file_artifact. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --- plugins/artifact/__init__.py | 377 +++++++++++++++++- plugins/artifact/protoagent.plugin.yaml | 10 +- .../skills/rendering-artifacts/SKILL.md | 17 + tests/test_artifact_plugin.py | 186 +++++++++ 4 files changed, 577 insertions(+), 13 deletions(-) diff --git a/plugins/artifact/__init__.py b/plugins/artifact/__init__.py index 4932869d7..f7f96472e 100644 --- a/plugins/artifact/__init__.py +++ b/plugins/artifact/__init__.py @@ -5,7 +5,10 @@ edit) or ``rewrite_artifact`` (a full replacement) — the Claude "update vs rewrite" model, so an artifact is a VERSION CHAIN you can step back through, not a flood of near-duplicates. ``list_artifacts`` / ``get_artifact`` (read the current source — how you take over an artifact you -didn't author) / ``delete_artifact`` manage them. The panel is a plugin-served shell page +didn't author) / ``delete_artifact`` manage them. ``save_file_artifact`` (ADR 0092) versions a +generated FILE (docx/xlsx/pptx/pdf/image) as a download artifact — bytes in a sidecar blob, a +diffable text preview in ``code``, an image thumbnail — rendered as a download card, not iframed. +The panel is a plugin-served shell page (iframed by the console, ADR 0026) that renders the generated code in a **nested sandboxed iframe** (``sandbox="allow-scripts"``, no same-origin) — the Claude Artifacts / Open WebUI isolation model: generated code runs, but can't touch the console, its cookies, or its APIs. @@ -116,6 +119,19 @@ def _max_code_bytes() -> int: return _cfg_int("max_code_kb", "ARTIFACT_MAX_CODE_KB", 512) * 1024 +# The binary blob cap for `file` artifacts (ADR 0092 D2) — separate from max_code_kb +# because a real .docx/.xlsx/.pdf is bytes on disk (sidecar file), not source text. +def _max_blob_bytes() -> int: + return _cfg_int("max_blob_kb", "ARTIFACT_MAX_BLOB_KB", 25 * 1024) * 1024 + + +# The extracted-text PREVIEW cap for a `file` version — the diffable projection stored in +# `code` (docx→text, xlsx→sheet table, pptx→outline, pdf→text). Kept well under +# max_code_kb so a huge document can't bloat history.json (read on every panel poll). +def _max_preview_bytes() -> int: + return _cfg_int("max_preview_kb", "ARTIFACT_MAX_PREVIEW_KB", 64) * 1024 + + # Interactive artifacts (window.protoArtifact.ask → the agent). OPT-IN: letting # sandboxed artifact code trigger LLM calls is a cost surface. `ask_enabled` + # `ask_system` are Settings ▸ Plugins fields (manifest `settings:`); ask_max_chars caps. @@ -149,6 +165,59 @@ def _store_path() -> Path: return base / "history.json" +# ── binary blobs (ADR 0092 D2) ─────────────────────────────────────────────── +# A `file` artifact's BYTES live as sidecar files under /blobs//, +# NOT inlined into history.json — the store is read on every panel poll, so a base64 +# .docx would bloat it badly. history.json keeps only {mime, filename, size, preview, +# thumb, blob:"."}; the version's ``blob`` names the sidecar file. The name +# is a random token (not the version index) so trimming to max_versions — which shifts +# indices — never mis-points a version at another version's bytes. + + +def _blob_root() -> Path: + return _store_path().parent / "blobs" + + +def _blob_path(art_id: str, name: str) -> Path: + """The sidecar file for ``name`` (a version's ``blob`` token) under artifact ``art_id``. + ``name`` is sanitized to a bare filename — no path traversal out of the blob dir.""" + safe = os.path.basename(str(name)) + return _blob_root() / art_id / safe + + +def _gc_blobs(store: dict) -> None: + """Delete sidecar blob files/dirs no longer referenced by a surviving version — the + retention sweep that pairs with _write_store's version/history trim. Best-effort: a + filesystem hiccup must never break a store write.""" + root = _blob_root() + if not root.exists(): + return + live: dict[str, set[str]] = {} + for a in store.get("artifacts", []): + names = { + v["blob"] + for v in a.get("versions", []) + if isinstance(v.get("blob"), str) and v["blob"] + } + if names: + live[a["id"]] = names + try: + for art_dir in root.iterdir(): + if not art_dir.is_dir(): + continue + keep = live.get(art_dir.name) + if keep is None: # artifact gone (deleted / trimmed out) → drop its whole dir + for f in art_dir.iterdir(): + f.unlink(missing_ok=True) + art_dir.rmdir() + continue + for f in art_dir.iterdir(): # artifact lives; drop only orphaned versions' blobs + if f.name not in keep: + f.unlink(missing_ok=True) + except OSError: + log.debug("[artifact] blob GC hiccup", exc_info=True) + + def _now() -> int: return int(time.time() * 1000) @@ -208,6 +277,7 @@ def _write_store(store: dict) -> None: finally: if os.path.exists(tmp): os.unlink(tmp) + _gc_blobs(store) # drop sidecar blobs orphaned by the version/history trim above def _find(store: dict, art_id: str | None) -> dict | None: @@ -244,16 +314,23 @@ def _emit(event: str, data: dict) -> None: log.debug("[artifact] emit(%s) failed", event, exc_info=True) -def _new_version(code: str, by: str = "agent") -> dict: - """A fresh version record. ``by`` is provenance: "agent" (a tool) or "user" (panel edit).""" - return {"code": code, "ts": _now(), "by": by} +def _new_version(code: str, by: str = "agent", extra: dict | None = None) -> dict: + """A fresh version record. ``by`` is provenance: "agent" (a tool) or "user" (panel edit). + ``extra`` merges in kind-specific fields — for a `file` version, ``file`` metadata + ({mime, filename, size, thumb}) and the ``blob`` sidecar token (ADR 0092 D2).""" + v = {"code": code, "ts": _now(), "by": by} + if extra: + v.update(extra) + return v -def _commit_version(store: dict, art: dict, code: str, by: str = "agent") -> int: +def _commit_version( + store: dict, art: dict, code: str, by: str = "agent", extra: dict | None = None +) -> int: """Append a version to ``art``, move it to the front, persist, broadcast ``updated``, and return the new 1-based version count. The shared tail of update/rewrite_artifact + the panel's user-edit PUT — one place owns append→touch→write→emit ordering.""" - nv = _new_version(code, by) + nv = _new_version(code, by, extra) art["versions"].append(nv) art["updated"] = nv["ts"] _touch(store, art) @@ -329,6 +406,218 @@ def _render_suffix(art_id: str, version: int) -> str: ) +# ── file artifacts (ADR 0092 D2) ───────────────────────────────────────────── +# save_file_artifact turns a generated file (docx/xlsx/pptx/pdf/image/text) into a +# VERSIONED artifact: bytes stored as a sidecar blob, a diffable text PREVIEW extracted +# into `code`, and — for images only — a base64 thumbnail (the cheap win; rasterizing +# Office/PDF pages needs LibreOffice/pdfium and is deferred). Every extractor imports its +# lib lazily and degrades to a note if it's missing (a lean non-desktop env without the +# doc stack), so the tool never hard-fails — it just stores the blob with a thin preview. +_PREVIEW_TRUNC = "\n… (preview truncated — download the file for the full content)" + + +def _guess_mime(path: Path) -> str: + import mimetypes + + mime, _ = mimetypes.guess_type(str(path)) + return mime or "application/octet-stream" + + +def _clip(text: str) -> str: + """Clip an extracted preview to _max_preview_bytes (utf-8), with a truncation note.""" + limit = _max_preview_bytes() + data = (text or "").encode("utf-8") + if len(data) <= limit: + return text or "" + return data[: max(0, limit - len(_PREVIEW_TRUNC.encode()))].decode( + "utf-8", "ignore" + ) + _PREVIEW_TRUNC + + +def _extract_docx(path: Path) -> str: + from docx import Document # python-docx + + doc = Document(str(path)) + return "\n".join(p.text for p in doc.paragraphs if p.text.strip()) + + +def _extract_xlsx(path: Path) -> str: + from openpyxl import load_workbook + + wb = load_workbook(str(path), read_only=True, data_only=True) + out: list[str] = [] + for ws in wb.worksheets: + out.append(f"# {ws.title}") + for r, row in enumerate(ws.iter_rows(values_only=True)): + if r >= 200: # cap rows per sheet — a preview, not the whole workbook + out.append("… (more rows — download for all)") + break + out.append( + ", ".join("" if c is None else str(c) for c in (row or ())[:50]) + ) + wb.close() + return "\n".join(out) + + +def _extract_pptx(path: Path) -> str: + from pptx import Presentation # python-pptx + + prs = Presentation(str(path)) + out: list[str] = [] + for i, slide in enumerate(prs.slides, 1): + out.append(f"── Slide {i} ──") + for shape in slide.shapes: + if shape.has_text_frame and shape.text_frame.text.strip(): + out.append(shape.text_frame.text) + return "\n".join(out) + + +def _extract_pdf(path: Path) -> str: + from pypdf import PdfReader # already a core dep (pyproject) + + reader = PdfReader(str(path)) + out: list[str] = [] + for i, page in enumerate(reader.pages): + if i >= 50: # cap pages + out.append("… (more pages — download for all)") + break + out.append(page.extract_text() or "") + return "\n".join(out) + + +# ext → (extractor, degrade-note). Text-ish kinds decode verbatim in _extract_preview. +_EXTRACTORS = { + ".docx": (_extract_docx, "python-docx"), + ".xlsx": (_extract_xlsx, "openpyxl"), + ".pptx": (_extract_pptx, "python-pptx"), + ".pdf": (_extract_pdf, "pypdf"), +} +_TEXT_EXT = {".txt", ".md", ".markdown", ".csv", ".json", ".log", ".yaml", ".yml"} + + +def _extract_preview(path: Path, data: bytes, mime: str) -> str: + """A readable, diffable text projection of the file, capped. Office/PDF via their lib + (lazy, degrades if absent); text/* decoded verbatim; anything else → a size note.""" + ext = path.suffix.lower() + if ext in _EXTRACTORS: + fn, lib = _EXTRACTORS[ext] + try: + return _clip(fn(path)) + except Exception: # noqa: BLE001 — missing lib / corrupt file → thin preview, never crash + log.debug("[artifact] %s preview extract failed", ext, exc_info=True) + return ( + f"({ext[1:].upper()} file — no text preview: {lib} unavailable or the file " + f"couldn't be parsed. Download it for the full content.)" + ) + if mime.startswith("text/") or ext in _TEXT_EXT: + return _clip(data.decode("utf-8", "replace")) + if mime.startswith("image/"): + return f"(image · {mime})" + return f"(binary file · {mime} · {len(data)} bytes — download to open)" + + +def _thumbnail(data: bytes, mime: str) -> str | None: + """A small base64 PNG data-URI thumbnail for IMAGE files only (Pillow, bundled on the + desktop runtime via ADR 0092 D1). Office/PDF thumbnails need a rasterizer we don't ship + — those show the file icon + text preview instead. Returns None on any failure.""" + if not mime.startswith("image/"): + return None + try: + import base64 + from io import BytesIO + + from PIL import Image + + im = Image.open(BytesIO(data)) + im.thumbnail((320, 320)) + if im.mode not in ("RGB", "RGBA"): + im = im.convert("RGBA") + buf = BytesIO() + im.save(buf, format="PNG") + return "data:image/png;base64," + base64.b64encode(buf.getvalue()).decode() + except Exception: # noqa: BLE001 — Pillow absent (lean env) / undecodable → no thumb + log.debug("[artifact] thumbnail failed", exc_info=True) + return None + + +@tool +def save_file_artifact(path: str, title: str = "", artifact_id: str = "") -> str: + """Save a GENERATED FILE (a .docx / .xlsx / .pptx / .pdf / image / text file you already + wrote to disk) into the Artifact panel as a VERSIONED download artifact — so the file gets + the same edit-history + inspectable panel treatment as a rendered artifact, and the user + can download it or diff it across versions. + + Use this right AFTER a skill writes a document to disk (e.g. cowork's docx/xlsx/pptx/pdf + skills, a generated report or image): pass the file ``path``. The panel stores the bytes, + shows a download card with a readable text preview (docx→text, xlsx→sheet table, + pptx→slide outline, pdf→text; images get a thumbnail), and offers a Download button. + + ``title`` is an optional label. To save a NEW revision of a file you saved before (so it + becomes v2, v3… of the same artifact rather than a new panel entry), pass that artifact's + ``artifact_id`` (see ``list_artifacts``). Returns the artifact id. + + This is for FILES on disk. To render HTML/SVG/React/Markdown/charts, use ``show_artifact``. + """ + p = Path(os.path.expanduser(path or "")).resolve() + if not p.exists() or not p.is_file(): + return f"No file at {path!r}. Write the file first, then pass its path." + data = p.read_bytes() + limit = _max_blob_bytes() + if len(data) > limit: + return ( + f"File too large ({len(data) // 1024} KB > {limit // 1024} KB). Raise the artifact " + f"max_blob_kb setting if you really need to store a file this big." + ) + mime = _guess_mime(p) + preview = _extract_preview(p, data, mime) + thumb = _thumbnail(data, mime) + ext = p.suffix.lower().lstrip(".") or "bin" + + store = _read_store() + art = _find(store, artifact_id) if artifact_id else None + if artifact_id and art is None: + return f"No artifact {artifact_id!r} to revise. Use list_artifacts, or omit artifact_id for a new one." + + art_id = art["id"] if art else _new_id() + blob_name = f"{secrets.token_hex(8)}.{ext}" + blob_file = _blob_path(art_id, blob_name) + blob_file.parent.mkdir(parents=True, exist_ok=True) + blob_file.write_bytes(data) + + file_meta = { + "mime": mime, + "filename": p.name, + "size": len(data), + "thumb": thumb or "", + } + if art is None: + nv = _new_version(preview, extra={"file": file_meta, "blob": blob_name}) + art = { + "id": art_id, + "title": title or p.name, + "kind": "file", + "versions": [nv], + "created": nv["ts"], + "updated": nv["ts"], + } + store["artifacts"].insert(0, art) + store["current"] = art_id + _write_store(store) + _emit("created", {"id": art_id, "kind": "file", "title": art["title"]}) + v = 1 + else: + if title: + art["title"] = title + v = _commit_version( + store, art, preview, extra={"file": file_meta, "blob": blob_name} + ) + kb = len(data) // 1024 + return ( + f"Saved file artifact {art_id} → v{v}: {p.name} ({mime}, {kb} KB) — now in the " + f"Artifact panel with a preview and a Download button." + ) + + @tool def show_artifact(kind: str, code: str, title: str = "") -> str: """CREATE a new generative-UI artifact in the console's Artifact panel. @@ -684,6 +973,35 @@ async def _save_edit(art_id: str, body: dict = Body(...)) -> dict: v = _commit_version(store, art, code, by="user") return {"ok": True, "id": art_id, "version": v} + @router.get("/artifact/{art_id}/blob") + async def _blob(art_id: str, version: int = 0): + """Serve a `file` artifact version's stored BYTES for download (ADR 0092 D2). The + panel's Download button hits this with the operator bearer; ``version`` is 1-based + (0/absent = latest). Returns the sidecar blob with its stored mime + an attachment + filename. 404 if the artifact/version/blob is missing or isn't a file artifact.""" + from fastapi.responses import FileResponse + + store = _read_store() + art = _find(store, art_id) + if art is None: + raise HTTPException(404, f"unknown artifact {art_id}") + vers = art.get("versions") or [] + idx = (version - 1) if version and 1 <= version <= len(vers) else len(vers) - 1 + if idx < 0: + raise HTTPException(404, "no versions") + v = vers[idx] + blob_name, meta = v.get("blob"), v.get("file") or {} + if not blob_name: + raise HTTPException(404, "not a file artifact / no stored blob") + f = _blob_path(art_id, blob_name) + if not f.exists(): + raise HTTPException(404, "blob missing") + return FileResponse( + f, + media_type=meta.get("mime") or "application/octet-stream", + filename=meta.get("filename") or f.name, + ) + @router.delete("/artifact/{art_id}") async def _delete(art_id: str) -> dict: """Delete an artifact (the panel's trash button). Gated like the rest.""" @@ -707,6 +1025,7 @@ def register(registry) -> None: _REGISTRY = registry for t in ( show_artifact, + save_file_artifact, update_artifact, rewrite_artifact, list_artifacts, @@ -982,6 +1301,33 @@ def register(registry) -> None: 'document.getElementById("md").innerHTML = marked.parse(decodeURIComponent(escape(atob("' + b64 + '"))));' + mmRun + '<\/script>'; } + // `file` artifacts (ADR 0092 D2) don't iframe generated code — they show a static, + // themed DOWNLOAD CARD: thumbnail (images) or a file glyph, name + mime + size, and the + // extracted text preview (v.code) in a scroll box. Self-contained srcdoc (no scripts, so + // the sandbox stays inert); tokens read from the live theme like base(). + function fmtSize(n){ n=+n||0; return n<1024?n+" B":n<1048576?(n/1024).toFixed(1)+" KB":(n/1048576).toFixed(1)+" MB"; } + function fileCard(v){ + var cs=getComputedStyle(document.documentElement); + function tok(n,d){ return (cs.getPropertyValue(n)||d).trim(); } + var bg=tok("--pl-color-bg","#0a0a0c"), fg=tok("--pl-color-fg","#ededed"), + muted=tok("--pl-color-fg-muted","#9aa0aa"), border=tok("--pl-color-border","rgba(255,255,255,.12)"); + var f=v.file||{}, name=f.filename||"file", mime=f.mime||"application/octet-stream"; + var thumb = f.thumb + ? '' + : '
📄
'; + return '
'+thumb + + '
'+esc(name)+'
'+esc(mime)+' · '+fmtSize(f.size)+'
' + + '
Preview
'+esc(v.code||"")+'
'; + } var $art=document.getElementById("art"), $vprev=document.getElementById("vprev"), $vnext=document.getElementById("vnext"), $vlabel=document.getElementById("vlabel"), $dl=document.getElementById("dl"), $del=document.getElementById("del"), @@ -1023,8 +1369,10 @@ def register(registry) -> None: $vlabel.textContent="v"+(vi+1)+"/"+a.versions.length; $vprev.disabled = vi<=0; $vnext.disabled = vi>=a.versions.length-1; $empty.style.display="none"; + $edit.style.display = a.kind==="file" ? "none" : ""; // a file's preview isn't user-editable var key=a.id+"@"+vi; // re-srcdoc only when the shown version actually changes - if(key!==lastRendered){ lastRendered=key; renderingId=a.id; renderingVer=vi+1; $frame.srcdoc=srcdoc(a.kind, v.code); $frame.style.display="block"; } + if(key!==lastRendered){ lastRendered=key; renderingId=a.id; renderingVer=vi+1; + $frame.srcdoc = a.kind==="file" ? fileCard(v) : srcdoc(a.kind, v.code); $frame.style.display="block"; } } // Live re-theme (#1872): base() bakes the theme tokens into the srcdoc as literal @@ -1053,11 +1401,18 @@ def register(registry) -> None: $vnext.addEventListener("click", function(){ var a=selArt(); if(!a)return; var vi=verIdx(a); if(vi- WebUI do it. React artifacts can import a curated offline set (d3, chart.js, lucide) and the protoLabs design-system @pl/ui wrappers. The shell page is served by this plugin and iframed; the agent's generated code renders in a nested sandbox with no access to the console. + It also versions generated FILES (ADR 0092): save_file_artifact stores a .docx/.xlsx/.pptx/ + .pdf/image as a download artifact with a diffable text preview and (for images) a thumbnail. # Bundled into core (protoAgent #1443) and ON by default — a first-party generative-UI # surface (like notes/docs). Turn it off per-instance with `plugins: { disabled: [artifact] }`. # Pulled in-tree so it ships with the agent and iterates in the monorepo. @@ -19,7 +21,7 @@ capabilities: # Truly no network: the server makes no outbound calls, and the react/mermaid libs # are VENDORED + served same-origin, so artifacts render fully offline (no cdnjs). network: [] - filesystem: scoped # a small artifact history under the instance data dir + filesystem: scoped # artifact history + sidecar file blobs under the instance data dir # Operator config (ADR 0019) — `config:` declares the defaults; `settings:` renders the # editable fields in Settings ▸ Plugins (no restart; an env var of the same knob still # overrides for headless/ACP setups, precedence env > UI > default). @@ -29,6 +31,8 @@ config: history: 20 max_versions: 50 max_code_kb: 512 + max_blob_kb: 25600 + max_preview_kb: 64 ask_max_chars: 4000 settings: - { key: ask_enabled, label: "Interactive artifacts", type: bool, description: "Let artifacts call back to the agent via window.protoArtifact.ask() — turns them into mini-apps (game NPCs, tutors, generators). Off by default: it lets artifact code trigger LLM calls." } @@ -37,6 +41,8 @@ settings: - { key: history, label: "Artifacts kept", type: number, description: "How many artifacts to keep in the panel; the oldest is evicted past this." } - { key: max_versions, label: "Versions per artifact", type: number, description: "Max edit-versions kept per artifact (oldest edits trimmed)." } - { key: max_code_kb, label: "Max artifact size (KB)", type: number, description: "Max source size per version; a larger render is rejected." } + - { key: max_blob_kb, label: "Max file size (KB)", type: number, description: "Max size of a file saved via save_file_artifact (stored as a sidecar blob); a larger file is rejected." } + - { key: max_preview_kb, label: "Max file preview (KB)", type: number, description: "Max extracted-text preview kept per file version (the diffable projection shown in the panel)." } # The shell page is PUBLIC (an iframe page-load can't carry a bearer, and the page # derives its slug base from /plugins/…); its data routes are gated under # /api/plugins/artifact (plugin-view rule 2). diff --git a/plugins/artifact/skills/rendering-artifacts/SKILL.md b/plugins/artifact/skills/rendering-artifacts/SKILL.md index 05a849d55..64a04ce52 100644 --- a/plugins/artifact/skills/rendering-artifacts/SKILL.md +++ b/plugins/artifact/skills/rendering-artifacts/SKILL.md @@ -130,6 +130,23 @@ guess; read the error and make the targeted edit. - **`delete_artifact(artifact_id)`** — remove one for cleanup. (The user can also delete from the panel's trash button.) +## Saving a generated FILE (docx / xlsx / pptx / pdf / image) + +When a skill writes a real **file** to disk — a Word doc, a spreadsheet, a slide deck, a PDF, an +image — call **`save_file_artifact(path, title?)`** right after, to put it in the Artifact panel as +a **versioned download artifact**: the bytes are stored, a readable text preview is extracted +(docx→text, xlsx→sheet table, pptx→slide outline, pdf→text; images get a thumbnail), and the panel +shows a download card. Re-saving the same document as a new revision? Pass the prior +`artifact_id` so it becomes v2, v3… of the same artifact instead of a new panel entry. + +```text +# after cowork's docx skill writes /work/report.docx +save_file_artifact("/work/report.docx", "Q3 Report") +``` + +This is for files that already exist on disk. To *render* HTML/React/SVG/Markdown/charts, use +`show_artifact` (above) — don't write those to a file first. + ## Interactive artifacts (calling back to you) `html` and `react` artifacts can call **`window.protoArtifact.ask(prompt)`** — it returns a diff --git a/tests/test_artifact_plugin.py b/tests/test_artifact_plugin.py index 55d92cc0d..ead72a017 100644 --- a/tests/test_artifact_plugin.py +++ b/tests/test_artifact_plugin.py @@ -760,3 +760,189 @@ def test_live_theme_repush_is_wired(monkeypatch, tmp_path): # shell side: observe the kit re-theme + re-push after a fresh srcdoc load. assert "new MutationObserver(pushTheme).observe(document.documentElement" in html assert '$frame.addEventListener("load", pushTheme)' in html + + +# ── file artifacts (ADR 0092 D2): save_file_artifact + sidecar blobs + blob route ─ + + +def test_save_file_artifact_missing_file(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + out = art.save_file_artifact.invoke({"path": str(tmp_path / "nope.docx")}) + assert "No file at" in out + assert _arts(art) == [] + + +def test_save_file_artifact_creates_file_kind_with_blob_and_preview(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + f = tmp_path / "notes.txt" + f.write_text("hello world\nsecond line", encoding="utf-8") + out = art.save_file_artifact.invoke({"path": str(f), "title": "My Notes"}) + assert "Saved file artifact" in out + a = _arts(art)[0] + assert a["kind"] == "file" and a["title"] == "My Notes" + v = a["versions"][0] + # text preview lands verbatim in `code` (diffable); file metadata + blob token alongside. + assert "hello world" in v["code"] + assert v["file"]["filename"] == "notes.txt" and v["file"]["size"] == len(f.read_bytes()) + assert v["file"]["mime"].startswith("text/") + assert v["blob"] # sidecar token + # the bytes live on disk under blobs//, NOT inlined in the store + blob = art._blob_path(a["id"], v["blob"]) + assert blob.exists() and blob.read_bytes() == f.read_bytes() + + +def test_save_file_artifact_revision_appends_version(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + f = tmp_path / "r.txt" + f.write_text("v1 body", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f)}) + aid = _arts(art)[0]["id"] + f.write_text("v2 body changed", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f), "artifact_id": aid}) + a = _arts(art)[0] + assert a["id"] == aid and len(a["versions"]) == 2 + assert "v1 body" in a["versions"][0]["code"] and "v2 body changed" in a["versions"][1]["code"] + # each version keeps its OWN blob (distinct tokens), both on disk + b1, b2 = a["versions"][0]["blob"], a["versions"][1]["blob"] + assert b1 != b2 + assert art._blob_path(aid, b1).exists() and art._blob_path(aid, b2).exists() + + +def test_unknown_revision_target_is_rejected(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + f = tmp_path / "x.txt" + f.write_text("x", encoding="utf-8") + out = art.save_file_artifact.invoke({"path": str(f), "artifact_id": "a-nope"}) + assert "No artifact" in out and _arts(art) == [] + + +def test_oversized_file_is_rejected(monkeypatch, tmp_path): + monkeypatch.setenv("ARTIFACT_MAX_BLOB_KB", "1") + art = _load(monkeypatch, tmp_path) + f = tmp_path / "big.bin" + f.write_bytes(b"x" * 4096) + out = art.save_file_artifact.invoke({"path": str(f)}) + assert "too large" in out.lower() and _arts(art) == [] + + +def test_blob_gc_drops_orphaned_and_deleted(monkeypatch, tmp_path): + """Trimming past max_versions and deleting an artifact both sweep their sidecar blobs.""" + monkeypatch.setenv("ARTIFACT_MAX_VERSIONS", "2") + art = _load(monkeypatch, tmp_path) + f = tmp_path / "g.txt" + for i in range(3): # 3 revisions, cap is 2 → the oldest version's blob is orphaned + f.write_text(f"rev {i}", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f), "artifact_id": (_arts(art)[0]["id"] if _arts(art) else "")}) + a = _arts(art)[0] + assert len(a["versions"]) == 2 # trimmed + # exactly the 2 surviving version blobs remain on disk + live = {v["blob"] for v in a["versions"]} + on_disk = {p.name for p in (art._blob_root() / a["id"]).iterdir()} + assert on_disk == live + # deleting the artifact drops its whole blob dir + art.delete_artifact.invoke({"artifact_id": a["id"]}) + assert not (art._blob_root() / a["id"]).exists() + + +def test_blob_route_serves_bytes_with_filename(monkeypatch, tmp_path): + from fastapi.testclient import TestClient + + art = _load(monkeypatch, tmp_path) + f = tmp_path / "report.csv" + f.write_bytes(b"a,b\n1,2\n") + art.save_file_artifact.invoke({"path": str(f), "title": "Report"}) + aid = _arts(art)[0]["id"] + c = TestClient(_app(art)) + r = c.get(f"/api/plugins/artifact/artifact/{aid}/blob") + assert r.status_code == 200 + assert r.content == b"a,b\n1,2\n" + assert "report.csv" in r.headers.get("content-disposition", "") + # unknown artifact / non-file artifact → 404 + assert c.get("/api/plugins/artifact/artifact/nope/blob").status_code == 404 + art.show_artifact.invoke({"kind": "html", "code": "

x

"}) + html_id = _arts(art)[0]["id"] + assert c.get(f"/api/plugins/artifact/artifact/{html_id}/blob").status_code == 404 + + +def test_thumbnail_only_for_images(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + # a text file never gets a thumbnail + f = tmp_path / "t.txt" + f.write_text("plain", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f)}) + assert _arts(art)[0]["versions"][0]["file"]["thumb"] == "" + # an image gets a base64 PNG data-URI thumbnail (skip if Pillow isn't installed) + Image = pytest.importorskip("PIL.Image") + img = tmp_path / "pic.png" + Image.new("RGB", (64, 48), (200, 30, 30)).save(str(img)) + art.save_file_artifact.invoke({"path": str(img)}) + thumb = _arts(art)[0]["versions"][0]["file"]["thumb"] + assert thumb.startswith("data:image/png;base64,") + + +def test_save_file_artifact_registered_and_shell_has_filecard(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + names = [] + + class _Reg: + def register_tool(self, t): + names.append(t.name) + + def register_skill_dir(self, *a, **k): + pass + + def register_router(self, *a, **k): + pass + + def emit(self, *a, **k): + pass + + art.register(_Reg()) + assert "save_file_artifact" in names + # the shell renders file kinds as a download card, hides Edit, and downloads via the blob route + html = art._SHELL_HTML + assert "function fileCard(v)" in html + assert 'a.kind==="file" ? fileCard(v) : srcdoc(a.kind, v.code)' in html + assert "/blob?version=" in html + + +def test_docx_extraction_reads_paragraphs(monkeypatch, tmp_path): + docx = pytest.importorskip("docx") # python-docx; present on the desktop stack (ADR 0092 D1) + art = _load(monkeypatch, tmp_path) + d = docx.Document() + d.add_paragraph("First para of the report.") + d.add_paragraph("Second para with detail.") + p = tmp_path / "r.docx" + d.save(str(p)) + art.save_file_artifact.invoke({"path": str(p)}) + v = _arts(art)[0]["versions"][0] + assert "First para of the report." in v["code"] and "Second para" in v["code"] + assert _arts(art)[0]["kind"] == "file" + + +def test_xlsx_extraction_reads_sheet_cells(monkeypatch, tmp_path): + openpyxl = pytest.importorskip("openpyxl") + art = _load(monkeypatch, tmp_path) + wb = openpyxl.Workbook() + ws = wb.active + ws.title = "Sales" + ws.append(["Region", "Total"]) + ws.append(["West", 1200]) + p = tmp_path / "s.xlsx" + wb.save(str(p)) + art.save_file_artifact.invoke({"path": str(p)}) + code = _arts(art)[0]["versions"][0]["code"] + assert "# Sales" in code and "Region" in code and "1200" in code + + +def test_unparseable_office_file_degrades_not_crashes(monkeypatch, tmp_path): + """A .docx the extractor can't parse (corrupt, or python-docx absent in a lean env) still + saves — the preview is a readable degrade note, the bytes are stored, no crash.""" + art = _load(monkeypatch, tmp_path) + p = tmp_path / "broken.docx" + p.write_bytes(b"not really a docx") + out = art.save_file_artifact.invoke({"path": str(p)}) + assert "Saved file artifact" in out + v = _arts(art)[0]["versions"][0] + assert "no text preview" in v["code"].lower() + assert art._blob_path(_arts(art)[0]["id"], v["blob"]).read_bytes() == b"not really a docx" From c2d4a9ba1f9263fc3a0649cb00e4cac3fcb28ffd Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Tue, 21 Jul 2026 23:51:27 -0700 Subject: [PATCH 2/5] fix(artifact): guard kind-confusion on file artifacts + review polish (protoreview #2126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the QA panel review on #2126. Major (correctness): a `file` artifact and a text-source artifact must never cross _commit_version paths. Add an `_is_file` guard on BOTH directions: - save_file_artifact refuses to revise a non-file artifact (its kind would stay wrong and the panel would render the preview as raw source). - update_artifact / rewrite_artifact / the panel PUT (_save_edit) refuse a file artifact (a text edit would append a version with no blob → orphaned bytes + a broken card). The hidden Edit button was only a client mask; the route is now guarded server-side (409). Minor: download-error now flashes "Failed" instead of silently swallowing; SKILL.md signature includes artifact_id?; the manifest-fields test covers max_blob_kb + max_preview_kb; added positive-path pptx + pdf extraction tests and regression tests for both kind guards. (_clip's byte-slice + decode(errors="ignore") is intended — dropping a partial codepoint at a hard truncation boundary is correct.) Tests: bare (core CI, no doc stack) 65 passed / 5 skipped; full stack 70 passed. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --- plugins/artifact/__init__.py | 28 +++++++- .../skills/rendering-artifacts/SKILL.md | 2 +- tests/test_artifact_plugin.py | 69 ++++++++++++++++++- 3 files changed, 96 insertions(+), 3 deletions(-) diff --git a/plugins/artifact/__init__.py b/plugins/artifact/__init__.py index f7f96472e..56dc192a7 100644 --- a/plugins/artifact/__init__.py +++ b/plugins/artifact/__init__.py @@ -284,6 +284,15 @@ def _find(store: dict, art_id: str | None) -> dict | None: return next((a for a in store["artifacts"] if a["id"] == art_id), None) +def _is_file(art: dict) -> bool: + """A `file` artifact (ADR 0092 D2) — bytes in a sidecar blob, `code` is a derived + preview. Text-source edits (update/rewrite/panel PUT) must NOT touch these (they'd + append a version with no blob → orphaned bytes + a broken card), and save_file_artifact + must NOT revise a non-file artifact (its kind stays wrong → renders the preview as raw + source). Both directions are guarded on this.""" + return art.get("kind") == "file" + + def _too_big(code: str) -> str | None: limit = _max_code_bytes() if len(code.encode("utf-8")) > limit: @@ -577,6 +586,11 @@ def save_file_artifact(path: str, title: str = "", artifact_id: str = "") -> str art = _find(store, artifact_id) if artifact_id else None if artifact_id and art is None: return f"No artifact {artifact_id!r} to revise. Use list_artifacts, or omit artifact_id for a new one." + if art is not None and not _is_file(art): + return ( + f"Artifact {artifact_id!r} is a {art['kind']} artifact, not a file — save_file_artifact " + f"can only revise a file artifact. Omit artifact_id to create a new one." + ) art_id = art["id"] if art else _new_id() blob_name = f"{secrets.token_hex(8)}.{ext}" @@ -690,6 +704,11 @@ def update_artifact(old_string: str, new_string: str, artifact_id: str = "") -> art = _find(store, artifact_id or store["current"]) if art is None: return "No artifact to update. Create one with show_artifact first." + if _is_file(art): + return ( + f"Artifact {art['id']} is a file artifact (its source is a file on disk, not " + f"editable text). Re-generate the file and re-save it with save_file_artifact." + ) src = art["versions"][-1]["code"] n = src.count(old_string) if n == 0: @@ -723,6 +742,11 @@ def rewrite_artifact(code: str, title: str = "", artifact_id: str = "") -> str: art = _find(store, artifact_id or store["current"]) if art is None: return "No artifact to rewrite. Create one with show_artifact first." + if _is_file(art): + return ( + f"Artifact {art['id']} is a file artifact (its source is a file on disk, not " + f"editable text). Re-generate the file and re-save it with save_file_artifact." + ) if title: art["title"] = title v = _commit_version(store, art, code) @@ -970,6 +994,8 @@ async def _save_edit(art_id: str, body: dict = Body(...)) -> dict: art = _find(store, art_id) if art is None: raise HTTPException(404, f"unknown artifact {art_id}") + if _is_file(art): # a file artifact's preview isn't user-editable (would orphan its blob) + raise HTTPException(409, "file artifacts are not editable — re-save the file") v = _commit_version(store, art, code, by="user") return {"ok": True, "id": art_id, "version": v} @@ -1409,7 +1435,7 @@ def register(registry) -> None: if(a.kind==="file"){ // download the STORED BYTES via the gated blob route (ADR 0092 D2) try{ var r=await kit.apiFetch("/api/plugins/artifact/artifact/"+encodeURIComponent(a.id)+"/blob?version="+(vi+1)); if(!r.ok) throw 0; saveBlob(await r.blob(), (v.file&&v.file.filename)||("artifact-"+a.id)); } - catch(e){ /* transient — the panel poll will re-sync */ } + catch(e){ $dl.textContent="Failed"; setTimeout(function(){ $dl.textContent="Download"; },1800); } return; } saveBlob(new Blob([v.code],{type:"text/plain"}), "artifact-"+a.id+"-v"+(vi+1)+"."+(EXT[a.kind]||"txt")); diff --git a/plugins/artifact/skills/rendering-artifacts/SKILL.md b/plugins/artifact/skills/rendering-artifacts/SKILL.md index 64a04ce52..5dece3434 100644 --- a/plugins/artifact/skills/rendering-artifacts/SKILL.md +++ b/plugins/artifact/skills/rendering-artifacts/SKILL.md @@ -133,7 +133,7 @@ guess; read the error and make the targeted edit. ## Saving a generated FILE (docx / xlsx / pptx / pdf / image) When a skill writes a real **file** to disk — a Word doc, a spreadsheet, a slide deck, a PDF, an -image — call **`save_file_artifact(path, title?)`** right after, to put it in the Artifact panel as +image — call **`save_file_artifact(path, title?, artifact_id?)`** right after, to put it in the Artifact panel as a **versioned download artifact**: the bytes are stored, a readable text preview is extracted (docx→text, xlsx→sheet table, pptx→slide outline, pdf→text; images get a thumbnail), and the panel shows a download card. Re-saving the same document as a new revision? Pass the prior diff --git a/tests/test_artifact_plugin.py b/tests/test_artifact_plugin.py index ead72a017..8dac25d94 100644 --- a/tests/test_artifact_plugin.py +++ b/tests/test_artifact_plugin.py @@ -249,7 +249,14 @@ def test_manifest_exposes_all_settings_fields(monkeypatch, tmp_path): # every operator knob is a Settings ▸ Plugins field, with the right type. assert by_key["ask_enabled"]["type"] == "bool" assert by_key["ask_system"]["type"] == "string" - for num in ("ask_max_chars", "history", "max_versions", "max_code_kb"): + for num in ( + "ask_max_chars", + "history", + "max_versions", + "max_code_kb", + "max_blob_kb", + "max_preview_kb", + ): assert by_key[num]["type"] == "number", f"{num} should be a number field" # every settings key has a declared default in config:. assert set(by_key) <= set(m["config"]) @@ -946,3 +953,63 @@ def test_unparseable_office_file_degrades_not_crashes(monkeypatch, tmp_path): v = _arts(art)[0]["versions"][0] assert "no text preview" in v["code"].lower() assert art._blob_path(_arts(art)[0]["id"], v["blob"]).read_bytes() == b"not really a docx" + + +def test_pptx_extraction_reads_slide_text(monkeypatch, tmp_path): + pptx = pytest.importorskip("pptx") # python-pptx; present on the desktop stack (ADR 0092 D1) + art = _load(monkeypatch, tmp_path) + prs = pptx.Presentation() + slide = prs.slides.add_slide(prs.slide_layouts[5]) # title-only layout + slide.shapes.title.text = "Quarterly Roadmap" + prs.save(str(tmp_path / "deck.pptx")) + art.save_file_artifact.invoke({"path": str(tmp_path / "deck.pptx")}) + code = _arts(art)[0]["versions"][0]["code"] + assert "Slide 1" in code and "Quarterly Roadmap" in code + + +def test_pdf_extraction_reads_text(monkeypatch, tmp_path): + pytest.importorskip("pypdf") + canvas = pytest.importorskip("reportlab.pdfgen.canvas") # generate a real PDF to read back + art = _load(monkeypatch, tmp_path) + p = tmp_path / "doc.pdf" + c = canvas.Canvas(str(p)) + c.drawString(72, 720, "Invoice total due") + c.save() + art.save_file_artifact.invoke({"path": str(p)}) + assert "Invoice total due" in _arts(art)[0]["versions"][0]["code"] + + +# ── kind-confusion guards (protoreview #2126: both _commit_version directions) ── + + +def test_save_file_artifact_refuses_to_revise_a_non_file_artifact(monkeypatch, tmp_path): + art = _load(monkeypatch, tmp_path) + art.show_artifact.invoke({"kind": "html", "code": "

hi

"}) + html_id = _arts(art)[0]["id"] + f = tmp_path / "x.txt" + f.write_text("body", encoding="utf-8") + out = art.save_file_artifact.invoke({"path": str(f), "artifact_id": html_id}) + assert "not a file" in out + a = art._find(art._read_store(), html_id) + assert a["kind"] == "html" and len(a["versions"]) == 1 # untouched, not corrupted + + +def test_text_edits_refuse_a_file_artifact(monkeypatch, tmp_path): + from fastapi.testclient import TestClient + + art = _load(monkeypatch, tmp_path) + f = tmp_path / "r.txt" + f.write_text("original", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f)}) + fid = _arts(art)[0]["id"] + assert "file artifact" in art.update_artifact.invoke( + {"old_string": "original", "new_string": "x", "artifact_id": fid} + ) + assert "file artifact" in art.rewrite_artifact.invoke({"code": "x", "artifact_id": fid}) + # the panel PUT route is guarded too (the hidden Edit button is only a client mask) + c = TestClient(_app(art)) + r = c.put(f"/api/plugins/artifact/artifact/{fid}", json={"code": "x"}) + assert r.status_code == 409 + # still a single, intact file version with its blob + a = _arts(art)[0] + assert a["kind"] == "file" and len(a["versions"]) == 1 and a["versions"][0]["blob"] From b6d7a629866126df5538f3fe4d11e152b10aead5 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Wed, 22 Jul 2026 00:04:07 -0700 Subject: [PATCH 3/5] fix(artifact): _clip truncates on a UTF-8 codepoint boundary (protoreview #2126 WARN) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-review's one remaining finding: _clip byte-sliced then decode('utf-8','ignore'), which silently dropped a multi-byte char straddling the cut. Back the cut off any trailing continuation byte (0b10xxxxxx) so a codepoint is never split — decode('utf-8') is now exact, no 'ignore'. Added a boundary test (4-byte emoji over budget → clean truncation). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --- plugins/artifact/__init__.py | 16 ++++++++++------ tests/test_artifact_plugin.py | 13 +++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/plugins/artifact/__init__.py b/plugins/artifact/__init__.py index 56dc192a7..53247057a 100644 --- a/plugins/artifact/__init__.py +++ b/plugins/artifact/__init__.py @@ -433,14 +433,18 @@ def _guess_mime(path: Path) -> str: def _clip(text: str) -> str: - """Clip an extracted preview to _max_preview_bytes (utf-8), with a truncation note.""" + """Clip an extracted preview to _max_preview_bytes (utf-8), with a truncation note. + Cuts on a codepoint boundary — back the byte cut off any trailing continuation byte + (0b10xxxxxx) so a multi-byte char is never split (no silently-dropped straddler).""" + text = text or "" + data = text.encode("utf-8") limit = _max_preview_bytes() - data = (text or "").encode("utf-8") if len(data) <= limit: - return text or "" - return data[: max(0, limit - len(_PREVIEW_TRUNC.encode()))].decode( - "utf-8", "ignore" - ) + _PREVIEW_TRUNC + return text + cut = max(0, limit - len(_PREVIEW_TRUNC.encode())) + while cut > 0 and (data[cut] & 0xC0) == 0x80: # inside a multi-byte sequence → back off + cut -= 1 + return data[:cut].decode("utf-8") + _PREVIEW_TRUNC def _extract_docx(path: Path) -> str: diff --git a/tests/test_artifact_plugin.py b/tests/test_artifact_plugin.py index 8dac25d94..f06a434ac 100644 --- a/tests/test_artifact_plugin.py +++ b/tests/test_artifact_plugin.py @@ -1013,3 +1013,16 @@ def test_text_edits_refuse_a_file_artifact(monkeypatch, tmp_path): # still a single, intact file version with its blob a = _arts(art)[0] assert a["kind"] == "file" and len(a["versions"]) == 1 and a["versions"][0]["blob"] + + +def test_clip_truncates_on_a_codepoint_boundary(monkeypatch, tmp_path): + """_clip must not split a multi-byte char at the byte cut (protoreview #2126): a + 4-byte emoji straddling the budget is excluded cleanly, not decoded to a broken char.""" + art = _load(monkeypatch, tmp_path) + note_len = len(art._PREVIEW_TRUNC.encode()) + monkeypatch.setattr(art, "_max_preview_bytes", lambda: note_len + 5) # 5-byte body budget + out = art._clip("😀" * 30) # 4-byte codepoints, well over budget — byte-5 cut splits the 2nd + assert out.endswith(art._PREVIEW_TRUNC) + body = out[: -len(art._PREVIEW_TRUNC)] + assert body == "😀" # exactly one whole emoji fits; the straddling one is dropped, not mangled + body.encode("utf-8") # valid utf-8 round-trips (no partial sequence) From 5323e344e889a97e8ded43741fa1c7b2387e735b Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Wed, 22 Jul 2026 00:14:08 -0700 Subject: [PATCH 4/5] fix(artifact): per-directory error isolation in _gc_blobs (protoreview #2126 WARN) The blob-GC sweep wrapped its entire `for art_dir in root.iterdir()` loop in one try/except OSError, so a single un-sweepable dir (unexpected subdir, permissions/lock) aborted the whole pass and stranded every other orphan. Move the guard inside the loop so one bad dir is logged and skipped while the rest are still collected; the root iterdir() has its own guard. Test: an orphan dir that fails to drain doesn't stop a sibling orphan from being swept. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --- plugins/artifact/__init__.py | 13 ++++++++++--- tests/test_artifact_plugin.py | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/plugins/artifact/__init__.py b/plugins/artifact/__init__.py index 53247057a..aae0bb4fc 100644 --- a/plugins/artifact/__init__.py +++ b/plugins/artifact/__init__.py @@ -202,7 +202,14 @@ def _gc_blobs(store: dict) -> None: if names: live[a["id"]] = names try: - for art_dir in root.iterdir(): + art_dirs = list(root.iterdir()) + except OSError: + log.debug("[artifact] blob GC: cannot list %s", root, exc_info=True) + return + for art_dir in art_dirs: + # Per-directory isolation: a failure sweeping one dir (e.g. an unexpected subdir, a + # permissions/lock issue) must NOT abort the rest — it'd strand every other orphan. + try: if not art_dir.is_dir(): continue keep = live.get(art_dir.name) @@ -214,8 +221,8 @@ def _gc_blobs(store: dict) -> None: for f in art_dir.iterdir(): # artifact lives; drop only orphaned versions' blobs if f.name not in keep: f.unlink(missing_ok=True) - except OSError: - log.debug("[artifact] blob GC hiccup", exc_info=True) + except OSError: + log.debug("[artifact] blob GC hiccup on %s", art_dir, exc_info=True) def _now() -> int: diff --git a/tests/test_artifact_plugin.py b/tests/test_artifact_plugin.py index f06a434ac..512e056f2 100644 --- a/tests/test_artifact_plugin.py +++ b/tests/test_artifact_plugin.py @@ -1026,3 +1026,19 @@ def test_clip_truncates_on_a_codepoint_boundary(monkeypatch, tmp_path): body = out[: -len(art._PREVIEW_TRUNC)] assert body == "😀" # exactly one whole emoji fits; the straddling one is dropped, not mangled body.encode("utf-8") # valid utf-8 round-trips (no partial sequence) + + +def test_blob_gc_isolates_a_failing_dir(monkeypatch, tmp_path): + """A failure sweeping one blob dir must not abort GC of the others (protoreview #2126): + per-directory error isolation, so a stray un-drainable dir can't strand every orphan.""" + art = _load(monkeypatch, tmp_path) + root = art._blob_root() + bad = root / "a-bad" + bad.mkdir(parents=True) + (bad / "sub").mkdir() # a subdir → f.unlink() raises OSError while draining this orphan + good = root / "a-good" + good.mkdir() + (good / "v.bin").write_bytes(b"x") + art._gc_blobs({"artifacts": []}) # both are orphans (empty store) + assert not good.exists() # the good orphan was still swept despite bad failing (any order) + assert bad.exists() # bad couldn't be removed, but didn't abort the sweep From ed55290bc6c7ac675a4293aa06bd73a849b3c391 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Wed, 22 Jul 2026 00:24:09 -0700 Subject: [PATCH 5/5] fix(artifact): blob route 404s an out-of-range version; DRY the file-edit refusal (protoreview #2126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two advisory findings from the re-review (PR was approved-on-green; landing them anyway since the first is a real correctness gap): - _blob route: an EXPLICIT out-of-range `version` silently served the latest instead of 404-ing, contradicting the route's own docstring — a caller relying on 404 for a bad version got the wrong bytes. Now: explicit version must be in [1, N] (else 404); absent/0 → latest. Test covers all four cases. - DRY: the identical "file artifact is not editable" refusal in update_artifact + rewrite_artifact is extracted to _file_not_editable(art). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --- plugins/artifact/__init__.py | 27 +++++++++++++++++---------- tests/test_artifact_plugin.py | 20 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/plugins/artifact/__init__.py b/plugins/artifact/__init__.py index aae0bb4fc..42ddf7ddd 100644 --- a/plugins/artifact/__init__.py +++ b/plugins/artifact/__init__.py @@ -300,6 +300,14 @@ def _is_file(art: dict) -> bool: return art.get("kind") == "file" +def _file_not_editable(art: dict) -> str: + """The refusal returned when a text-source edit tool targets a file artifact.""" + return ( + f"Artifact {art['id']} is a file artifact (its source is a file on disk, not " + f"editable text). Re-generate the file and re-save it with save_file_artifact." + ) + + def _too_big(code: str) -> str | None: limit = _max_code_bytes() if len(code.encode("utf-8")) > limit: @@ -716,10 +724,7 @@ def update_artifact(old_string: str, new_string: str, artifact_id: str = "") -> if art is None: return "No artifact to update. Create one with show_artifact first." if _is_file(art): - return ( - f"Artifact {art['id']} is a file artifact (its source is a file on disk, not " - f"editable text). Re-generate the file and re-save it with save_file_artifact." - ) + return _file_not_editable(art) src = art["versions"][-1]["code"] n = src.count(old_string) if n == 0: @@ -754,10 +759,7 @@ def rewrite_artifact(code: str, title: str = "", artifact_id: str = "") -> str: if art is None: return "No artifact to rewrite. Create one with show_artifact first." if _is_file(art): - return ( - f"Artifact {art['id']} is a file artifact (its source is a file on disk, not " - f"editable text). Re-generate the file and re-save it with save_file_artifact." - ) + return _file_not_editable(art) if title: art["title"] = title v = _commit_version(store, art, code) @@ -1023,9 +1025,14 @@ async def _blob(art_id: str, version: int = 0): if art is None: raise HTTPException(404, f"unknown artifact {art_id}") vers = art.get("versions") or [] - idx = (version - 1) if version and 1 <= version <= len(vers) else len(vers) - 1 - if idx < 0: + if not vers: raise HTTPException(404, "no versions") + if version: # an EXPLICIT version must be in range — don't silently fall back to latest + if not (1 <= version <= len(vers)): + raise HTTPException(404, f"no version {version} (have 1..{len(vers)})") + idx = version - 1 + else: # 0/absent → latest + idx = len(vers) - 1 v = vers[idx] blob_name, meta = v.get("blob"), v.get("file") or {} if not blob_name: diff --git a/tests/test_artifact_plugin.py b/tests/test_artifact_plugin.py index 512e056f2..872c4be80 100644 --- a/tests/test_artifact_plugin.py +++ b/tests/test_artifact_plugin.py @@ -1042,3 +1042,23 @@ def test_blob_gc_isolates_a_failing_dir(monkeypatch, tmp_path): art._gc_blobs({"artifacts": []}) # both are orphans (empty store) assert not good.exists() # the good orphan was still swept despite bad failing (any order) assert bad.exists() # bad couldn't be removed, but didn't abort the sweep + + +def test_blob_route_version_bounds(monkeypatch, tmp_path): + """An explicit out-of-range version 404s per the route's docstring, not silently the + latest (protoreview #2126); absent/0 → latest, in-range → that version.""" + from fastapi.testclient import TestClient + + art = _load(monkeypatch, tmp_path) + f = tmp_path / "d.txt" + f.write_text("v1", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f)}) + aid = _arts(art)[0]["id"] + f.write_text("v2", encoding="utf-8") + art.save_file_artifact.invoke({"path": str(f), "artifact_id": aid}) + c = TestClient(_app(art)) + base = f"/api/plugins/artifact/artifact/{aid}/blob" + assert c.get(base).content == b"v2" # absent → latest + assert c.get(base + "?version=0").content == b"v2" # 0 → latest + assert c.get(base + "?version=1").content == b"v1" # explicit in-range + assert c.get(base + "?version=99").status_code == 404 # out of range → 404 (not latest)