fix: saving, plugin sandboxing and honest failures - #55
Conversation
af7b2df to
afb8451
Compare
IAmJSD
left a comment
There was a problem hiding this comment.
This review was written by Claude (Fable 5), acting on Astrid's behalf.
There's a lot of genuinely good work here — the with_extension clobber fix is real and well-tested, the off-thread autosave is architecturally sound (COW snapshot inside the entity update, serialized runs, history/damage dropped from the snapshot), and the MCP validation/protocol-version/pixel-cap work is correct. But CI is red for a structural reason, the resource limits break legitimate plugins on ordinary images, and several of the PR body's claims aren't actually delivered by the diff. Requesting changes.
Blocker — capability gating breaks plugin loading entirely (crates/plugin-host-wasm/src/lib.rs:248, with the bootstrap at :167): LoadedPlugin::load bootstraps with a placeholder manifest whose capabilities is empty, then instantiates to read the real manifest. Since schist::log is now only linked when the manifest declares Capability::Log, the bootstrap linker never defines it and wasmtime refuses instantiation for any module that imports it — including the repo's own pgm example, whose manifest does declare ["log"]. That's the CI failure (pgm_codec_plugin_decodes_an_image, all three OSes). Always define the import and make it a no-op (or trap) when the capability is absent.
Major — the limits don't fit the data sizes this same PR endorses:
MAX_PLUGIN_MEMORY = 256 MiB(lib.rs:39):run_filterwrites the full RGBA f32 buffer into plugin memory — 16 bytes/pixel — so any image over ~16.7 MP (a stock 24 MP photo, well under the 30000×30000 document limit and the MCP 200 MP cap you add in this PR) can't even allocate its input.FUEL_PER_CALL = 5e8(lib.rs:31): ~25–40 instructions/pixel on a 12–20 MP image; the sepia example alone lands around 4–5e8 on a 12 MP photo. Tests only pass because they use tiny images. Scale fuel with pixel count, raise the budget, or use epoch interruption (which the PR body itself names as the right mechanism).
Major — advertised fixes that aren't wired up:
try_apply(plugin-api/src/lib.rs:428,plugin-host-wasm/src/lib.rs:474) has zero callers: the app (workspace.rs:2486,:4853,:4920) and MCP (session.rs:370) still callapply(), andWasmFilter::applydoeslet _ = self.try_apply(...). A trapped/fuel-exhausted filter still produces exactly the silent no-op the commit message says it fixes.disabled_write_error(lib.rs:578,:690) is written but never read —Workspace::set_plugin_enabledstill unconditionally reports success.
Claim mismatches with the PR body:
- The body says MCP save no longer calls
create_dir_allunconditionally, butwrite_atomically(session.rs:592) still does — a path typo still silently creates the whole tree. - The body says duplicate codec/filter/command ids are now refused;
register_commandsis untouched.
Minor:
temp_save_pathwas inserted betweenblit_rgba8and its doc comment (core/src/document.rs:1055), so the docs now describe the wrong function.a_failed_write_leaves_no_temp_file(session.rs:836) never induces a failure — it asserts cleanup after a successful write.- Two near-identical pixel-cap tests (
session.rs:851,:864) look like an unfinished consolidation. - There are now three atomic-write implementations with two temp-name schemes; MCP's
write_atomicallyshould probably call thetemp_save_pathhelper this PR adds toschist_core, and the interactive Save path still lacks the flush-before-rename this PR argues for.
Happy to re-review once the blocker and the limits are addressed — the underlying direction (honest failures, bounded plugins, atomic writes) is right.
9eddad6 to
f864bcd
Compare
|
fixed |
IAmJSD
left a comment
There was a problem hiding this comment.
This follow-up review was written by Claude (Fable 5), acting on Astrid's behalf.
Nearly there — the blocker and most findings are genuinely fixed: schist::log is always defined (no-op without the capability), filter fuel scales with pixel count (a 24 MP sepia verifiably passes), try_apply is wired through the dialog commit, preview and MCP paths with real error surfacing, the disabled-write error reaches the status bar, there's now one shared write_atomically (fsync before rename, refuses a missing parent, honest failure-cleanup test that actually induces a failure), and duplicate ids are refused for codecs, filters and commands. Good round.
One regression keeps this from merging:
Major — codec decode is fuel-starved on ordinary images (crates/plugin-host-wasm/src/lib.rs:382): the fuel scaling was applied to run_filter only; decode still calls plain instantiate → flat FUEL_PER_CALL = 5e8. Empirically (repo's own pgm example, same harness on both revisions): 12 MP and 24 MP decodes fail with "plugin trapped or ran out of fuel" at this head and succeed on main. Opening an ordinary photo through any plugin codec now fails. Mirror the filter fix — pixel count isn't known pre-decode, so scale with input size (FUEL_PER_CALL + bytes.len() * k) or use epoch interruption.
Smaller items, non-blocking:
MAX_PLUGIN_MEMORY = 4 GiBis the wasm32 architectural max, so the limiter can no longer reject anything — the OOM-kill scenario the constant's own doc comment warns about is reinstated verbatim. Consider scaling the cap with the call's working set, like the fuel.- The Filter Gallery commit path still swallows failures:
run_gallerymapstry_applyerrors tolog::warn!andcommit_galleryrecords a "Filter Gallery" history entry and success status even if every wasm entry trapped — the silent-no-op pattern this PR fixes on the other three call sites. preview_filter's error path setsself.statusbut returns withoutcx.notify(), so the message may not repaint and the stale preview lingers.- No test covers a module importing
schist::logwithout declaring the capability (both examples declare it); the path is trivially safe but cheap to pin.
Fix the decode fuel and this merges.
groups four branches plus the autosave half of a fifth: saving, sandboxing, and failures the app hid.
saving
with_extension("schist-tmp"), which replaces the final extension: savingphoto.psdwrote and renamedphoto.schist-tmp, destroying any real file of that name, and two saves of the same stem raced for it.create_dir_all(parent)unconditionally, so a path typo scattered empty directory trees instead of reporting that the destination does not exist.Entity::update, i.e. on the main thread. a hard hitch every thirty seconds on any large document, seconds of frozen ui mid-brushstroke. only the snapshot is taken on the main thread now, and it is a handful ofArcbumps.sandboxing
a wasm plugin could commit 4 GiB with no memory limiter; the fuel budget froze the ui for seconds per call with no epoch interruption; a codec probe ran with a full decode budget on every file open and turned any failure into a bare
false, so a broken plugin looked exactly like "no codec for foo.xyz"; declared capabilities were never enforced, so a plugin declaringcapabilities: []still got the log import; andInstance::readallocated up to 512 MiB before range-checking.plugin ids were arbitrary text checked only for non-emptiness, and the disabled set was newline-separated — an id of
"evil\ncom.vendor.trusted"disabled an unrelated plugin as a side effect, andretain(|d| d != id)could never remove the injected entry.failures reported as success
log::error!and returned normally: the status bar read the filter's name, the image was unchanged, the title showed unsaved changes, and history gained a no-op step.set_editorandset_layer_propsskipped absent, misspelled and wrongly-typed keys and then returned "editor state updated" — a model sending{"brushsize": 40}was told it worked.image.cropandlayer.adjustment.*, neither of which exists anywhere in the repo.create_session {"width":30000,"height":30000}— a plausible typo for 3000x3000 — allocated 3.6 GB in onevec!and aborted the server, taking every other session's unsaved work with it.initializeechoed back whatever protocol version the client asked for.merge notes
two branches independently fixed the temp path and the session pixel cap; i kept the richer version of each (the mcp one also flushes before renaming). two independently added plugin-id validation; the stricter one won.
cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace— 636 passed, 0 failed.how this relates to my other open prs
these seven are independent of each other — each branches off
mainand each is green on its own. they do share files with my five open prs (#36, #38, #42, #44, #46), mostlyworkspace.rs, so whichever lands first will leave the others needing a rebase. happy to rebase in whatever order suits you, or to split any of these further if one is too big to review in a sitting.