feat(matterviz): add signed prerelease self-updater - #51
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a signed prerelease updater across Rust libraries and binaries, desktop HTTP integration, viewer UI, process lifecycle handling, secure packaging, preview signing, release verification, and operational documentation. ChangesMatterViz updater
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Viewer
participant HttpService
participant UpdateManager
participant Updater
participant ReleaseServer
Viewer->>HttpService: request update status or action
HttpService->>UpdateManager: validate visibility and start operation
UpdateManager->>Updater: run JSON updater command
Updater->>ReleaseServer: fetch signed preview metadata and archive
ReleaseServer-->>Updater: signed manifest and package
Updater-->>UpdateManager: update state JSON
UpdateManager-->>HttpService: status response
HttpService-->>Viewer: render status or modal progress
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/matterviz-updater/src/bin/multiwfn-matterviz-sign.rs (1)
117-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the release-target builder shared by
build-manifestandbuild-sign.Lines 119-123 and 162-166 are identical (read archive, parse inventory, verify metadata, build
ReleaseTarget). Extracting one helper avoids the two copies drifting apart in security-relevant tooling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/matterviz-updater/src/bin/multiwfn-matterviz-sign.rs` around lines 117 - 170, Extract the duplicated release-target construction logic from the “build-manifest” and “build-sign” command branches into a shared helper, preserving archive reading, inventory parsing, metadata validation, hashing, and error behavior. Update both branches to call the helper when producing their target lists, using the existing target/archive/inventory inputs.frontend/matterviz-viewer/src/UpdateModal.svelte (1)
12-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
UpdateModal.sveltewith the viewer’s runes style
This modal still uses legacyexport let/$:while the rest of the viewer is on runes ($state). Converting it to$props()/$derived/$effectwould keep the Svelte style consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/matterviz-viewer/src/UpdateModal.svelte` around lines 12 - 24, Convert UpdateModal’s legacy export declarations and reactive `$:` statement to Svelte runes: obtain the component inputs through `$props()`, store mutable status and other local state with `$state`, derive the initial status with `$derived` where appropriate, and synchronize changes from `initial_status` using `$effect`. Preserve the existing defaults, callback types, and status-update behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/matterviz-spectrum-log.md`:
- Line 3: Correct the future-dated documentation markers: update the
architecture section heading in docs/matterviz-spectrum-log.md lines 3-3 and the
update marker/self-update section date in docs/matterviz-spectrum-todo.md lines
3-5 to the actual change date or revised review context, using consistent dates
across both documents.
In `@frontend/matterviz-updater/src/bin/multiwfn-matterviz-updater.rs`:
- Around line 37-67: Update reply’s enabled calculation to avoid calling
authenticate_current_inventory, since it re-hashes every managed file on each
response; validate only settings.ini, local target, embedded key registry
presence, and the current inventory proof’s signature/release tag using the
existing non-hashing verification path. Reuse the already computed current and
registry values instead of calling read_current_tag and embedded_key_registry
repeatedly, while preserving the enabled=false behavior when any prerequisite or
proof validation fails.
In `@frontend/matterviz-updater/src/lib.rs`:
- Around line 1462-1469: The write-probe handling in apply_transaction must
tolerate a stale .multiwfn-updater-write-probe from a prior crash. Update the
OpenOptions/create_new flow to remove an existing probe or otherwise handle
AlreadyExists before continuing, while preserving the write and sync validation
and final cleanup.
---
Nitpick comments:
In `@frontend/matterviz-updater/src/bin/multiwfn-matterviz-sign.rs`:
- Around line 117-170: Extract the duplicated release-target construction logic
from the “build-manifest” and “build-sign” command branches into a shared
helper, preserving archive reading, inventory parsing, metadata validation,
hashing, and error behavior. Update both branches to call the helper when
producing their target lists, using the existing target/archive/inventory
inputs.
In `@frontend/matterviz-viewer/src/UpdateModal.svelte`:
- Around line 12-24: Convert UpdateModal’s legacy export declarations and
reactive `$:` statement to Svelte runes: obtain the component inputs through
`$props()`, store mutable status and other local state with `$state`, derive the
initial status with `$derived` where appropriate, and synchronize changes from
`initial_status` using `$effect`. Preserve the existing defaults, callback
types, and status-update behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 759d1537-c878-4821-8a49-57b2cb949065
⛔ Files ignored due to path filters (1)
frontend/matterviz-updater/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/matterviz-gui.ymldocs/matterviz-spectrum-log.mddocs/matterviz-spectrum-todo.mddocs/matterviz-updater-signing.mddocs/matterviz-updater.mdfrontend/matterviz-desktop/src/cli.rsfrontend/matterviz-desktop/src/main.rsfrontend/matterviz-desktop/src/service.rsfrontend/matterviz-desktop/src/updater.rsfrontend/matterviz-updater/Cargo.tomlfrontend/matterviz-updater/README.mdfrontend/matterviz-updater/src/bin/multiwfn-matterviz-sign.rsfrontend/matterviz-updater/src/bin/multiwfn-matterviz-updater.rsfrontend/matterviz-updater/src/lib.rsfrontend/matterviz-updater/trusted-keys.jsonfrontend/matterviz-viewer/src/App.sveltefrontend/matterviz-viewer/src/UpdateModal.sveltefrontend/matterviz-viewer/src/styles.cssfrontend/matterviz-viewer/src/update.tsfrontend/matterviz-viewer/src/vite-env.d.tsfrontend/matterviz-viewer/tests/update.test.tsnoGUI/matterviz_spawn.ctests/matterviz-volume-e2e/src/lib.rstests/test_matterviz_build_names.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
5dcb072 to
47a5e80
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/matterviz-desktop/src/service.rs (3)
1029-1038: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
plot_store.clear()to the sharedsignal_returnhelper.
ServiceRunner::signal_returnclears onlyvolume_store. Every other shutdown/invalidation path in this file (HttpService::signal_returnat Line 416-426,ServiceRunner::run's accept-error branch at Line 494-502,terminate_control_sessionat Line 1346-1361, and the explicit calls in the/api/returnhandler at Line 812-813) clears both stores. The/api/update/installsuccess branch at Line 696-711 calls this privatesignal_return, then redundantly re-clearsvolume_storebut never clearsplot_store. Stale plot data stays servable through/api/plot-data/{id}during the shutdown window that follows a successful update.Move the
plot_store.clear()call into this shared function so every caller gets consistent cleanup, and the redundant explicitvolume_store.clear()calls at the call sites become unnecessary.🐛 Proposed fix
fn signal_return(&self) -> Result<(), String> { let result = signal_return( self.in_memory_session, &self.return_signaled, self.control_session.as_deref(), &self.session, ); self.volume_store.clear(); + self.plot_store.clear(); result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/matterviz-desktop/src/service.rs` around lines 1029 - 1038, Update ServiceRunner::signal_return to clear plot_store alongside volume_store after invoking the shared signal_return helper. Remove redundant volume_store.clear() calls from its callers, including the /api/update/install success path, while preserving cleanup behavior for all shutdown and invalidation flows.
255-269: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
active_data_budgetfor orbital admissions with both stores
active_volume_budgetis an alias foractive_data_budget, butServiceRunner::stream_orbitalonly addsself.volume_store.bytes(). Includeself.plot_store.bytes()in thatcurrent_activevalue so admissions and test admission checks use the same active-data budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/matterviz-desktop/src/service.rs` around lines 255 - 269, Update ServiceRunner::stream_orbital so its current_active calculation includes both self.volume_store.bytes() and self.plot_store.bytes(). Use this combined value for orbital admission checks, preserving the active_data_budget behavior and ensuring test admission checks use the same total.
1387-1486: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the total time to read one HTTP request, not just each individual
read()call.
read_http_requestbounds header size (MAX_HEADERS), request-line length (MAX_REQUEST_LINE), and body size (MAX_PLOT_EXPORT_BYTES), but it has no overall wall-clock deadline. The caller setsstream.set_read_timeout(Duration::from_secs(2))(Line 538), which only limits how long a singleread()call can block. An attacker who sends at least one byte every 1.9 seconds can keep both the header-read loop (Line 1393-1408) and the body-read loop (Line 1469-1479) alive indefinitely.This is reachable before the Host check (Line 555) and before the capability check (Line 612), so it does not require a valid session token. Each accepted connection runs on its own spawned thread (
ServiceRunner::run, Line 486-493), so a small number of slow connections can pin threads for an unbounded time, degrading availability for legitimate clients on this host.Track an overall deadline across both loops, mirroring the existing
Instant-based deadline pattern already used incontrol_transport.rs'sread_exact_timeout.🔒 Suggested direction
fn read_http_request(stream: &mut TcpStream) -> Result<HttpRequest, HttpRequestError> { const MAX_REQUEST_DURATION: Duration = Duration::from_secs(30); let deadline = Instant::now() + MAX_REQUEST_DURATION; // ... loop { if Instant::now() >= deadline { return Err(HttpRequestError::BadRequest); } let length = stream.read(&mut chunk).map_err(|_| HttpRequestError::BadRequest)?; // ... } // ... while body.len() < body_length { if Instant::now() >= deadline { return Err(HttpRequestError::BadRequest); } // ... } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/matterviz-desktop/src/service.rs` around lines 1387 - 1486, Update read_http_request to establish one Instant-based deadline for the entire request, using a suitable maximum duration. Check that deadline before each read in both the header-accumulation loop and body-reading loop, returning BadRequest when expired, so progress bytes cannot extend the total allowed time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/matterviz-desktop/src/service.rs`:
- Around line 1029-1038: Update ServiceRunner::signal_return to clear plot_store
alongside volume_store after invoking the shared signal_return helper. Remove
redundant volume_store.clear() calls from its callers, including the
/api/update/install success path, while preserving cleanup behavior for all
shutdown and invalidation flows.
- Around line 255-269: Update ServiceRunner::stream_orbital so its
current_active calculation includes both self.volume_store.bytes() and
self.plot_store.bytes(). Use this combined value for orbital admission checks,
preserving the active_data_budget behavior and ensuring test admission checks
use the same total.
- Around line 1387-1486: Update read_http_request to establish one Instant-based
deadline for the entire request, using a suitable maximum duration. Check that
deadline before each read in both the header-accumulation loop and body-reading
loop, returning BadRequest when expired, so progress bytes cannot extend the
total allowed time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f30b289d-41fc-492b-85b7-a463567a6cec
📒 Files selected for processing (12)
.github/workflows/matterviz-gui.yml.gitignoredocs/matterviz-spectrum-log.mddocs/matterviz-spectrum-todo.mdfrontend/matterviz-desktop/src/main.rsfrontend/matterviz-desktop/src/service.rsfrontend/matterviz-viewer/src/App.sveltefrontend/matterviz-viewer/src/styles.cssfrontend/matterviz-viewer/src/vite-env.d.tsnoGUI/matterviz_spawn.ctests/matterviz-volume-e2e/src/lib.rstests/test_matterviz_build_names.py
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/matterviz-desktop/src/main.rs
- frontend/matterviz-viewer/src/vite-env.d.ts
- noGUI/matterviz_spawn.c
- frontend/matterviz-viewer/src/styles.css
- docs/matterviz-spectrum-todo.md
- tests/matterviz-volume-e2e/src/lib.rs
- frontend/matterviz-viewer/src/App.svelte
- docs/matterviz-spectrum-log.md
- tests/test_matterviz_build_names.py
- .github/workflows/matterviz-gui.yml
1 participant
Summary
Verification
Release safety
frontend/matterviz-updater/trusted-keys.jsonintentionally remains empty. Preview signing fails closed until the protectedmatterviz-preview-signingEnvironment and production public key are configured. This PR does not create a tag or release. The first trust-root preview must be installed manually; a subsequent signed preview is required for real N-to-N+1 validation.Summary by CodeRabbit