feat(desktop): bundle the document-generation stack (ADR 0092 D1)#2123
Conversation
…pp (ADR 0092 D1) The ADR 0058 D2 frozen gate refuses any plugin whose `requires_pip` isn't importable in the read-only PyInstaller bundle — so cowork's docx/xlsx/pptx/pdf skills (hard deps: python-docx, openpyxl, python-pptx, reportlab) can't produce a file on desktop, and protobanana is refused over `pillow>=10`. For the "own your stack" demographic, "the desktop app can't make a .docx" is a credibility gap, not an edge case. Bake the document-generation stack into the desktop bundle, ON by default: - `apps/desktop/sidecar/requirements-docs.txt` (committed) — the 4 doc libs; installed in the desktop-build CI sidecar step. - `DOC_COLLECT_ALL` in build_sidecar.py — find_spec-guarded like the Google tier, so a lean local freeze without the extra still succeeds. `--collect-all` is required (reportlab fonts + docx/pptx default templates are package data a bare import-scan misses). Effect: once the libs are importable in the bundle, `_deps_satisfied` short-circuits and cowork's hard `requires_pip` is satisfied — cowork installs and WORKS on desktop with zero manifest change; protobanana's pillow refusal disappears (Pillow arrives transitively). This expands 0058's bundle baseline; it does NOT ship pip — libs are baked at build time, 0058's sanctioned mechanism. The general third-party dep path (pip-less wheel installer, #1631 Scope A) stays forthcoming ADR 0093. ADR 0092 also specs the follow-up in-track work: a binary "download artifact" tier in the artifact plugin (D2) and an explicit `save_file_artifact` tool (D3) so generated files become versioned artifacts. Refs #1631, #1953 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head ff28641f847a · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: Low. The change adds six document-format packages (python-docx, openpyxl, python-pptx, reportlab, lxml, Pillow) to the sidecar build, gated behind the same find_spec → skip-with-note pattern already used by the Google tier. Missing packages silently degrade without hard errors. CI now installs requirements-docs.txt before the sidecar build, and the ADR + nav entries are mechanically consistent. No refuted claims; all finder assertions about refactoring equivalence, import-name correctness, CI ordering, and local-dev resilience were verified as SUPPORTED.
Fix-first item: None urgent. The one UNCERTAIN item — whether _importable in the installer correctly resolves distribution-name → import-name mismatches (e.g., python-docx → docx) under --collect-all — is pre-existing (same pattern as the Google tier) and would be caught by a sidecar smoke test. A follow-up smoke-test run is the pragmatic next step before merging.
Panel disagreements: None. All finder claims converged; the verifier confirmed every substantive claim.
What verification changed: The verifier elevated no new blocker/major/minor findings. The _importable metadata-fallback uncertainty (pre-existing, not introduced by this diff) is reported below as a nit for visibility.
Gaps: The structural-analysis pass (code-review-structural:verify) completed and its concerns were folded into the verification table. No structural pass was skipped. Coverage note: build_sidecar.py was the most-changed production file; the refactoring there is mechanically sound, and the only residual concern is the cross-file _importable interaction below.
[
{
"file": "apps/desktop/sidecar/build_sidecar.py",
"line": 0,
"severity": "nit",
"category": "cross-file",
"claim": "Installer `_importable` metadata fallback may produce false negatives for packages where the import name differs from the distribution name (e.g., `python-docx` → `docx`). The `find_spec` fallback calls `pkg.replace('-', '_')`, yielding `python_docx` rather than `docx`; if PyInstaller's `--collect-all docx` does not also collect `python_docx-*.dist-info` metadata, `_importable('python-docx')` could return False despite the package being collected. Same pre-existing pattern as the Google tier — not introduced by this diff.",
"evidence": "The diff adds `'docx': 'python-docx'` (and analogous `pptx`/`PIL` mappings) to `DOC_COLLECT_ALL` in `build_sidecar.py`. The installer's `_importable` function resolves `python-docx` via `importlib.metadata.version('python-docx')` → fallback `find_spec('python_docx')`, which does not match the actual module name `docx`. Identical pattern exists for `google-cloud-*` packages in the pre-existing Google tier.",
"verdict": "uncertain",
"note": "`_importable('python-docx')` tries `md.version('python-docx')` then `find_spec('python_docx')`. The fallback uses `pkg.replace('-','_')` — for `python-docx`→`python_docx` (not `docx`), `python-pptx`→`python_pptx` (not `pptx`), `Pillow`→`Pillow` (not `PIL`). Whether `--collect-all docx` also collects `python_docx-*.dist-info` depends on PyInstaller's distribution-name resolution. Smoke test would catch. Same pattern as pre-existing Google tier — not a new-introduced defect."
}
]…solves them (protoreview #2123) protoreview flagged an uncertain-but-load-bearing gap: the ADR 0058 D2 gate checks a plugin's requires_pip via installer._importable, which tries importlib.metadata.version(<dist>) FIRST. With only --collect-all <import>, whether the python_docx-*.dist-info metadata lands in the frozen bundle is PyInstaller-version dependent — so cowork's `requires_pip: [python-docx, …]` could still be refused on the desktop app even though `import docx` works. Make it deterministic: DOC_COLLECT_ALL now carries (import_name, distribution_name), and the build emits --copy-metadata <dist> alongside --collect-all <import> for each. So metadata.version("python-docx") resolves in the frozen app regardless of PyInstaller's collect-all metadata behavior — the gate short-circuits and cowork installs. The dist≠import gap (python-docx→docx, python-pptx→pptx, Pillow→PIL) is exactly the case this closes. ADR 0092 verification note updated to name the metadata gate (not just import). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z
|
Good catch on the
Also updated ADR 0092's verification note so the follow-up frozen smoke asserts both |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 0c599ddf1b24 · formal
[review-synthesizer completed: workflow code-review-structural:report]
I don't have the findings to synthesize — your message appears to have been cut off after the verifier status note and the setup.py mention. The actual annotated findings array that the verifier produced (even if partial) isn't present in what reached me.
Could you re-supply the verifier-annotated findings? I need the JSON array (or arrays) from the finder panel, each with at minimum file, line, severity, claim, evidence, and whatever verdict / note fields the verifier attached. Once I have that, I can dedup, re-grade, drop the refuted, carry verdicts forward, and produce the brief + canonical fenced JSON.
…t (ADR 0092 D2) (#2126) * feat(artifact): versioned file/download artifacts — save_file_artifact (ADR 0092 D2) 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 <artifact-dir>/blobs/<id>/<token>.<ext> (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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z * fix(artifact): guard kind-confusion on file artifacts + review polish (protoreview #2126) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z * fix(artifact): _clip truncates on a UTF-8 codepoint boundary (protoreview #2126 WARN) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z * fix(artifact): blob route 404s an out-of-range version; DRY the file-edit refusal (protoreview #2126) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rtifacts, fleet inputs/secrets (#2130) Bump 0.106.0 → 0.107.0 and roll the changelog for the release. Highlights: - Versioned file artifacts + save_file_artifact (ADR 0092 D2, #2126) - Document-generation stack baked into the desktop bundle (ADR 0092 D1, #2123) - Fleet agents accept operator inputs + secrets at create time (#2121/#2122/#2125/#2127) - Deterministic persona-drift detection (#2116) - Chat tab context menu Close Others/Left/Right (#2112) - Fixes: archetype-catalog bundling (#2115), PyPI publishes on tag push (#2113) Backfilled [Unreleased] entries for the feature PRs that lacked them, rolled to [0.107.0], scaffolded + polished the marketing changelog. uv.lock: project version line only (surgical — avoids uv 0.11.13-vs-pinned-0.11.29 marker drift). Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Bakes the document-generation stack (
python-docx,openpyxl,python-pptx,reportlab;Pillow+lxmltransitively) into the frozen desktop app, on by default — so the knowledge-work surface actually produces files on desktop.This is Track 1a of the cowork-docs initiative and ships ADR 0092.
Why
ADR 0058 D2 refuses any plugin whose
requires_pipisn't importable in the read-only bundle, with "install it on a server/Docker build instead." Real hits:pillow>=10.For the "own your stack" demographic (migrating off Claude Cowork / OpenAI Codex to a local-first agent), "the desktop app can't make a
.docx" is a credibility gap, not an edge case. cowork is a first-party flagship, so it gets a platform answer.How
apps/desktop/sidecar/requirements-docs.txt(committed) — installed in the desktop-build CI sidecar step.DOC_COLLECT_ALLinbuild_sidecar.py—find_spec-guarded exactly like the GoogleOPTIONAL_COLLECT_ALLtier, so a lean local freeze without the extra still succeeds.--collect-all(not bare hidden-imports) is required: reportlab ships font/AFM data and python-docx/pptx ship their default templates as package data that an import-scan misses.Effect: once the libs are importable,
_deps_satisfiedshort-circuits and cowork's hardrequires_pipis simply satisfied — cowork installs and works on desktop with zero manifest change, and protobanana's pillow refusal disappears.This expands 0058's bundle baseline; it does not ship pip — libs are baked at build time (0058's sanctioned mechanism, same as the
plugins/tree + Google libs).Not in this PR (specced in ADR 0092, follow-ups in-track)
save_file_artifact(path, title)tool cowork's skills call so outputs become versioned artifacts.Test / verify
ruff✅, docs-nav--check✅ (ADR 0092 in nav),tests/test_mcp_routes.py+tests/test_bundled_config_assets.py✅ (the two suites that touchbuild_sidecar.py).scripts/live_smoke.py --binto assertimport docx, openpyxl, pptx, reportlabin the actual frozen binary.Refs #1631, #1953
🤖 Generated with Claude Code