feat(launcher): replace Tauri prototype with egui + Azure Blob seed/patch manifest#343
Merged
Conversation
…atch manifest
Scraps the Tauri 2 WebView2 launcher in crates/launcher/ and replaces it
with a native egui (eframe) Windows app that consumes a JSON manifest
from Azure Blob Storage. The new launcher is ~5 MB single-file, has no
JS toolchain, and uses the original CME launcher's seed-plus-patches
model rather than the archive.org RAR-of-CABs first-run install.
Crate (crates/launcher/, 10 flat src files):
- main.rs / app.rs eframe entry + egui panels (Config / Install /
Launch / Upload Logs / Status)
- manifest.rs schema-1 JSON parser with forward-ref and
duplicate-id validation
- install.rs resumable HTTP downloads (Range), sha256 verify,
seed-first then ordered-patch overlay extraction
- patch_rdata.rs salvaged 22-byte .rdata hostname patch
- launch.rs detects SGW.exe, AteraLoader.exe + Atrea bats,
spawns via CreateProcess / cmd /C as appropriate
- logs.rs collects Binaries/sgwdebuglog* + sessions/**,
single PUT to an Azure SAS URL, content-digest
dedupe so repeat clicks against unchanged logs
make zero HTTP requests
- state.rs InstalledState (in game dir) + UploadedLedger
(next to .exe)
- worker.rs tokio worker, Command/Event channels, progress
forwarder
Workflows (mirror the server release-on-comment pattern):
- launcher-build.yml path-filtered fmt/clippy/build/test
on PRs touching crates/launcher/**
- launcher-release.yml workflow_dispatch, bakes
LAUNCHER_LOG_SAS_URL from secrets,
creates a GitHub Release tagged
launcher-<date>-<sha7>
- launcher-release-on-comment.yml /release-launcher ChatOps on merged
PRs, validates write access,
dispatches launcher-release.yml
Docs:
- docs/client/sgw-launcher.md rewritten
- docs/client/launcher-guide.md new — user +
operator guide
- docs/client/launcher-storage-setup.md new — Azure
container /
SAS runbook
- docs/plans/2026-03-06-sgw-launcher-{design,plan}.md superseded
- crates/README.md + docs/readme.md + CLAUDE.md index updates
Verified locally on Windows:
cargo check -p sgw-launcher OK
cargo clippy -p sgw-launcher --all-targets -- -D warnings OK
cargo test -p sgw-launcher 40/40 pass
cargo fmt -p sgw-launcher -- --check OK
launcher GUI opens, panels render, manifest-fetch path runs.
The launcher remains in WORKSPACE_EXCLUDES in test.yml so eframe Linux
deps do not slow the main CI pipeline; launcher-build.yml owns its
full clippy/build/test cycle on windows-latest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 41 minutes and 36 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughReplaces the Tauri-based launcher with a native egui/eframe Windows launcher, implements manifest-authenticated seed+patch install with Ed25519 verification, resumable SHA-verified downloads, in-place SGW.exe hostname patching, deterministic log zipping and Azure SAS upload with ledger dedupe, and adds CI/release workflows and operator docs. ChangesComplete launcher rewrite: UI framework, installation, and release pipeline
Sequence Diagram (high-level install + upload flow) sequenceDiagram
participant UI
participant Worker
participant InstallModule as "install_all"
participant HTTP as "reqwest"
participant Logs as "logs::upload_blob"
UI->>Worker: Command::Install(config, manifest)
Worker->>InstallModule: spawn install_all(ctx)
InstallModule->>HTTP: GET seed/patch (Range, resume)
InstallModule->>InstallModule: verify SHA, extract ZIP
InstallModule-->>Worker: InstallComplete / InstallError
UI->>Worker: Command::UploadLogs(...)
Worker->>Logs: upload_logs_task
Logs->>HTTP: PUT ZIP to SAS URL
Logs-->>Worker: UploadComplete / UploadError
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous Tauri/WebView2 launcher prototype with a native Windows launcher crate (sgw-launcher) built on eframe/egui, using an Azure Blob–hosted seed + patch manifest for installs/updates and optional debug-log uploads. It also updates and adds documentation for both player usage and operator storage setup, and introduces dedicated GitHub Actions workflows for launcher CI and release.
Changes:
- Replaced the Tauri-based launcher UI/tooling with an egui/eframe implementation backed by a manifest-driven seed/patch install pipeline.
- Added debug-log zipping + Azure Blob single-PUT upload with local dedupe ledger, plus Atera debug tooling detection/launch.
- Added launcher-specific CI/release workflows and updated docs/indexes to reflect the new launcher architecture and operational runbook.
Reviewed changes
Copilot reviewed 35 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/readme.md | Updates docs index to include new launcher docs and revised launcher design description. |
| docs/plans/2026-03-06-sgw-launcher-plan.md | Marks the old Tauri implementation plan as superseded and points to the new launcher/doc location. |
| docs/plans/2026-03-06-sgw-launcher-design.md | Marks the old Tauri-era design decisions doc as superseded and links to the current design doc. |
| docs/client/sgw-launcher.md | Rewrites launcher design doc around egui + Azure Blob manifest seed/patch model and log upload. |
| docs/client/launcher-storage-setup.md | Adds Azure Blob operational runbook for anonymous reads and SAS-scoped log uploads. |
| docs/client/launcher-guide.md | Adds player/operator guide for install/update behavior and patch publishing workflow. |
| crates/README.md | Expands launcher crate description and links to the new launcher documentation. |
| crates/launcher/ui/styles.css | Removes Tauri web UI CSS (no longer used). |
| crates/launcher/ui/main.js | Removes Tauri web UI JS (no longer used). |
| crates/launcher/ui/index.html | Removes Tauri web UI HTML (no longer used). |
| crates/launcher/tauri.conf.json | Removes Tauri configuration (no longer used). |
| crates/launcher/src/worker.rs | Adds tokio-backed worker translating UI commands into background tasks and events. |
| crates/launcher/src/updater.rs | Removes old per-file patch updater implementation. |
| crates/launcher/src/state.rs | Adds persisted installed-patch ledger and uploaded-log dedupe ledger state files. |
| crates/launcher/src/patch_rdata.rs | Updates and retains .rdata hostname patching logic with refreshed docs/tests. |
| crates/launcher/src/manifest.rs | Adds manifest schema, fetch, and validation, plus blob URL resolution helper. |
| crates/launcher/src/main.rs | Replaces Tauri entrypoint with eframe/egui app boot + tokio runtime. |
| crates/launcher/src/logs.rs | Adds log collection, stable content digest, zip creation, and Azure PUT upload. |
| crates/launcher/src/launch.rs | Adds SGW/Atera detection and process/batch spawning helpers. |
| crates/launcher/src/install.rs | Adds resumable seed/patch download, sha256 verification, zip extraction, and post-install hostname patch. |
| crates/launcher/src/extract.rs | Removes old 7za-based RAR/CAB extraction helper (no longer used). |
| crates/launcher/src/download.rs | Removes old download helper (replaced by new install pipeline). |
| crates/launcher/src/config.rs | Reworks config to manifest_url/server_host/install_path + compile-time SAS option and paths next to exe. |
| crates/launcher/src/commands.rs | Removes Tauri command layer (no longer used). |
| crates/launcher/src/app.rs | Adds egui UI state machine/panels, dispatching worker commands and rendering progress/status. |
| crates/launcher/gen/schemas/capabilities.json | Removes auto-generated Tauri schema artifact (no longer used). |
| crates/launcher/gen/schemas/acl-manifests.json | Removes auto-generated Tauri schema artifact (no longer used). |
| crates/launcher/Cargo.toml | Switches dependencies from Tauri to eframe/egui + reqwest(rustls) + zip/logging/state deps. |
| crates/launcher/capabilities/default.json | Removes Tauri capabilities config (no longer used). |
| crates/launcher/build.rs | Replaces tauri-build with winres-based icon embedding on Windows targets. |
| crates/launcher/binaries/README.md | Removes Tauri sidecar binary instructions (no longer used). |
| crates/launcher/binaries/.gitkeep | Keeps binaries directory placeholder after removing sidecar usage. |
| CLAUDE.md | Updates workspace exclude guidance to include the new egui launcher crate. |
| .github/workflows/launcher-release.yml | Adds workflow_dispatch release pipeline building exe and publishing a GitHub Release. |
| .github/workflows/launcher-release-on-comment.yml | Adds ChatOps /release-launcher dispatcher workflow with permission/merge checks. |
| .github/workflows/launcher-build.yml | Adds path-filtered launcher CI (fmt on ubuntu; clippy/build/nextest on windows). |
Comment on lines
+239
to
+244
| fn reqwest_error_from(resp: &reqwest::Response) -> reqwest::Error { | ||
| // `error_for_status_ref` returns Err only for 4xx/5xx; on the call site | ||
| // we've already verified the status falls outside 2xx, so unwrap_err is | ||
| // safe. | ||
| resp.error_for_status_ref().unwrap_err() | ||
| } |
Comment on lines
+93
to
+101
| /// Resolves a blob path (e.g. `seed/sgw.zip`) against the manifest URL's | ||
| /// container by stripping the final path segment off the manifest URL. | ||
| pub fn blob_url(manifest_url: &str, blob_path: &str) -> String { | ||
| let base = match manifest_url.rsplit_once('/') { | ||
| Some((b, _)) => b, | ||
| None => manifest_url, | ||
| }; | ||
| format!("{base}/{blob_path}") | ||
| } |
Comment on lines
+279
to
+310
| let file = std::fs::File::open(zip_path)?; | ||
| let mut archive = zip::ZipArchive::new(file)?; | ||
| let total = archive.len(); | ||
| for i in 0..total { | ||
| if cancel.is_cancelled() { | ||
| return Err(InstallError::Cancelled); | ||
| } | ||
| let mut entry = archive.by_index(i)?; | ||
| // `enclosed_name()` returns Option<&Path>; clone to PathBuf so the | ||
| // borrow on `entry` ends before we use `entry.is_dir()` / &mut entry. | ||
| let rel = match entry.enclosed_name() { | ||
| Some(p) => p.to_path_buf(), | ||
| None => continue, | ||
| }; | ||
| let out = dest.join(&rel); | ||
| if entry.is_dir() { | ||
| std::fs::create_dir_all(&out)?; | ||
| } else { | ||
| if let Some(parent) = out.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| let mut f = std::fs::File::create(&out)?; | ||
| std::io::copy(&mut entry, &mut f)?; | ||
| } | ||
| let _ = progress.send(Progress::Extracting { | ||
| label: label.to_string(), | ||
| current: i + 1, | ||
| total, | ||
| filename: out.display().to_string(), | ||
| }); | ||
| } | ||
| Ok(()) |
Comment on lines
+68
to
+103
| pub fn compute_content_digest(install_dir: &Path) -> Result<Option<String>, LogError> { | ||
| let files = collect_log_inputs(install_dir); | ||
| if files.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| let binaries = install_dir.join("Binaries"); | ||
| let mut hasher = Sha256::new(); | ||
| for path in &files { | ||
| let rel = rel_in_archive(path, &binaries); | ||
| hasher.update(rel.as_bytes()); | ||
| hasher.update([0u8]); | ||
| let data = std::fs::read(path)?; | ||
| hasher.update(&data); | ||
| hasher.update([0u8]); | ||
| } | ||
| Ok(Some(format!("{:x}", hasher.finalize()))) | ||
| } | ||
|
|
||
| pub fn build_log_zip(install_dir: &Path) -> Result<Option<Vec<u8>>, LogError> { | ||
| let files = collect_log_inputs(install_dir); | ||
| if files.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| let binaries = install_dir.join("Binaries"); | ||
| let mut buf: Vec<u8> = Vec::new(); | ||
| { | ||
| let cursor = std::io::Cursor::new(&mut buf); | ||
| let mut zw = zip::ZipWriter::new(cursor); | ||
| let options: FileOptions<()> = | ||
| FileOptions::default().compression_method(zip::CompressionMethod::Deflated); | ||
| for path in &files { | ||
| let rel = rel_in_archive(path, &binaries); | ||
| zw.start_file(rel, options)?; | ||
| let data = std::fs::read(path)?; | ||
| zw.write_all(&data)?; | ||
| } |
Comment on lines
+61
to
+63
| # Account SAS (simplest — no Entra ID required). For higher security | ||
| # preferential is a user-delegation SAS bound to a service principal. | ||
| az storage account generate-sas \ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.github/workflows/launcher-release.yml:
- Around line 39-43: Update the launcher-release workflow to pin all GitHub
Actions to immutable commit SHAs and disable checkout credential persistence:
replace mutable tags actions/checkout@v6, dtolnay/rust-toolchain@stable,
Swatinem/rust-cache@v2 and actions/upload-artifact@v7 with their respective
commit SHAs, and add with: persist-credentials: false under the actions/checkout
step so credentials are not persisted after checkout; ensure you update the
action references in the same job where those symbols appear.
In `@crates/launcher/src/app.rs`:
- Around line 22-23: The status Vec<String> field is unbounded and should be
replaced or guarded to keep a fixed-cap history (e.g., 500–1000 entries) to
avoid memory growth; change the struct field (status) to a bounded container
(VecDeque<String> or keep Vec<String> but enforce a cap) and update the code
paths that push new status lines (the push/add_status logic that appends to
status and the display code that reads the latest 100 entries) so that after
inserting a new entry you drop/rotate oldest entries when length exceeds the
cap—implement this in the methods that append status (referencing the status
field and any add/push functions) and ensure existing display logic that slices
the last 100 still works with the capped buffer.
In `@crates/launcher/src/config.rs`:
- Around line 14-18: The constant LOG_UPLOAD_SAS_URL currently bakes writable
SAS credentials into the client binary; remove this compile-time secret and
replace it with a runtime flow: stop using the
option_env!("LAUNCHER_LOG_SAS_URL") constant and instead implement a runtime
fetch/getter (e.g., a get_log_upload_sas_url() or LogUploadClient method) that
obtains a short-lived, server-issued SAS token or a signed upload URL from a
trusted backend or relay endpoint with authentication and rate limits; keep the
client UI/logic tolerant of None (disable upload button) and ensure no writable
credentials are embedded in the executable.
In `@crates/launcher/src/install.rs`:
- Around line 188-190: The HTTP error path in download_to_file incorrectly calls
resp.error_for_status_ref().unwrap_err(), which can panic for non-4xx/5xx
statuses; instead, replace the unwrap_err usage by explicitly matching
resp.error_for_status_ref() (or simply avoid assuming an Err) and always convert
the response into a reqwest error via reqwest_error_from(&resp) when
status.is_success() is false and not resumed; update the error branch in
download_to_file to handle both Ok(_) and Err(_) from error_for_status_ref()
without panicking, referencing the download_to_file function and
reqwest_error_from helper to locate the change.
In `@crates/launcher/src/logs.rs`:
- Around line 121-136: The upload_blob function currently constructs a new
reqwest::Client per call which is inefficient; change it to reuse a shared
client by either (A) adding a reqwest::Client parameter (e.g. client:
&reqwest::Client) to upload_blob and use that client for the .put() call (update
all callers to pass a shared client), or (B) create a global lazy/static client
(e.g. with once_cell::sync::Lazy or lazy_static) and use that singleton inside
upload_blob. Keep existing behavior for insert_blob_path and LogError handling
unchanged; ensure function signature and call sites are updated if you choose
the parameter approach.
In `@crates/launcher/src/worker.rs`:
- Around line 1-6: worker.rs lacks unit tests for the async orchestration
(command→event flow) so add a test exercising the core behavior: spawn the
Worker runtime or call the function that accepts Command (use the public API in
worker.rs), send a Command (e.g., install/launch/upload) and assert Events on
the events_rx receiver for progress forwarding, cancellation handling, and that
ledger persistence is invoked after upload completes (mock or instrument the
ledger persistence call). Target symbols: Command, Event, the events_rx receiver
returned/owned by the worker, and the function that dispatches Commands in
worker.rs; if unit-testing is impractical, add a short doc comment in worker.rs
referencing the integration/smoke test file(s) that cover these scenarios and
why unit tests are omitted.
In `@docs/client/sgw-launcher.md`:
- Around line 188-189: Update the markdown link in docs/client/sgw-launcher.md
so the href points to the actual file ../../crates/launcher/icons/icon.ico
(keeping the link text "icon.ico"); locate the sentence that currently links to
../../crates/launcher/icons/ and replace the directory target with the specific
file path ../../crates/launcher/icons/icon.ico so the link resolves to the
embedded icon file referenced by build.rs.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33db2522-ce26-4905-9dc3-bc5214ff5acb
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcrates/launcher/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**crates/launcher/gen/schemas/capabilities.jsonis excluded by!**/gen/**crates/launcher/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**crates/launcher/gen/schemas/linux-schema.jsonis excluded by!**/gen/**
📒 Files selected for processing (34)
.github/workflows/launcher-build.yml.github/workflows/launcher-release-on-comment.yml.github/workflows/launcher-release.ymlCLAUDE.mdcrates/README.mdcrates/launcher/Cargo.tomlcrates/launcher/binaries/.gitkeepcrates/launcher/binaries/README.mdcrates/launcher/build.rscrates/launcher/capabilities/default.jsoncrates/launcher/src/app.rscrates/launcher/src/commands.rscrates/launcher/src/config.rscrates/launcher/src/download.rscrates/launcher/src/extract.rscrates/launcher/src/install.rscrates/launcher/src/launch.rscrates/launcher/src/logs.rscrates/launcher/src/main.rscrates/launcher/src/manifest.rscrates/launcher/src/patch_rdata.rscrates/launcher/src/state.rscrates/launcher/src/updater.rscrates/launcher/src/worker.rscrates/launcher/tauri.conf.jsoncrates/launcher/ui/index.htmlcrates/launcher/ui/main.jscrates/launcher/ui/styles.cssdocs/client/launcher-guide.mddocs/client/launcher-storage-setup.mddocs/client/sgw-launcher.mddocs/plans/2026-03-06-sgw-launcher-design.mddocs/plans/2026-03-06-sgw-launcher-plan.mddocs/readme.md
💤 Files with no reviewable changes (10)
- crates/launcher/ui/styles.css
- crates/launcher/capabilities/default.json
- crates/launcher/ui/index.html
- crates/launcher/binaries/README.md
- crates/launcher/src/extract.rs
- crates/launcher/src/updater.rs
- crates/launcher/ui/main.js
- crates/launcher/src/commands.rs
- crates/launcher/src/download.rs
- crates/launcher/tauri.conf.json
Comment on lines
+1
to
+6
| //! Background worker. | ||
| //! | ||
| //! Hosts the tokio runtime and turns UI [`Command`]s into spawned tasks | ||
| //! that emit [`Event`]s back on an unbounded channel. The egui app | ||
| //! polls the channel each frame via `events_rx.try_recv()`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
No unit tests for worker orchestration.
The coding guidelines require tests for runtime behavior changes. While install.rs, launch.rs, and logs.rs have their own tests, worker.rs has none. The orchestration logic (especially the progress forwarding, cancellation handling, and ledger persistence after upload) could benefit from at least a basic test exercising the command→event flow.
If testing async orchestration is impractical here, consider documenting which integration or smoke tests cover this layer.
🤖 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 `@crates/launcher/src/worker.rs` around lines 1 - 6, worker.rs lacks unit tests
for the async orchestration (command→event flow) so add a test exercising the
core behavior: spawn the Worker runtime or call the function that accepts
Command (use the public API in worker.rs), send a Command (e.g.,
install/launch/upload) and assert Events on the events_rx receiver for progress
forwarding, cancellation handling, and that ledger persistence is invoked after
upload completes (mock or instrument the ledger persistence call). Target
symbols: Command, Event, the events_rx receiver returned/owned by the worker,
and the function that dispatches Commands in worker.rs; if unit-testing is
impractical, add a short doc comment in worker.rs referencing the
integration/smoke test file(s) that cover these scenarios and why unit tests are
omitted.
Collaborator
Category 1: Security Concerns (G1-G3)G1: Zip-slip protection comment neededLocation: The code correctly uses Risk: A future maintainer might "simplify" to Recommendation: // SECURITY: enclosed_name() is critical here — it rejects absolute paths
// and path traversal (../) to prevent zip-slip attacks. Do NOT replace
// with entry.name() or entry.mangled_name().
let Some(relative) = entry.enclosed_name() else {
continue;
};Effort: 2 minutes G2: SAS URL baked into binaryLocation: The SAS URL is compiled into the binary. If leaked (decompilation, memory dump), anyone can upload arbitrary blobs to your Azure container. The SAS has Options:
Recommendation: Option 1 is fine for a game launcher. Document the risk and set up monitoring. Effort: 30 minutes (documentation + Azure alert) G3: No hostname validation in patch_rdataLocation: No validation that Recommendation: fn is_valid_hostname(s: &str) -> bool {
s.len() <= 22
&& s.chars().all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-')
&& !s.starts_with('-')
&& !s.ends_with('-')
}Effort: 15 minutes Category 1 Total Effort: ~45 minutes |
Collaborator
Category 2: Error Handling (G4-G6)G4: Range request handling — ACTUALLY FINELocation: I initially flagged this as a bug, but on closer inspection the code is correct: let mut file = if resumed && existing_len > 0 {
tokio::fs::OpenOptions::new().append(true).open(dest).await?
} else {
tokio::fs::File::create(dest).await? // truncates and starts fresh
};If the server returns 200 (doesn't support Range), Status: No action needed. G5: Unbounded event channelLocation: The event channel from worker to UI is unbounded. If the UI thread stalls (e.g., during a long egui layout), events pile up unboundedly. Why this is probably fine: The launcher UI is simple — no complex layouts, no blocking operations on the UI thread. Events are small (enum variants with a few fields). You'd need to stall for minutes to accumulate meaningful memory. If you want to fix it: let (tx, rx) = tokio::sync::mpsc::channel(100); // bounded
// Then handle SendError by dropping oldest or blockingEffort: 15 minutes (if desired) G6: Config save failure doesn't block installLocation: Config is saved on every "Install / Update" click. If the save fails, the install still proceeds. The error is logged to status, but the user might not notice. Current behavior: if let Err(e) = self.config.save() {
self.status.push(format!("Warning: failed to save config: {e}"));
}
// install proceeds anywayWhy this might be intentional: Config save failing (disk full, permissions) shouldn't block the user from installing the game. The config will be re-saved on next successful operation. If you want stricter behavior: if let Err(e) = self.config.save() {
self.status.push(format!("ERROR: Cannot save config: {e}"));
return; // block install
}Effort: 5 minutes (if desired) Category 2 Total Effort: ~20 minutes (all optional) |
Collaborator
Category 3: Missing Test Coverage (G7-G10)G7: No test for download resume pathLocation: The resume logic (Range header, 206 status, append mode) is the most complex code path in the file, but it's untested. Recommendation: #[tokio::test]
async fn download_resumes_partial_file() {
// Start a mock server that supports Range requests
let server = MockServer::start().await;
let full_content = b"0123456789ABCDEF";
Mock::given(method("GET"))
.and(header("Range", "bytes=8-"))
.respond_with(ResponseTemplate::new(206)
.body(&full_content[8..])
.insert_header("Content-Range", "bytes 8-15/16"))
.mount(&server).await;
// Create partial file with first 8 bytes
let dir = tempfile::tempdir().unwrap();
let dest = dir.path().join("test.bin");
std::fs::write(&dest, &full_content[..8]).unwrap();
// Resume should append remaining bytes
download_to_file(&server.uri(), &dest, &tx, token).await.unwrap();
assert_eq!(std::fs::read(&dest).unwrap(), full_content);
}Effort: 45 minutes (including mock server setup) G8: No test for cancellation during download/extractLocation: The Recommendation: #[tokio::test]
async fn download_respects_cancellation() {
let token = CancellationToken::new();
let token_clone = token.clone();
// Cancel after 100ms
tokio::spawn(async move {
tokio::time::sleep(Duration::from_millis(100)).await;
token_clone.cancel();
});
// Start a slow download
let result = download_to_file(&slow_server_url, &dest, &tx, token).await;
assert!(result.is_err());
// Partial file should exist but be incomplete
}Effort: 30 minutes G9: No tests for worker.rsLocation: The worker is the glue between UI and async operations. It's simple (just a command dispatcher), but untested. What to test:
Effort: 1 hour G10: No tests for UI state transitionsLocation: The
These are untested. UI state machines are notoriously buggy. Why this is hard to test: egui apps are typically tested via screenshot comparison or manual testing, not unit tests. The state transitions could be extracted into a pure function for testing. Effort: 1-2 hours (if desired) Category 3 Total Effort: ~3-4 hours |
Collaborator
Category 4: UX Concerns (G11-G12)G11: Status log memory leakLocation: The status log display is capped at 100 lines, but the underlying // Display caps at 100
for line in self.status.iter().rev().take(100) {
ui.label(line);
}
// But the Vec keeps growing
self.status.push(format!("Downloaded {} bytes", n));Over a long session with many operations, this leaks memory. Recommendation — Ring buffer: const MAX_STATUS_LINES: usize = 100;
fn push_status(&mut self, msg: String) {
if self.status.len() >= MAX_STATUS_LINES {
self.status.remove(0);
}
self.status.push(msg);
}Or use Effort: 10 minutes G12: No download speed or ETA displayLocation: The progress bar shows bytes downloaded and total, but not:
For a multi-GB seed download, users want to know how long they'll be waiting. Recommendation: struct DownloadProgress {
started_at: Instant,
bytes_downloaded: u64,
bytes_total: u64,
}
impl DownloadProgress {
fn speed_mbps(&self) -> f64 {
let elapsed = self.started_at.elapsed().as_secs_f64();
if elapsed > 0.0 {
(self.bytes_downloaded as f64 / 1_000_000.0) / elapsed
} else {
0.0
}
}
fn eta_seconds(&self) -> Option<u64> {
let speed = self.speed_mbps();
if speed > 0.0 {
let remaining = self.bytes_total - self.bytes_downloaded;
Some((remaining as f64 / (speed * 1_000_000.0)) as u64)
} else {
None
}
}
}Effort: 30 minutes Category 4 Total Effort: ~40 minutes |
Collaborator
Category 5: Documentation (G13-G14)G13: No schema versioning strategy documentedLocation: The code rejects Questions a future maintainer will have:
Recommendation: Add to ## Schema Versioning
The manifest `schema` field is an integer version number.
### When to bump the schema version:
- Adding required fields → bump
- Removing fields → bump
- Changing field semantics → bump
- Adding optional fields → no bump needed
### Backwards compatibility:
The launcher supports only the current schema version. When bumping:
1. Update `SUPPORTED_SCHEMA` constant
2. Update manifest generation in CI
3. All users must update their launcher
### Current schema: 1Effort: 20 minutes G14: No troubleshooting section in launcher guideLocation: Users will encounter issues. The guide should help them self-diagnose. Recommendation: Add a troubleshooting section: ## Troubleshooting
### "Failed to fetch manifest"
- Check your internet connection
- Verify the manifest URL is accessible in a browser
- Check if your firewall/antivirus is blocking the launcher
### "Hash mismatch" after download
- The download may have been corrupted
- Click "Install / Update" to re-download
- If persistent, the CDN cache may be stale — wait 15 minutes
### "SGW.exe won't launch"
- Ensure Visual C++ Redistributable 2015-2022 is installed
- Try running as Administrator
- Check Windows Event Viewer for crash details
### "Stuck at 0%" download
- Large seed files may take time to start
- Check if another download is in progress
- Try cancelling and restarting
### Logs location
Launcher logs are stored in: `<install_dir>/Logs/`
Upload them via the "Upload Logs" button for support.Effort: 30 minutes Category 5 Total Effort: ~50 minutes Summary
Priority
Total: 14 gaps, ~6 hours of follow-up work, none blocking. |
Addresses reviewer feedback from MyPalClara, CodeRabbit, and Copilot on PR #343. All items either flagged by multiple reviewers or classified as "quick win" by the human reviewer. Code: - app.rs: cap status history at MAX_STATUS_LINES (1000) via push_status() helper. Was unbounded though display capped at 100. Flagged by both MyPalClara (G11) and CodeRabbit. - install.rs: replace the panic-prone unwrap_err() on error_for_status_ref() with a new InstallError::UnexpectedStatus variant. The old path would panic on a stray 1xx/3xx (unlikely given reqwest auto-follows redirects, but the comment claimed invariants the code didn't actually enforce). Flagged by both Copilot and CodeRabbit. - install.rs: SECURITY comment above the enclosed_name() call in extract_zip explaining it's the zip-slip gate. (MyPalClara G1). - manifest.rs: blob_url now strips ?query and #fragment from the manifest URL before splitting on the last "/", so a SAS-tokened manifest URL produces a valid blob URL. (Copilot.) - patch_rdata.rs: validate hostname before writing into SGW.exe's .rdata — ASCII alphanumeric + "." + "-", no leading/trailing "-" or ".", non-empty. New PatchError::InvalidHostname variant. (MyPalClara G3.) Workflows: - launcher-release.yml: pin actions/checkout, dtolnay/rust-toolchain, Swatinem/rust-cache, actions/upload-artifact to immutable commit SHAs (with version comments for human readability), and add persist-credentials: false on actions/checkout. This is the privileged workflow that consumes the LAUNCHER_LOG_SAS_URL secret and creates GitHub Releases — the supply-chain surface from third-party actions matters most here. (CodeRabbit, with zizmor cross-reference.) - launcher-build.yml: add persist-credentials: false on every actions/checkout step (4 jobs). Doesn't touch secrets but cheap defence-in-depth. - launcher-release-on-comment.yml: no actions/checkout used (only gh API calls), so nothing to harden there. Docs: - sgw-launcher.md: fix icon link to point at icons/icon.ico instead of the icons/ directory. (CodeRabbit.) - launcher-storage-setup.md: fix "For higher security preferential is..." grammar. (Copilot.) Add Threat-Model section documenting the SAS-in-binary trade-off, with required operator action items (storage cost cap, upload-rate anomaly alert, 6-month rotation cadence). (MyPalClara G2 + CodeRabbit; user decision: accept the risk + document rather than build a server-issued token broker.) - launcher-guide.md: new Schema Versioning Policy section under Step 3 — Write the Manifest. Covers when to bump vs not, the multi-schema transition procedure, and pointers for migrations that don't fit the additive-optional shape. (MyPalClara G13.) - launcher-guide.md: new Troubleshooting section covering manifest errors, hash mismatch, HTTP status codes, launch failures, SAS expiry, dedupe-skipped uploads, and where to find diagnostic files. (MyPalClara G14.) Tests: - 9 new tests (40 → 49). Cover push_status cap behaviour, blob_url query/fragment stripping (3 cases), hostname validation (empty / disallowed chars / leading-or-trailing dash-or-dot / typical DNS names). Verified locally on Windows: cargo fmt -p sgw-launcher -- --check exit 0 cargo check -p sgw-launcher OK cargo clippy -p sgw-launcher --all-targets -- -D warnings clean cargo test -p sgw-launcher 49/49 pass Deferred to follow-up issues (per reviewer classification and user direction): - G7/G8: integration tests for download resume + cancellation paths - G9/G10: worker.rs / app.rs UI state-machine tests - Copilot: extract_zip + log builder on spawn_blocking / streaming file reads — perf, not correctness; current sizes are bounded - G12: download speed + ETA display - SAS broker endpoint (the "Major" CodeRabbit asked for) — needs a server-side issuer that does not exist yet; accept-and-document is the chosen disposition for this PR Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Review feedback addressed in
|
Contributor
Does it do what it says on the tin?Mostly yes. Verified against the code:
Bug — hostname re-patch when
|
Contributor
Feature gaps vs. what a real launcher needsSolid v1. Gap matrix vs. typical MMO launcher functionality:
The two biggest — self-update and repair/verifyWithout self-update, every launcher bug = a "please redownload from GH Releases" announcement. Both inexpensive. Self-update sketch: // New manifest field
pub struct Manifest {
...
#[serde(default)]
pub launcher: Option<LauncherEntry>,
}
pub struct LauncherEntry {
pub version: String,
pub blob: String,
pub sha256: String,
}// In LauncherApp::new, after manifest fetch:
if let Some(l) = &manifest.launcher {
if l.version > env!("CARGO_PKG_VERSION") {
// download to sgw-launcher.new.exe, rename current to .old,
// rename new to current, std::process::Command::new(current).spawn(),
// std::process::exit(0). Standard Windows self-update pattern.
}
}Repair sketch — store per-file sha for the seed: // InstalledState gains
pub struct InstalledState {
...
#[serde(default)]
pub seed_files: std::collections::HashMap<String, String>,
}
// Populated at install time from a manifest of seed contents (publish step
// emits this alongside seed.zip).
// "Verify" button hashes each tracked file, lists mismatches, re-downloads
// seed.zip and overlays only the bad files.Admin-rights / install pathDefault install path is Pick one:
fn install_dir_writable(dir: &Path) -> bool {
let _ = std::fs::create_dir_all(dir);
let probe = dir.join(".launcher-write-probe");
let ok = std::fs::write(&probe, b"x").is_ok();
let _ = std::fs::remove_file(&probe);
ok
}Gate the Install button on this, with a friendly error when it fails. Atomic installMid-patch extract failure currently leaves half-applied state. Wrap each patch in a staging dir + rename: async fn apply_patch(ctx: &InstallContext<'_>, patch: &PatchEntry) -> Result<(), InstallError> {
let staging = ctx.install_dir.join(format!(".staging-{}", safe_id(&patch.id)));
std::fs::create_dir_all(&staging)?;
// download + verify into .tmp-*, extract into staging
extract_zip(&tmp, &staging, ...)?;
// Only after extract succeeds, overlay staging onto install_dir.
move_overlay(&staging, ctx.install_dir)?;
std::fs::remove_dir_all(&staging).ok();
Ok(())
}
Single-instance lock// main.rs - before eframe::run_native
use fs2::FileExt;
let lock_path = config::exe_dir().join("launcher.lock");
let lock = std::fs::File::create(&lock_path)?;
if lock.try_lock_exclusive().is_err() {
// Show a native message box or just stderr + exit
eprintln!("Another Stargate Worlds launcher is already running.");
std::process::exit(1);
}
// hold `lock` for process lifetimeAdd |
Contributor
Security improvements beyond the SAS4a. Manifest is unsigned — biggest gap
Remediation — embed an Ed25519 pubkey, ship detached signature: // manifest.rs
const SIGNING_PUBKEY: &[u8; 32] = include_bytes!("../keys/manifest-signing.pub");
pub async fn fetch_manifest(client: &reqwest::Client, url: &str)
-> Result<Manifest, ManifestError>
{
let body = client.get(url).send().await?.error_for_status()?.bytes().await?;
let sig = client.get(format!("{url}.sig")).send().await?
.error_for_status()?.bytes().await?;
use ed25519_dalek::{Signature, VerifyingKey, Verifier};
let vk = VerifyingKey::from_bytes(SIGNING_PUBKEY.try_into().unwrap())?;
let signature = Signature::from_slice(&sig)?;
vk.verify(&body, &signature).map_err(|_| ManifestError::BadSignature)?;
let manifest: Manifest = serde_json::from_slice(&body)?;
manifest.validate()?;
Ok(manifest)
}Add Operator workflow change: signing step in the publish flow runs This is the single highest-leverage security improvement. Removes manifest tampering and seed/patch substitution from the threat model. Anonymous-read on the container is fine once the manifest is signed. 4b. Replay protection — monotonic versionOnce signed, the remaining attack is replaying an old (validly signed) manifest to roll back patches. Defense: pub struct Manifest {
pub schema: u32,
pub version: u64, // monotonic
pub published_at: chrono::DateTime<chrono::Utc>,
pub seed: SeedEntry,
pub patches: Vec<PatchEntry>,
}
// state.rs
pub struct InstalledState {
...
#[serde(default)]
pub highest_manifest_version: Option<u64>,
}
// install.rs - reject regressions
if let Some(seen) = state.highest_manifest_version {
if manifest.version < seen {
return Err(InstallError::ManifestReplay {
seen, got: manifest.version,
});
}
}
state.highest_manifest_version = Some(manifest.version);4c.
|
Contributor
Alternatives to SAS / anonymous auth for blob hostingRanked by cost vs. security gain. Option A — GitHub Releases as the CDN (cheapest, zero infra)The launcher itself is already shipped via GH Releases. Ship
Implementation sketch: # launcher-release.yml additions
- name: build seed + patches
run: ./tools/build-seed.sh # already-existing publish flow
- name: attest provenance
uses: actions/attest-build-provenance@v1
with:
subject-path: 'release/seed.zip,release/patches/*.zip,release/manifest.json'
- name: upload assets
run: |
gh release upload "${TAG}" release/seed.zip release/patches/*.zip release/manifest.jsonOption B — Cloudflare R2 + Worker auth (moderate cost, strong)R2 has zero egress fees. Tiny Worker validates a launcher-supplied token (signed by your auth server, ~5min TTL) and either redirects to a signed R2 URL or proxies bytes.
Option C — Azure Front Door + Function gateway (Azure-native)Container fully private; Function App validates and rewrites to authenticated blob URLs.
Option D — Self-hosted nginx with signed-URL middleware
Option E — Sign the manifest, keep blobs anonymousCovered in Security comment §4a. Highest-leverage, lowest cost (~50 LOC + a signing step in publish flow). Anonymous-read on Azure is fine afterward. Recommendation
What a future ADR should captureDrop into
|
Contributor
Code-level issues6a.
|
Contributor
Test gapsCoverage of the building blocks is strong (40 tests, well-targeted unit-level). Orchestration is what's missing — and §2's hostname re-patch bug would have been caught by a single regression test. 7a. End-to-end
|
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/launcher-build.yml (2)
41-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse workspace-wide rustfmt check in CI.
Line 43 currently checks only one package; this can let cross-crate formatting drift through. Please switch to
cargo fmt --all -- --checkto match repo policy.As per coding guidelines
**/*.rs: “Always runcargo fmt --all -- --checkbefore submitting a PR to ensure code formatting passes CI gates.”🤖 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 @.github/workflows/launcher-build.yml around lines 41 - 46, The CI rustfmt step currently runs "cargo fmt -p sgw-launcher -- --check" which only checks one package; change that invocation to run workspace-wide formatting with "cargo fmt --all -- --check" so the job verifies all crates follow repo policy and prevents cross-crate formatting drift.
61-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign clippy step with the repository-required invocation.
Line 62 runs clippy only for
sgw-launcher, but the required policy is the workspace command with explicit excludes. Please update this job to run the exact repository-standard clippy command.As per coding guidelines
**/*.rs: “Runcargo clippy --workspace --exclude cimmeria-app --exclude cimmeria-content-editor --exclude cimmeria-scene-editor --exclude sgw-launcher --all-targets -- -D warningsbefore submitting a PR.”🤖 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 @.github/workflows/launcher-build.yml around lines 61 - 63, The clippy job currently runs `cargo clippy -p sgw-launcher --all-targets -- -D warnings` (job name "cargo clippy"); replace that run command with the repository-standard workspace invocation: run `cargo clippy --workspace --exclude cimmeria-app --exclude cimmeria-content-editor --exclude cimmeria-scene-editor --exclude sgw-launcher --all-targets -- -D warnings` so the workflow runs clippy across the workspace with the explicit excludes required by policy.crates/launcher/src/install.rs (2)
174-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
416 Range Not Satisfiablefor already-complete temp files.When
existing_len > 0, a fully downloaded temp file can trigger416on resume; current logic returnsUnexpectedStatusand blocks progress. Treat416as recoverable (e.g., verify local file hash and continue, or reset temp file and retry from zero).🤖 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 `@crates/launcher/src/install.rs` around lines 174 - 200, The code currently treats any non-success status (except 206) as fatal; add handling for 416 when resuming: after getting resp and computing status/resumed, if status.as_u16() == 416 && existing_len > 0 then treat as recoverable — either verify the local temp file (e.g., compare its length or hash to the expected content length/ETag from a HEAD or the response) and return success, or delete/reset the temp file and retry the download without the Range header (re-create req via reqwest::Client::new().get(url) and resend). Update the conditional that returns InstallError::UnexpectedStatus to exclude the 416-resumable path and use the symbols existing_len, resp/status/resumed, and InstallError::UnexpectedStatus to locate the change.
179-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit HTTP timeout to download requests.
Line 183 performs a network call with no timeout, so a stalled connection can hang install/update indefinitely. Set a request/client timeout and map timeout failures into
InstallError.🤖 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 `@crates/launcher/src/install.rs` around lines 179 - 184, The download request currently uses reqwest::Client::new().get(url) with no timeout; create the client with an explicit timeout (e.g., using reqwest::Client::builder().timeout(Duration::from_secs(...)).build() or set a per-request timeout via the RequestBuilder.timeout(...) call) and replace the existing Client::new() usage around the `req` variable. Also update the await/send error handling for `resp = req.send().await` to map timeout errors into the crate's InstallError (detect via the reqwest error's is_timeout() or matching the error kind) and return a clear InstallError variant (e.g., InstallError::Timeout) while mapping other errors as before; reference `req`, `existing_len`, `resp`, and `InstallError` to locate the changes.
🤖 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 `@crates/launcher/src/install.rs`:
- Around line 195-199: Add a regression test that triggers the new
unexpected-status branch by simulating a non-2xx/non-206 HTTP response and
asserting that the call returns InstallError::UnexpectedStatus; locate the test
alongside other tests for install.rs (e.g., in a #[cfg(test)] mod or integration
test) and call the function that exercises the path that checks
status/is_success() with resumed=false so the code path that returns
InstallError::UnexpectedStatus (referencing InstallError::UnexpectedStatus, the
status and resumed variables and the url argument) is executed, using a mock
HTTP client or test server to return a non-2xx/non-206 status and verifying the
error variant and contained status/url values.
In `@crates/launcher/src/patch_rdata.rs`:
- Around line 25-26: Update the InvalidHostname error text to accurately reflect
the validation rules: state that allowed characters are ASCII alphanumeric, '.'
and '-', and that hostnames must not start or end with either '.' or '-'. Modify
the message for the InvalidHostname variant (InvalidHostname { addr: String })
and the related error occurrences around the other block (lines 43-47) so
logs/UI show "no leading/trailing '.' or '-'" instead of only mentioning '-' to
match the actual validation logic.
In `@docs/client/launcher-guide.md`:
- Around line 435-438: Update the troubleshooting heading and copy that
currently reads "Install failed: Unexpected HTTP 4xx/5xx for …" to reflect the
installer now raises "Unexpected HTTP <status>" for any non-success response;
change the wording (the heading and the explanatory line about the seed or patch
blob URL) to state "non-success HTTP status" or "Unexpected HTTP <status>" so it
no longer implies only 4xx/5xx errors and accurately describes the installer
behavior.
---
Outside diff comments:
In @.github/workflows/launcher-build.yml:
- Around line 41-46: The CI rustfmt step currently runs "cargo fmt -p
sgw-launcher -- --check" which only checks one package; change that invocation
to run workspace-wide formatting with "cargo fmt --all -- --check" so the job
verifies all crates follow repo policy and prevents cross-crate formatting
drift.
- Around line 61-63: The clippy job currently runs `cargo clippy -p sgw-launcher
--all-targets -- -D warnings` (job name "cargo clippy"); replace that run
command with the repository-standard workspace invocation: run `cargo clippy
--workspace --exclude cimmeria-app --exclude cimmeria-content-editor --exclude
cimmeria-scene-editor --exclude sgw-launcher --all-targets -- -D warnings` so
the workflow runs clippy across the workspace with the explicit excludes
required by policy.
In `@crates/launcher/src/install.rs`:
- Around line 174-200: The code currently treats any non-success status (except
206) as fatal; add handling for 416 when resuming: after getting resp and
computing status/resumed, if status.as_u16() == 416 && existing_len > 0 then
treat as recoverable — either verify the local temp file (e.g., compare its
length or hash to the expected content length/ETag from a HEAD or the response)
and return success, or delete/reset the temp file and retry the download without
the Range header (re-create req via reqwest::Client::new().get(url) and resend).
Update the conditional that returns InstallError::UnexpectedStatus to exclude
the 416-resumable path and use the symbols existing_len, resp/status/resumed,
and InstallError::UnexpectedStatus to locate the change.
- Around line 179-184: The download request currently uses
reqwest::Client::new().get(url) with no timeout; create the client with an
explicit timeout (e.g., using
reqwest::Client::builder().timeout(Duration::from_secs(...)).build() or set a
per-request timeout via the RequestBuilder.timeout(...) call) and replace the
existing Client::new() usage around the `req` variable. Also update the
await/send error handling for `resp = req.send().await` to map timeout errors
into the crate's InstallError (detect via the reqwest error's is_timeout() or
matching the error kind) and return a clear InstallError variant (e.g.,
InstallError::Timeout) while mapping other errors as before; reference `req`,
`existing_len`, `resp`, and `InstallError` to locate the changes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ef2bcd1-506c-4848-ba80-6c068c9528cb
📒 Files selected for processing (9)
.github/workflows/launcher-build.yml.github/workflows/launcher-release.ymlcrates/launcher/src/app.rscrates/launcher/src/install.rscrates/launcher/src/manifest.rscrates/launcher/src/patch_rdata.rsdocs/client/launcher-guide.mddocs/client/launcher-storage-setup.mddocs/client/sgw-launcher.md
Fixes the silent failure mode Cady caught on PR #343: editing `server_host` after install left the .exe pointing at the previously- patched host because `needs_patching()` only searched for the original CME literal (which the first patch already overwrote). Changes ------- patch_rdata.rs: - New `patch_hostname_any(data, new_host, previous_host)` that falls back to searching for `previous_host` (zero-padded to the 22-byte slot) when the original CME literal is gone. Old `patch_hostname` is removed from the public surface — tests use a #[cfg(test)] shim. - New `host_differs(data, expected, previous)` predicate the installer uses to decide whether to re-patch. - Updated `InvalidHostname` error text to mention both `.` and `-` (validation rejected both but message only mentioned `-`). Closes CodeRabbit #2. state.rs: - `InstalledState.patched_host: Option<String>` records which host was last written into the .exe. Defaults to `None` for legacy state files (forward-compat tested). install.rs: - install_all calls `host_differs` + `patch_exe_any` with the persisted `patched_host`, then writes the new host back to state. - Re-seed clears patched_host (the new seed has the original CME literal, so we patch fresh). - New regression test `download_to_file_returns_unexpected_status_on_non_2xx` using wiremock — guards the non-panicking HTTP status branch added in the prior fixup. Closes CodeRabbit #1. app.rs: - Install panel surfaces "Server host changed — SGW.exe needs re-patching" and re-enables Install/Update when the only thing that drifted is the hostname. Without this, the UI hid the button after a successful install and the user had no in-launcher path to re-patch. docs/client/launcher-guide.md: - Troubleshooting heading: "Unexpected HTTP <status>" instead of "4xx/5xx". Closes CodeRabbit #3. Tests: 49 → 56 (+7). Adds wiremock dev-dep for HTTP mocking. Verified locally on Windows: cargo fmt -p sgw-launcher -- --check OK cargo clippy -p sgw-launcher --all-targets -- -D warnings clean cargo test -p sgw-launcher 56/56 pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Cady #5 (code-level) and #4g (LauncherConfig schema drift), plus #2 admin-rights and #6h ledger pruning. Atomic state writes ------------------- state.rs exports an `atomic_write(path, bytes)` helper that stages to `<path>.json.tmp` and renames onto the target. Used by InstalledState, UploadedLedger, and LauncherConfig persistence. Power loss between the truncate and the finish either leaves the old content or the new content — never a half-written JSON that the load path treats as "missing, fall back to default" (which would silently mean "nothing installed" → multi-GB re-download). NTFS rename is atomic on the same volume. LauncherConfig hardening ------------------------ - `install_path: String` → `install_path: PathBuf` (Cady #6a). The egui TextEdit widget still binds to a `String` UI buffer (`LauncherApp::install_path_text`), synced into `config.install_path` on Save. Catches non-UTF-16-safe Unicode boundaries on Windows paths that would have hit `String` representations badly. - `load(&PathBuf)` / `save(&PathBuf)` → `&Path` (Cady #6b, clippy::ptr_arg). - New `schema_version: u32` field with `#[serde(default)]` for forward- compat (Cady #4g). Legacy config files without the field load with `schema_version = CONFIG_SCHEMA_VERSION`. Future-schema configs fail with `ConfigError::UnsupportedSchema { got, expected }` instead of silently coercing — protects fields the current struct doesn't know about. - Default install path now `%LOCALAPPDATA%\Stargate Worlds` instead of `%ProgramFiles%\Stargate Worlds` so the launcher can write without UAC elevation (Cady #2 admin-rights gap). Falls back to `$HOME/.local/share/Stargate Worlds` for non-Windows dev builds. - Default manifest URL flipped to the GitHub Releases convention (`content-current` rolling tag) ahead of the GH Releases hosting pivot. The Azure-Blob URL convention still works — `blob_url()` handles both — and the operator runbook switch lands in the dedicated hosting-pivot commit. UploadedLedger pruning ---------------------- `record()` drops oldest entries beyond `LEDGER_KEEP = 100`. Each entry is ~100 bytes, so the ledger file stays under ~10 KB even after thousands of upload clicks. Pruning is transparent to dedupe checks: `contains()` is "have we seen this digest in the last 100 uploads?" which is sufficient for the wallet-protection goal. Tests: 56 → 58. New legacy-load tests for both InstalledState and LauncherConfig, plus unsupported-schema rejection. Verified locally on Windows: cargo fmt -p sgw-launcher -- --check OK cargo clippy -p sgw-launcher --all-targets -- -D warnings clean cargo test -p sgw-launcher 58/58 pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… tmp, atomic exe write
Addresses Cady #6f, #6g, #6i and #4c.
Shared reqwest::Client (Cady #6g)
---------------------------------
Worker owns one `reqwest::Client::builder().https_only(true).build()`
and clones the handle into every spawned task. Reuses the connection
pool across the seed download, all patch downloads, and the log
upload — previously each call built its own client and discarded the
pool. https_only(true) defends against http:// downgrade attacks and
catches misconfigured manifest URLs at the client layer (in addition
to the in-progress explicit URL prefix check landing in the security
hardening commit).
Signature changes:
fetch_manifest(url) → fetch_manifest(&http, url)
upload_blob(sas, blob, body) → upload_blob(&http, sas, blob, body)
download_to_file(url, ...) → download_to_file(&http, url, ...)
InstallContext { ... } → adds `http: &reqwest::Client`
Cancel token per install (Cady #6f)
-----------------------------------
Worker now owns `current_install_cancel: Option<CancellationToken>`.
Each `spawn_install` allocates a fresh token; `Command::Cancel` takes
and cancels whichever token is current. Previously a single token was
shared across all installs, so a double-click that fired two installs
and then a cancel would kill both (the new install would inherit the
already-cancelled token and exit immediately on first chunk).
The token is `take()`-d on cancel rather than cloned, so a subsequent
cancel is a no-op until the next install allocates a new token.
Sha-keyed tmp filename (Cady #6i)
---------------------------------
`.tmp-patch-{id}.zip` → `.tmp-patch-{id}-{sha12}.zip`. If an operator
republishes a patch with the same id but a new sha (operationally
wrong, but possible during a hot-fix), the launcher used to resume
the stale tmp via Range, SHA-verify fails, and the user had to
manually delete the tmp file. With the sha in the filename, different
sha → different tmp path → fresh download.
Atomic SGW.exe write (Cady #4c)
-------------------------------
`patch_exe_any` writes to `<exe_path>.exe.patching` then renames onto
SGW.exe. NTFS rename is atomic on the same volume, so a power loss
between the truncate and the finish leaves either the old SGW.exe or
the new one — never a corrupt .exe that would force a multi-GB
re-seed for recovery.
Verified locally on Windows:
cargo fmt -p sgw-launcher -- --check OK
cargo clippy -p sgw-launcher --all-targets -- -D warnings clean
cargo test -p sgw-launcher 58/58 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, path-prefix check, writability probe Addresses Cady #2 (admin-rights gap, single-instance lock), #4d (HTTPS enforcement), and #4h (launch path-prefix check). Single-instance file lock (Cady #2 / #4e) ----------------------------------------- `main.rs` acquires an exclusive `fs4::FileExt::try_lock_exclusive` on `<exe dir>/launcher.lock` before constructing any state. A second launcher process fails the try-lock and exits with code 3 + a stderr message. Previously two instances could race on `launcher-installed.json` + `.tmp-*.zip` files: one declaring patches applied while the other was still extracting them, etc. Lock release happens automatically on process drop. The `File` is held for the full process lifetime via an explicit `drop(lock_file)` at the bottom of `main`. HTTPS-only enforcement (Cady #4d) --------------------------------- - `manifest::fetch_manifest` rejects non-`https://` URLs with `ManifestError::InsecureUrl(url)` before issuing the request. - `logs::upload_blob` rejects non-`https://` SAS base URLs with `LogError::InsecureUrl` before issuing the PUT. - Belt-and-braces with the worker's `Client::builder().https_only(true)` added in the previous commit. The explicit prefix check produces a friendlier user-facing error than reqwest's policy enforcement when a user types `http://...` in the manifest URL field. Launch path-prefix sanity check (Cady #4h) ------------------------------------------ `launch::spawn` now canonicalizes both `install_dir` and the resolved target path, then asserts `target.starts_with(install_dir)`. If the install dir is a junction or the target resolves through a symlink to an unexpected location, the spawn fails with `LaunchError::PathEscape { install_dir, target }` instead of running the binary with the install dir's cwd. Not a privilege boundary (the user chose `install_dir`); cheap defence-in-depth against config-injected paths. Writability probe (Cady #2) --------------------------- New `launch::install_dir_writable(&Path) -> bool` tries to create the dir, write a probe file, remove it, and reports whether all three succeeded. The Install panel calls this on every frame; the Install / Update button is disabled with a red error label when the chosen directory isn't writable. Without this, picking `C:\Program Files\…` without UAC produces an opaque mid-extract "Access denied" after gigabytes of seed download. Probe file is always cleaned up; check survives missing parents (creates them). New tests: 58 → 62. - manifest::fetch_manifest_rejects_non_https - logs::upload_blob_rejects_non_https - launch::install_dir_writable_succeeds_on_temp - launch::install_dir_writable_creates_missing_parents Verified locally on Windows: cargo fmt -p sgw-launcher -- --check OK cargo clippy -p sgw-launcher --all-targets -- -D warnings clean cargo test -p sgw-launcher 62/62 pass Adds dep: fs4 = "0.13" (maintained fork of fs2, used for file lock). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#4a) Closes the manifest-tampering hole on PR #343: anyone who could MITM or compromise the manifest host could previously ship arbitrary seed/patch payloads because SHA verification of the artifacts was vacuous — the launcher trusted whatever hashes the (untrusted) manifest claimed. Cady ranked this "the single highest-leverage security improvement" in his review. Mechanism --------- - `fetch_manifest` now fetches both `manifest.json` and `manifest.json.sig` (conventional `.sig` suffix). Signature file is hex-encoded (128 chars = 64 raw bytes) — chosen over raw binary so the file is grep-able in CI logs. - Signature is verified against [`MANIFEST_SIGNING_PUBKEY`] — an embedded Ed25519 verifying key. - If sig fetch or verify fails, the manifest is rejected. No unsigned fallback. Key embedding ------------- - `MANIFEST_SIGNING_PUBKEY` is a `LazyLock<VerifyingKey>` that reads `LAUNCHER_MANIFEST_PUBKEY_HEX` at compile time via `option_env!`. Production builds bake the operator's real pubkey from a repo secret. - Default (no env override) derives the pubkey from `[0x2a; 32]` — a publicly-known dev private key, intentionally insecure so dev / PR / CI builds work without operator setup. Dev builds must never be released to players; the release workflow (next commit) injects the real key. - `LAUNCHER_MANIFEST_PUBKEY_HEX` wiring into `launcher-release.yml` + operator documentation lands in a later commit alongside the GH Releases hosting pivot. Why this works without infrastructure changes --------------------------------------------- Anonymous-read on the blob host is fine once the manifest is signed — an attacker tampering with the in-transit manifest can't produce a valid signature without the offline private key. Seed/patch blob integrity still rides on the SHA-256s _in the now-authenticated_ manifest. SAS-in-binary mitigation (the log upload SAS) is unaffected — it's still bounded by the threat model documented in the storage runbook. Code ---- - New `ManifestError::BadSignature` (verification failed) and `BadSignatureFormat(&'static str)` (malformed hex / wrong length). - Helper `hex_decode_32` / `hex_decode_64` (tiny inline impl, no new hex-crate dep). - `verify_manifest_signature(body, sig_hex)` exposed for tests + potential reuse. Tests: 62 → 67. New coverage: - verify_manifest_signature_accepts_signed_body - verify_manifest_signature_rejects_wrong_body - verify_manifest_signature_rejects_wrong_key - verify_manifest_signature_rejects_malformed_hex - hex_decode_32_round_trip Verified locally on Windows: cargo fmt -p sgw-launcher -- --check OK cargo clippy -p sgw-launcher --all-targets -- -D warnings clean cargo test -p sgw-launcher 67/67 pass Adds deps: ed25519-dalek = "2" (alloc only — no rand/pkcs8 features needed since the launcher only verifies). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Option A)
Pivots the content hosting model away from "everything in one Azure
Blob container" to "GitHub Releases for content, Azure Blob only for
log uploads". Cady ranked this as the cleanest of the four hosting
options in his review — $0 cost, free egress, sigstore attestation
available for free via actions/attest-build-provenance, and the
launcher binary itself already ships via GH Releases so the operator
workflow is consistent.
Content layout
--------------
Two release-tag families:
- content-current mutable rolling tag. Contains
manifest.json + manifest.json.sig.
`gh release upload --clobber` to
publish a new manifest.
- content-YYYY-MM-DD-NNN immutable per-publication tag.
Contains seed.zip and/or per-patch
zips. Never overwritten.
The manifest references seed/patch blobs by absolute URL pointing
into the immutable tags. That gives both properties at once:
- manifest is mutable so publishing a new content drop is a
single `gh release upload --clobber`.
- blobs are immutable so existing installs can resume partial
downloads safely (bytes at a given URL never change).
Code
----
- `manifest::blob_url` now passes absolute `http(s)://` URLs in the
manifest's `blob` field through unchanged. Relative blob paths
still resolve against the manifest URL's container — backwards-
compatible with the previous Azure-only convention.
- New tests: blob_url_passes_absolute_https_through,
blob_url_passes_absolute_http_through.
- `DEFAULT_MANIFEST_URL` in config.rs already pointed at the GH
Releases convention (set in the atomic-writes commit).
Release workflow
----------------
`launcher-release.yml` now reads two repo secrets:
- LAUNCHER_LOG_SAS_URL (already there) — log upload SAS
- LAUNCHER_MANIFEST_PUBKEY_HEX (new) — 64-hex-char Ed25519
pubkey, embedded into the
release binary via option_env!
in manifest.rs.
Without LAUNCHER_MANIFEST_PUBKEY_HEX set, the build falls back to a
publicly-known dev pubkey (intentional so PR / CI builds compile);
the release workflow is the only place the real production pubkey
is injected.
Docs
----
- `docs/client/launcher-storage-setup.md` → renamed and rewritten as
`docs/client/launcher-distribution-setup.md`. Old doc was Azure-
centric; new doc covers all three operator concerns in one place:
Part 1 — Content distribution via GitHub Releases (the new model)
Part 2 — Manifest signing keypair (Ed25519 generation + rotation)
Part 3 — Debug-log uploads via Azure Blob (unchanged from before)
Part 4 — Threat model: SAS in the client binary (unchanged)
- `docs/client/launcher-guide.md` Step 4 rewritten for the GH
Releases publish flow (PowerShell + gh CLI instead of az CLI).
- `docs/client/sgw-launcher.md` install pipeline rewritten to cover
the new signature-verify step + the atomic re-patch flow + the
process-wide file lock.
- Cross-references in `docs/readme.md` updated.
Tests: 67 → 69. blob_url absolute-URL passthrough.
Verified locally on Windows:
cargo fmt -p sgw-launcher -- --check OK
cargo clippy -p sgw-launcher --all-targets -- -D warnings clean
cargo test -p sgw-launcher 69/69 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Round 2 feedback addressed — 6 new commitsThis is a big swing. Per scope choices (everything except self-update/repair + manifest signing + GH Releases pivot), the work landed across 6 focused commits on top of the previous fixup:
Tests: 49 → 69 (+20). All gates green locally ( What's now fixedReal bug (Cady §1)
Cady's code-level (§5)
Deferred: 6e (rescan on worker) and 6j (file layout) — not v1 concerns per Cady. Cady's security (§3)
Deferred: 4b Manifest replay protection — needs the signing foundation that just landed; clean follow-up. Cady's feature gaps (§2)
Deferred (per "skip only self-update/repair"): self-update, repair/verify, news, server selection, EULA, crash detection, ETA display. CodeRabbit's 3 new nits
GH Releases pivot (
|
| # | Item | Source |
|---|---|---|
| 1 | Atomic install via staging dir + rename overlay | Cady §2 |
| 2 | E2E install_all test with wiremock (seed → patches → idempotent re-run) |
Cady #7a |
| 3 | Resume from partial .tmp-* test |
Cady #7d / MyPalClara G7 |
| 4 | Explicit zip-slip lock-in test | Cady #7e |
| 5 | Single-instance lock refusal test | Cady #7f |
| 6 | Manifest replay protection via manifest.version monotonic |
Cady #4b |
| 7 | Move install-dir rescan to worker (UI thread stutter on slow paths) | Cady #6e |
| 8 | Self-update via manifest.launcher field |
Cady §2 |
| 9 | Repair / verify per-file sha map | Cady §2 |
The bug fix in e7e54dc already has a regression test (patch_hostname_any_repatches_when_host_changes) — that's the one Cady asked for in #7c.
CI
launcher-build.yml should run on the push and exercise the full Windows pipeline against all 6 commits. With 69 tests (up from 40 in the original PR) and the manifest-signing path exercised by 5 new unit tests, the surface area is meaningfully better-covered.
Watching for: the test suite to pass on windows-latest, no clippy regressions, and the workflow itself to handle the new LAUNCHER_MANIFEST_PUBKEY_HEX secret being unset gracefully (it should — option_env! falls back to the dev pubkey).
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/launcher/src/app.rs (1)
401-405:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the Atera debug filename in the help text.
The UI says
AtreaGameDebug.bat, but the launcher looks forAteraGameDebug.bat. This points users at the wrong artifact when debug launch is unavailable.🤖 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 `@crates/launcher/src/app.rs` around lines 401 - 405, The help text shown when !opts.atera_available() contains a typo: it says "AtreaGameDebug.bat" but the launcher expects "AteraGameDebug.bat"; update the string passed to egui::RichText::new in the ui.label call (the block guarded by if !opts.atera_available()) to replace "AtreaGameDebug.bat" with "AteraGameDebug.bat" so the UI matches the actual filename the launcher checks for.crates/launcher/src/logs.rs (1)
57-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't silently drop unreadable session files.
filter_map(|r| r.ok())turns traversal failures into a “successful” partial upload. For diagnostics, that's misleading; return the firstWalkDirerror or surface an explicit warning/error event instead.As per coding guidelines
crates/**: “Focus on: … Error handling (Result, Option, ? operator)”.🤖 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 `@crates/launcher/src/logs.rs` around lines 57 - 61, The WalkDir traversal silently drops IO errors because it uses filter_map(|r| r.ok()); replace that with explicit handling so unreadable session files are not ignored: iterate over WalkDir::new(&sessions) and match each entry (Ok(e) => push e.path() into files; Err(err) => either return Err(err) from the surrounding function or emit a clear warning/log/event), using the function's Result return type and the ? operator to propagate the first WalkDir error when appropriate; refer to the loop that builds files, the sessions variable, and the files vector when making the change.
🤖 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 `@crates/launcher/src/app.rs`:
- Around line 189-192: The edited install path in install_path_text is only
copied into config.install_path on Save, causing stale paths for Install/Update,
writability checks, launch, and log upload; call
self.sync_install_path_from_text() at the start of any path-dependent action
(e.g., immediately before the existing call to
self.config.save(&self.config_path) in the Save button handler and likewise at
the top of the Install/Update handlers and any functions doing writability
gating, launch, or log upload) so that config.install_path is always up-to-date
before performing path-dependent logic.
In `@crates/launcher/src/install.rs`:
- Around line 162-166: Remove reviewer/PR identifiers from the source comments
while keeping the rationale: edit the comment above the safe_sha computation
(the comment that explains including a sha prefix in the tmp filename) to drop
"Cady `#6i`" and any "PR `#343`" references but preserve the explanation about
avoiding stale resumes by using a sha-based tmp path; likewise find and clean
the other comment block that contains reviewer/PR IDs (the block around the
later lines referencing the same rationale) and remove those provenance tokens
while retaining the technical reasoning (references: the safe_sha variable and
patch.sha256).
- Around line 130-131: The temp filename currently embeds unvalidated manifest
data (seed.sha256) into short_hash and builds tmp via
ctx.install_dir.join(format!(".tmp-seed-{short_hash}.zip")), which allows path
traversal if the manifest is malicious; validate or sanitize seed.sha256 before
using it in any path: ensure seed.sha256 matches a hex-only pattern (e.g. only
0-9a-fA-F) and take a bounded substring of the validated hex (or fallback to a
computed safe identifier like a hex of the first N bytes of a verified digest)
then use that safe short_hash when constructing tmp; apply the same
validation/sanitization to the other occurrence that builds temp filenames at
the 166–169 site so no untrusted characters reach ctx.install_dir.join or
format!.
In `@crates/launcher/src/launch.rs`:
- Around line 74-82: The comment above the canonicalize call references a
reviewer/provenance ("Cady `#4h`"); remove that provenance while preserving the
security rationale: update the comment that precedes let canon_install =
install_dir.canonicalize()? to explain canonicalizing both install_dir and the
target path and checking the target remains under the install dir (to catch
config/junction path escapes), but delete the "Cady `#4h`" phrase so the comment
contains only the rationale and not PR/reviewer metadata.
In `@crates/launcher/src/main.rs`:
- Around line 30-56: The lock-file failure paths around File::create(&lock_path)
and FileExt::try_lock_exclusive(&lock_file) currently only call eprintln! and
exit; change them to present a user-visible error before exiting by invoking a
small helper (e.g., show_error_dialog_and_log) that (1) shows a native modal
message box/dialog with the formatted error text (Windows MessageBox or
cross-platform crate) and (2) appends the same error text to a persistent log
(e.g., create/write "launcher-error.log" next to lock_path) so double-click
launches surface the problem even with windows_subsystem="windows"; call this
helper immediately before std::process::exit(2) and std::process::exit(3)
respectively.
In `@crates/launcher/src/manifest.rs`:
- Around line 20-34: Remove the explicit "PR `#343`" provenance from the module
doc comment in manifest.rs while preserving the explanatory rationale about
signing and why it closes the manifest-tampering hole; locate the top-of-file
module doc block (the comment that begins with "Signing model" and references
MANIFEST_SIGNING_PUBKEY) and delete or reword the "PR `#343`" token so the comment
no longer cites a specific PR number, leaving the rest of the description, the
reference to MANIFEST_SIGNING_PUBKEY, and the env-var guidance unchanged.
- Around line 183-195: The code currently builds sig_url by appending ".sig" to
the entire manifest URL (sig_url = format!("{url}.sig")), which places the
suffix after the query string and breaks signed/SAS URLs; change the logic in
manifest.rs where sig_url is built so the ".sig" is inserted before any query
string — either parse the url with the Url crate and append ".sig" to the path
segment or split the string at the first '?' and format as "{base}.sig{?query}"
— then use that corrected sig_url with http.get/send/..text() to fetch the
detached signature.
- Around line 55-66: The current MANIFEST_SIGNING_PUBKEY LazyLock silently falls
back to SigningKey::from_bytes(&[0x2a; 32]).verifying_key() when
option_env!("LAUNCHER_MANIFEST_PUBKEY_HEX") is absent; instead, change the
initializer so that if the env var is missing it panics (hard-fails) for
non-test/dev builds while still allowing test/dev runs (e.g. guard with
cfg!(test) || cfg!(debug_assertions) to permit the dev fallback). Update the
closure creating MANIFEST_SIGNING_PUBKEY to parse LAUNCHER_MANIFEST_PUBKEY_HEX
as now, but if option_env! returns None and the build is not a test/dev build,
call panic! with a clear message about the missing LAUNCHER_MANIFEST_PUBKEY_HEX;
only then allow the SigningKey::from_bytes(&[0x2a; 32]).verifying_key() path for
tests/devs to preserve CI/tests.
In `@crates/launcher/src/patch_rdata.rs`:
- Around line 145-150: Remove the reviewer/provenance token "Cady `#4c`" from the
comment block in patch_rdata.rs while preserving the technical explanation about
atomic writes and the .patching rename rationale; update the comment associated
with the write/rename behavior (the block that begins "Writes are **atomic**")
to omit any PR/issue/reviewer identifiers and keep only the rationale about
writing to a sibling .patching file and NTFS rename atomicity.
In `@crates/launcher/src/worker.rs`:
- Around line 122-127: spawn_install currently creates a fresh CancellationToken
and replaces self.current_install_cancel without cancelling any prior install,
allowing concurrent installs to race; change spawn_install (and its use of
self.current_install_cancel and CancellationToken) to first check for an
existing token and call cancel() on it (or otherwise reject the new install)
before creating and storing the new token so only one Install task runs at a
time and shared resources (temp zips, extraction, launcher-installed.json,
SGW.exe patching) are not raced.
- Around line 58-65: Remove the review-provenance tokens "Cady `#6f`" and "Cady
`#6g`" from the field doc comments above current_install_cancel and http, leaving
the explanatory rationale intact (e.g., that current_install_cancel is a
per-install CancellationToken to avoid shared kill switches and that http is a
shared reqwest::Client with connection pooling and https_only protection);
update the doc comment lines for the current_install_cancel and http fields to
omit any PR/issue/reviewer identifiers so they comply with the crate comment
guidelines.
In `@docs/client/launcher-guide.md`:
- Line 313: The Contents/TOC entry still references the old "Upload to Azure
Blob" anchor; update the TOC item so its visible text and anchor match the
renamed section header "Step 4 — Sign + publish to GitHub Releases" (i.e.,
change the TOC label and the in-page link/slug to the new GitHub Releases anchor
used by that header) so the in-page navigation points to the correct section.
---
Outside diff comments:
In `@crates/launcher/src/app.rs`:
- Around line 401-405: The help text shown when !opts.atera_available() contains
a typo: it says "AtreaGameDebug.bat" but the launcher expects
"AteraGameDebug.bat"; update the string passed to egui::RichText::new in the
ui.label call (the block guarded by if !opts.atera_available()) to replace
"AtreaGameDebug.bat" with "AteraGameDebug.bat" so the UI matches the actual
filename the launcher checks for.
In `@crates/launcher/src/logs.rs`:
- Around line 57-61: The WalkDir traversal silently drops IO errors because it
uses filter_map(|r| r.ok()); replace that with explicit handling so unreadable
session files are not ignored: iterate over WalkDir::new(&sessions) and match
each entry (Ok(e) => push e.path() into files; Err(err) => either return
Err(err) from the surrounding function or emit a clear warning/log/event), using
the function's Result return type and the ? operator to propagate the first
WalkDir error when appropriate; refer to the loop that builds files, the
sessions variable, and the files vector when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea6f5da0-62c3-4438-b294-ca33609dd892
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/launcher-release.ymlcrates/launcher/Cargo.tomlcrates/launcher/src/app.rscrates/launcher/src/config.rscrates/launcher/src/install.rscrates/launcher/src/launch.rscrates/launcher/src/logs.rscrates/launcher/src/main.rscrates/launcher/src/manifest.rscrates/launcher/src/patch_rdata.rscrates/launcher/src/state.rscrates/launcher/src/worker.rsdocs/client/launcher-distribution-setup.mddocs/client/launcher-guide.mddocs/client/sgw-launcher.mddocs/readme.md
Five real bugs + a stack of cleanup that came out of the CodeRabbit
review pass on the round-2 commits:
- worker.rs::spawn_install now cancels the previous install token before
storing the new one. The UI button is gated against this today, but a
rapid Cancel-then-Install flow or any non-UI dispatcher would have
raced two installs on the temp zips, the extract, the state file, and
the SGW.exe patch.
- manifest.rs::fetch_manifest now constructs the `.sig` URL via a new
`sig_url_for()` helper that inserts the suffix on the path component
before any query string. Appending ".sig" to the entire URL produced
`manifest.json?token=....sig` against the GH Releases redirect / SAS
URL, which fetched the wrong object every time.
- manifest.rs::MANIFEST_SIGNING_PUBKEY no longer falls back to the
publicly-known dev key in release builds. If LAUNCHER_MANIFEST_PUBKEY_HEX
is unset, empty, or malformed at compile time, release builds resolve
to `None` and `verify_manifest_signature` returns the new
`SigningKeyUnavailable` error. The previous behavior (silent dev-key
acceptance in release) would have re-introduced the exact attack the
signing scheme exists to close. Option<VerifyingKey> rather than
panic! because the LazyLock body runs inside a tokio worker, where
panics are caught and silently lost — that would have hung the UI on
"Fetching manifest…" forever.
- app.rs::show_config_panel syncs install_path on TextEdit focus loss.
Path-dependent actions (writability probe, refresh_install_state,
Install/Update, Launch, Upload Logs) all see the live value the moment
focus shifts to any other widget — including the buttons themselves,
which egui processes after focus resolution. Used `.lost_focus()`
rather than `.changed()` because the writability probe calls
`create_dir_all`, so per-keystroke sync would have created partial
directories ("C:\G", "C:\Ga", …) as the user typed.
- install.rs validates manifest sha256 fields as hex before splicing
them into temp filenames (`safe_sha_prefix`). Signature verification
authenticates the *bytes* of the manifest, not the well-formedness of
individual fields — a malicious manifest signed with a compromised
key could otherwise have escaped install_dir via crafted `sha256`
strings like "../foo".
- main.rs surfaces fatal startup errors (lock-file failure, lock
contention) via Windows MessageBoxW instead of just stderr. With
`windows_subsystem = "windows"` in release builds, stderr is detached
on a double-click, so the previous eprintln!+exit became a silent
crash. windows-sys pinned to 0.61 to dedup with the version already
in the workspace lockfile.
- logs.rs::collect_log_inputs returns Result instead of swallowing
walkdir errors via filter_map(.ok()). A permission glitch on the
sessions tree no longer silently produces a "successful" partial
upload — for diagnostic logs that was actively misleading.
- launcher-guide.md TOC entry for Step 4 now points at the GH Releases
anchor that replaced "Upload to Azure Blob" in the round-2 pivot.
- Drop `Cady #N` / `PR #343` review-provenance markers from source
comments per the crate coding guidelines (provenance lives in the PR
body, not in source).
Codecov:
- codecov.yml drops the `crates/launcher/**` ignore — the launcher
has its own active CI now — and adds a `launcher` component with
`target: auto` so the per-PR comment doesn't shout on the first
upload while a baseline lands.
- launcher-build.yml adds a `coverage` job that runs `cargo llvm-cov
nextest` on ubuntu (test logic is platform-agnostic; eframe Linux
deps installed via apt) and uploads to Codecov under the `launcher`
flag. `continue-on-error: true` so a coverage-tool flake doesn't
gate the launcher pipeline.
7 new tests bring the launcher suite from 69 to 76:
- 4 for `sig_url_for` (no query, with query, with fragment, both)
- 3 for `safe_sha_prefix` (lower/upper hex accept, path-traversal
reject, short-but-hex input accept)
CodeRabbit's "AtreaGameDebug.bat vs AteraGameDebug.bat" comment was a
false positive — the community-tool filename uses the transposed
spelling everywhere (detection, spawn, UI text are all consistent).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/launcher/src/logs.rs (1)
42-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't still swallow
Binaries/read errors.The new hard-fail path only covers
sessions/. Ifstd::fs::read_dir(&binaries)fails withPermissionDeniedor similar, this function still returns an empty list and the caller reports "No log files found." Surface non-NotFounderrors here too.Suggested fix
- if let Ok(entries) = std::fs::read_dir(&binaries) { - for e in entries.flatten() { - let p = e.path(); - if let Some(name) = p.file_name().and_then(|n| n.to_str()) { - if name.starts_with("sgwdebuglog") && p.is_file() { - files.push(p); - } - } - } - } + match std::fs::read_dir(&binaries) { + Ok(entries) => { + for e in entries.flatten() { + let p = e.path(); + if let Some(name) = p.file_name().and_then(|n| n.to_str()) { + if name.starts_with("sgwdebuglog") && p.is_file() { + files.push(p); + } + } + } + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(e.into()), + }🤖 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 `@crates/launcher/src/logs.rs` around lines 42 - 72, collect_log_inputs currently swallows errors from std::fs::read_dir(&binaries) and only fails hard for the sessions walk; change the read_dir handling in collect_log_inputs to propagate non-NotFound errors instead of ignoring them: replace the if let Ok(entries) = std::fs::read_dir(&binaries) { ... } with a match (or map_err/and_then) that on Ok(entries) iterates as before, on Err(e) returns Err(convert e into LogError) unless e.kind() == std::io::ErrorKind::NotFound (in which case continue), ensuring you use the same LogError conversion used elsewhere in this module so callers receive a hard failure for real IO errors.
♻️ Duplicate comments (2)
crates/launcher/src/worker.rs (1)
58-66:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCancellation here still allows overlapping installs.
prev.cancel()only requests shutdown. The previous task can keep downloading or extracting until its next cancellation checkpoint, while the replacement task starts immediately on the same temp zips, extracted files, state file, andSGW.exe. Please serialize on actual task completion (for example by retaining aJoinHandleand awaiting it, or by rejecting a new install while one is still running) before claiming these installs cannot run concurrently.Also applies to: 124-133
🤖 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 `@crates/launcher/src/worker.rs` around lines 58 - 66, The current_install_cancel field only calls prev.cancel() which requests shutdown but does not wait for the previous install to finish, allowing overlap; change the design to serialize installs by pairing the CancellationToken with the running task's JoinHandle (e.g., replace or extend current_install_cancel: Option<CancellationToken> with a struct or separate current_install_handle: Option<JoinHandle<()>> and current_install_cancel: Option<CancellationToken>), and after calling prev.cancel() await the JoinHandle to completion (or reject/return the new install attempt if a handle exists) before starting the replacement install; update every site that currently just calls prev.cancel() (including the earlier usage noted around lines 124-133) to either await the stored JoinHandle or refuse concurrent installs so temp zips, extraction, launcher-installed.json, and SGW.exe patching cannot race.crates/launcher/src/manifest.rs (1)
54-92:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis still lets a broken release ship and fail only at runtime.
option_env!is resolved at compile time. ReturningNonehere means a release artifact can still build and be published whenLAUNCHER_MANIFEST_PUBKEY_HEXis missing or malformed, and the launcher only breaks later when a user fetches the manifest. Please add a build/startup hard-fail outside thisLazyLockso release workflow mistakes are caught before distribution.🤖 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 `@crates/launcher/src/manifest.rs` around lines 54 - 92, The manifest public-key logic in MANIFEST_SIGNING_PUBKEY currently defers failure to runtime via LazyLock and option_env!, allowing a malformed/missing LAUNCHER_MANIFEST_PUBKEY_HEX to produce a broken release artifact; add a compile-time/build-time hard-fail for release builds by adding a build.rs that (when not test/debug_assertions) reads LAUNCHER_MANIFEST_PUBKEY_HEX, runs the same validation you use (hex_decode_32 + VerifyingKey::from_bytes semantics) and exits non‑zero or panics (failing the build) if the var is unset/empty/malformed — keep the existing LazyLock only for the dev/test fallback path so local CI/dev still works but releases cannot be built with an invalid key.
🤖 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 @.github/workflows/launcher-build.yml:
- Around line 108-149: The workflow uses several tag/branch-based action refs
that should be pinned to immutable commit SHAs: replace actions/checkout@v6,
dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2,
taiki-e/install-action@cargo-llvm-cov, taiki-e/install-action@nextest,
actions/upload-artifact@v7, and codecov/codecov-action@v6 with their
corresponding full commit SHA refs (e.g., actions/checkout@<sha>) in the
launcher coverage job; ensure each SHA is the exact commit you want to lock to,
verify the SHAs are valid on the respective repos, and keep the rest of the step
configuration unchanged.
---
Outside diff comments:
In `@crates/launcher/src/logs.rs`:
- Around line 42-72: collect_log_inputs currently swallows errors from
std::fs::read_dir(&binaries) and only fails hard for the sessions walk; change
the read_dir handling in collect_log_inputs to propagate non-NotFound errors
instead of ignoring them: replace the if let Ok(entries) =
std::fs::read_dir(&binaries) { ... } with a match (or map_err/and_then) that on
Ok(entries) iterates as before, on Err(e) returns Err(convert e into LogError)
unless e.kind() == std::io::ErrorKind::NotFound (in which case continue),
ensuring you use the same LogError conversion used elsewhere in this module so
callers receive a hard failure for real IO errors.
---
Duplicate comments:
In `@crates/launcher/src/manifest.rs`:
- Around line 54-92: The manifest public-key logic in MANIFEST_SIGNING_PUBKEY
currently defers failure to runtime via LazyLock and option_env!, allowing a
malformed/missing LAUNCHER_MANIFEST_PUBKEY_HEX to produce a broken release
artifact; add a compile-time/build-time hard-fail for release builds by adding a
build.rs that (when not test/debug_assertions) reads
LAUNCHER_MANIFEST_PUBKEY_HEX, runs the same validation you use (hex_decode_32 +
VerifyingKey::from_bytes semantics) and exits non‑zero or panics (failing the
build) if the var is unset/empty/malformed — keep the existing LazyLock only for
the dev/test fallback path so local CI/dev still works but releases cannot be
built with an invalid key.
In `@crates/launcher/src/worker.rs`:
- Around line 58-66: The current_install_cancel field only calls prev.cancel()
which requests shutdown but does not wait for the previous install to finish,
allowing overlap; change the design to serialize installs by pairing the
CancellationToken with the running task's JoinHandle (e.g., replace or extend
current_install_cancel: Option<CancellationToken> with a struct or separate
current_install_handle: Option<JoinHandle<()>> and current_install_cancel:
Option<CancellationToken>), and after calling prev.cancel() await the JoinHandle
to completion (or reject/return the new install attempt if a handle exists)
before starting the replacement install; update every site that currently just
calls prev.cancel() (including the earlier usage noted around lines 124-133) to
either await the stored JoinHandle or refuse concurrent installs so temp zips,
extraction, launcher-installed.json, and SGW.exe patching cannot race.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d50273b-b82d-43f1-bd05-06f52735f2bd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/launcher-build.ymlcodecov.ymlcrates/launcher/Cargo.tomlcrates/launcher/src/app.rscrates/launcher/src/install.rscrates/launcher/src/launch.rscrates/launcher/src/logs.rscrates/launcher/src/main.rscrates/launcher/src/manifest.rscrates/launcher/src/patch_rdata.rscrates/launcher/src/worker.rsdocs/client/launcher-guide.md
Comment on lines
+108
to
+149
| - uses: actions/checkout@v6 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| components: llvm-tools-preview | ||
| - name: install eframe / egui Linux deps | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| libxkbcommon-dev libwayland-dev libx11-dev libxcb1-dev \ | ||
| libxrandr-dev libxi-dev libgl1-mesa-dev | ||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| shared-key: launcher-coverage | ||
| - name: install cargo-llvm-cov | ||
| uses: taiki-e/install-action@cargo-llvm-cov | ||
| - name: install nextest | ||
| uses: taiki-e/install-action@nextest | ||
| - name: clean stale counter data | ||
| run: cargo llvm-cov clean --workspace | ||
| - name: cargo llvm-cov nextest -p sgw-launcher | ||
| run: cargo llvm-cov --no-report nextest --profile=ci -p sgw-launcher | ||
| - name: render cobertura report | ||
| run: | | ||
| mkdir -p target/coverage | ||
| cargo llvm-cov report --cobertura --output-path target/coverage/cobertura.xml | ||
| cargo llvm-cov report --summary-only | tee target/coverage/summary.txt | ||
| - name: print summary to job log | ||
| run: | | ||
| echo "## Launcher coverage summary" >> $GITHUB_STEP_SUMMARY | ||
| echo '```' >> $GITHUB_STEP_SUMMARY | ||
| cat target/coverage/summary.txt >> $GITHUB_STEP_SUMMARY | ||
| echo '```' >> $GITHUB_STEP_SUMMARY | ||
| - name: upload coverage artifact | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: launcher-coverage | ||
| path: target/coverage/ | ||
| retention-days: 14 | ||
| - name: upload to Codecov | ||
| uses: codecov/codecov-action@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
check_tag() {
local repo="$1"
local tag="$2"
local url="https://api.github.com/repos/${repo}/git/ref/tags/${tag}"
local code
code=$(curl -sS -o /tmp/tag.json -w '%{http_code}' "$url")
printf '%-30s %-10s -> HTTP %s\n' "$repo" "$tag" "$code"
if [ "$code" = "200" ]; then
jq -r '.ref' /tmp/tag.json
else
cat /tmp/tag.json
fi
echo
}
check_tag actions/checkout v6
check_tag actions/upload-artifact v7
check_tag codecov/codecov-action v6Repository: SandboxServers/Cimmeria
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
check_ref() {
local repo="$1"
local ref="$2"
local tag_url="https://api.github.com/repos/${repo}/git/ref/tags/${ref}"
local head_url="https://api.github.com/repos/${repo}/git/ref/heads/${ref}"
local code_ref
code_ref=$(curl -sS -o /tmp/tag.json -w '%{http_code}' "$tag_url" || true)
if [ "$code_ref" = "200" ]; then
echo "$repo @ $ref -> tag -> HTTP 200 ($(jq -r '.ref' /tmp/tag.json))"
return 0
fi
code_ref=$(curl -sS -o /tmp/head.json -w '%{http_code}' "$head_url" || true)
if [ "$code_ref" = "200" ]; then
echo "$repo @ $ref -> branch -> HTTP 200 ($(jq -r '.ref' /tmp/head.json))"
return 0
fi
echo "$repo @ $ref -> not found as tag or branch (tag HTTP $(
curl -sS -o /tmp/tag2.json -w '%{http_code}' "$tag_url" || true
), branch HTTP $(
curl -sS -o /tmp/head2.json -w '%{http_code}' "$head_url" || true
))"
return 1
}
check_ref dtolnay/rust-toolchain stable
check_ref Swatinem/rust-cache v2
check_ref taiki-e/install-action cargo-llvm-cov
check_ref taiki-e/install-action nextestRepository: SandboxServers/Cimmeria
Length of output: 358
Pin GitHub Actions uses: refs in launcher coverage job to full commit SHAs
- Even though the current refs resolve (
actions/checkout@v6,dtolnay/rust-toolchain@stable,Swatinem/rust-cache@v2,taiki-e/install-action@cargo-llvm-cov,taiki-e/install-action@nextest,actions/upload-artifact@v7,codecov/codecov-action@v6), tag/branch-baseduses:are still floating and can change behavior; pin each to an exact commit SHA.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 108-108: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 111-111: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 120-120: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 124-124: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 126-126: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 143-143: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 149-149: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 111-111: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🤖 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 @.github/workflows/launcher-build.yml around lines 108 - 149, The workflow
uses several tag/branch-based action refs that should be pinned to immutable
commit SHAs: replace actions/checkout@v6, dtolnay/rust-toolchain@stable,
Swatinem/rust-cache@v2, taiki-e/install-action@cargo-llvm-cov,
taiki-e/install-action@nextest, actions/upload-artifact@v7, and
codecov/codecov-action@v6 with their corresponding full commit SHA refs (e.g.,
actions/checkout@<sha>) in the launcher coverage job; ensure each SHA is the
exact commit you want to lock to, verify the SHAs are valid on the respective
repos, and keep the rest of the step configuration unchanged.
Two fixes bundled: 1. Merge main into the branch. Cargo.lock had the only conflict; taken from main and re-resolved via `cargo check --workspace` to pick up any deltas this branch's deps need on top of main's recently-updated lockfile (reqwest 0.13 + cipher 0.5 family). 2. CI fix: `cargo llvm-cov (launcher)` was failing with `clang: error: invalid linker name in argument '-fuse-ld=mold'`. `.cargo/config.toml` pins `linker = "clang"` and `rustflags = ["-C", "link-arg=-fuse-ld=mold"]` for the Linux GNU target. Every other Linux job in the repo (`test.yml` jobs, `spec-lint.yml`) does `apt-get install -y clang mold` to satisfy that flag, but the launcher coverage job — added in this PR — only installed the egui/eframe display-server deps. Stock ubuntu-latest clang has no built-in mold support, so the `-fuse-ld=mold` flag bombed on the first build-script crate (quote / proc-macro2 / libc). Folded clang + mold into the existing `install eframe / egui Linux deps` step rather than adding a separate one, with a comment explaining the dependency on `.cargo/config.toml`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`default_uses_localappdata_when_set` failed on the ubuntu coverage
runner (`cargo llvm-cov (launcher)`) because the assertion compares
against `PathBuf::from("Z:\LocalApp\Stargate Worlds")`. On Windows
the embedded `\` is a path separator and the literal parses as three
components; on Linux it's a single component containing literal
backslashes, so `.join("Stargate Worlds")` produces a `/`-separated
mixed string that doesn't byte-equal the expected value.
The Windows `cargo nextest (Windows)` job passed because that runner
treats `\` as a separator. Only the Linux coverage job catches the
mismatch.
Gate the test with `#[cfg(target_os = "windows")]` — it's specifically
exercising the Windows `LOCALAPPDATA` convention inside
`default_install_path`. The Linux/`HOME` fallback branches in that
function are exercised implicitly by every other test that constructs
a `LauncherConfig::default()`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crates/launcher/(~6,989 lines of JS/HTML/Rust + Tauri configs + auto-generated schemas).workflow_dispatchrelease, and/release-launcherChatOps on merged PRs.What the new launcher does
State files:
launcher-config.jsonnext to.exe,launcher-installed.jsoninside the game dir (survives launcher reinstall),uploaded.jsonnext to.exe.Crate layout (10 flat src files)
main.rs+app.rsmanifest.rsinstall.rspatch_rdata.rs.rdatahostname patch (salvaged from old launcher)launch.rslogs.rsstate.rsworker.rsWorkflows
.github/workflows/launcher-build.ymlcrates/launcher/**. Runs fmt (ubuntu) + clippy/build/test (windows-latest)..github/workflows/launcher-release.ymlworkflow_dispatch. Builds release exe withLAUNCHER_LOG_SAS_URLinjected from secrets, creates GitHub Release taggedlauncher-<date>-<sha7>..github/workflows/launcher-release-on-comment.ymlissue_comment/release-launcheron merged PRs (mirror ofrelease-on-comment.yml).The launcher stays in
WORKSPACE_EXCLUDESintest.ymlso eframe Linux deps don't slow the main CI pipeline —launcher-build.ymlowns its full cycle onwindows-latest.Docs
docs/client/sgw-launcher.md— rewritten design docdocs/client/launcher-guide.md— new user + operator guide (player workflow + how to publish patches)docs/client/launcher-storage-setup.md— new Azure container / SAS minting / lifecycle runbookdocs/plans/2026-03-06-sgw-launcher-{design,plan}.md— marked superseded with link forwardcrates/README.md,docs/readme.md,CLAUDE.md— index entries updatedVerification (local, Windows)
Out-of-band setup required before release is functional
Beyond merging this PR:
sgwcontainer perlauncher-storage-setup.md.c(create) permission scoped to the container.LAUNCHER_LOG_SAS_URL.launcher-guide.mdPart 2.Without the secret, the release workflow still builds — log upload just stays permanently disabled in that binary.
Test plan
launcherworkflow (path-filtered) runs and is greenciworkflow stays green (launcher excluded viaWORKSPACE_EXCLUDES).rdatapatched → Launch SGW.exe works🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation