defold entrypoint#34
Conversation
|
Claude finished @cloud9c's task in 8m 20s —— View job Code ReviewReviewed at extra-high effort (5 finder angles × 8 candidates → verify → sweep). Findings posted as inline comments on the PR.
Summary of findings (10 inline)High severity
Medium severity Lower severity |
| modified_b | ||
| .cmp(&modified_a) | ||
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | ||
| .then_with(|| relative_a.cmp(&relative_b)) |
There was a problem hiding this comment.
Sort key precedence likely inverted. Modification time is the primary key, so the wasm-web/ / js-web/ architecture preference (the whole point of architecture_score) only acts as a tiebreaker when two HTML files share an identical mtime.
For Defold, both wasm-web/index.html and js-web/index.html are written by the bundler at roughly the same time, but any subsequent activity (re-running just one target, cp -p, an IDE touching one of them, a file-stager re-copying) gives one a newer mtime than the other and that one wins — even if it's js-web/ and the score says wasm-web/ should be preferred.
If the intent is "prefer wasm-web, then js-web, then most-recently-modified," the comparator order should be flipped:
| modified_b | |
| .cmp(&modified_a) | |
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | |
| .then_with(|| relative_a.cmp(&relative_b)) | |
| architecture_score(&relative_a) | |
| .cmp(&architecture_score(&relative_b)) | |
| .then_with(|| modified_b.cmp(&modified_a)) | |
| .then_with(|| relative_a.cmp(&relative_b)) | |
| }); |
| let modified_a = a.metadata().and_then(|m| m.modified()).ok(); | ||
| let modified_b = b.metadata().and_then(|m| m.modified()).ok(); |
There was a problem hiding this comment.
Option<SystemTime> orders None < Some(_), so any file whose metadata().modified() fails (broken symlink, FS that doesn't expose mtime, permissions error during stat) becomes None and — because of the modified_b.cmp(&modified_a) direction on line 61 — sorts before files with valid mtimes. The file with broken metadata wins selection.
Consider mapping None to SystemTime::UNIX_EPOCH (or Reverse) so that "no metadata" sorts to the bottom rather than the top.
| let architecture_score = |relative: &str| { | ||
| if relative.starts_with("wasm-web/") { | ||
| 0 | ||
| } else if relative.starts_with("js-web/") { | ||
| 1 | ||
| } else { | ||
| 2 | ||
| } | ||
| }; |
There was a problem hiding this comment.
architecture_score only matches when wasm-web/ / js-web/ is the FIRST path segment after upload_dir.
Defold's HTML5 bundle by default writes to build/default/<GameTitle>/{wasm-web,js-web}/.... If a user sets upload_dir = "build/default" (rather than build/default/<GameTitle>), the relative paths become e.g. MyGame/wasm-web/index.html and MyGame/js-web/index.html — both score 2, the architecture preference never kicks in, and the tiebreakers (mtime + alpha) decide arbitrarily.
Consider matching anywhere in the path, e.g. relative.contains("/wasm-web/") || relative.starts_with("wasm-web/").
| EngineType::Defold => { | ||
| let version: String = cliclack::input("Defold version") | ||
| .placeholder("1.0") | ||
| .default_input("1.0") | ||
| .interact()?; | ||
| Some(version) | ||
| } |
There was a problem hiding this comment.
Default Defold version "1.0" is not a real Defold release. Defold ships versions like 1.10.x / 1.9.x — Godot's default is 4.4, Unity's is 2022.3, both plausible releases on their actual streams. A user who hits Enter at this prompt ends up with [defold] version = "1.0" in wavedash.toml, which the server can't resolve to any runtime.
Also note generate_toml on line 254 has the same "1.0" fallback; both should be updated to a recent Defold release (e.g. 1.10.4 or whatever the current minimum-supported is).
| EngineType::Godot => "build", | ||
| EngineType::Unity => "build", | ||
| EngineType::Defold => "dist", | ||
| EngineType::Custom => "dist", |
There was a problem hiding this comment.
"dist" is not a Defold convention. Defold's bob.jar HTML5 bundler writes to build/default/<GameTitle>/ by default — most Defold users won't have a dist/ directory at all. Accepting the default in wavedash init produces a wavedash.toml whose upload_dir doesn't exist, and the next wavedash dev / build push errors immediately.
Consider defaulting to build/default (or prompting to enter the bundle path).
| Some( | ||
| fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path)) | ||
| .await?, | ||
| ) |
There was a problem hiding this comment.
Engine label hardcoded as "DEFOLD" instead of routed through EngineKind::Defold.as_label(). The parallel call in src/dev/mod.rs:133 correctly uses engine_kind.unwrap().as_label() — these two call sites can now drift silently.
| Some( | |
| fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path)) | |
| .await?, | |
| ) | |
| Some( | |
| fetch_entrypoint_params(EngineKind::Defold.as_label(), ver, html_path, Some(&html_relative_path)) | |
| .await?, | |
| ) |
| let html_relative_path = html_path | ||
| .strip_prefix(&upload_dir) | ||
| .unwrap_or(html_path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); |
There was a problem hiding this comment.
upload_dir is not canonicalized here, unlike dev/mod.rs:101. Both call sites compute html_relative_path the same way, but only handle_dev resolves symlinks/.//.. first. The unwrap_or(html_path) fallback means a strip-prefix failure silently sends the absolute filesystem path to the server as htmlPath — leaking the local layout and almost certainly producing a 4xx.
Easiest fix: canonicalize upload_dir here too (mirror the dev/mod.rs block), or replace unwrap_or with an explicit error so a mismatch becomes a clear bug rather than bad data.
| fn detect_defold(dir: &Path) -> Option<DetectedEngine> { | ||
| if dir.join("game.project").is_file() { | ||
| return Some(DetectedEngine { | ||
| engine_type: EngineType::Defold, | ||
| version_hint: None, | ||
| }); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
detect_defold has no version-extraction logic and no fallback heuristic. Compare with:
detect_godot(lines 64–108): parsesconfig/featuresandconfig_versionto populateversion_hint.detect_unity(lines 112–133): parsesProjectSettings/ProjectVersion.txtfor the exact editor version AND falls back to detecting anAssets/directory.
For Defold, version_hint is always None, so the prompt placeholder (line 325) is the hard-coded "1.0" — a value users may accept without thinking. Defold's game.project does carry a [bootstrap] section and tools like the .defignore / bob.jar runner could imply a version; even just defaulting to the latest known release would be safer than "1.0".
| let response = client | ||
| .post(&endpoint) | ||
| .json(&serde_json::json!({ | ||
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| "htmlPath": html_relative_path, | ||
| })) |
There was a problem hiding this comment.
New htmlPath field is always serialized, including for existing Godot/Unity callers that previously sent only engine/engineVersion/htmlContent (dev/mod.rs:117 now passes Some(&html_relative_path) for Godot/Unity too).
If the server validates request shape strictly, or if it uses htmlPath for resolution and didn't before, this is a silent behavior change for established engines. Worth confirming the backend coordinated change covers Godot/Unity, not just Defold.
| return Some(default_index); | ||
| } | ||
|
|
||
| let mut html_files: Vec<PathBuf> = Vec::new(); |
There was a problem hiding this comment.
The unchanged early-return on lines 17–21 short-circuits the new architecture-aware selection. If upload_dir/index.html exists for any reason — a stale leftover from a previous engine, a redirect a build tool placed at the root, a user-written launcher — it wins immediately, and wasm-web/index.html is never considered.
For a Defold project migrating onto wavedash, this is a real footgun: any pre-existing root index.html silently pre-empts the proper Defold entrypoint, and fetch_entrypoint_params parses the wrong HTML. Consider gating the early-return on engine kind, or only using it when no nested wasm-web/ / js-web/ directory is present.
|
Claude finished @cloud9c's task in 2m 59s —— View job Code Review — Follow-up on commit
|
| # | Severity | Location | Status |
|---|---|---|---|
| 1 | High | src/dev/entrypoint.rs:64-67 — sort key precedence (mtime dominates architecture) |
Outstanding — comparator order unchanged |
| 2 | High | src/dev/entrypoint.rs:42-43 — Option<SystemTime> orders None < Some |
Outstanding — .ok() still produces Option, no UNIX_EPOCH fallback |
| 3 | High | src/dev/entrypoint.rs:54-62 — architecture_score requires wasm-web/ as first segment after upload_dir |
Outstanding — still uses starts_with("wasm-web/") |
| 4 | Medium | src/init.rs:254 & :325-326 — default Defold version "1.0" |
Fixed in 10f7bb0 (now "1.12.4") |
| 5 | Medium | src/init.rs:51 — default_upload_dir "dist" is not a Defold convention |
Outstanding |
| 6 | Medium | src/dev/entrypoint.rs:21-24 — root index.html early-return short-circuits architecture-aware selection |
Acknowledged as intentional (new comment at lines 17-19 documents it). Risk note: any pre-existing root index.html in a Defold project (stale leftover, build-tool artifact) will still silently pre-empt the proper wasm-web/index.html. Worth gating on engine kind if Defold projects ever ship with a root index. |
| 7 | Medium | src/builds.rs:175-179 — upload_dir not canonicalized (unlike dev/mod.rs:101) |
Outstanding — strip_prefix still has silent unwrap_or fallback |
| 8 | Medium | src/init.rs:136-145 — detect_defold has no version-extraction / fallback heuristic |
Outstanding — version_hint is still always None |
| 9 | Low | src/builds.rs:181 — hardcoded "DEFOLD" literal instead of EngineKind::Defold.as_label() |
Outstanding — two sources of truth still |
| 10 | Low | src/dev/entrypoint.rs:91 — htmlPath now sent for Godot/Unity callers too |
Outstanding — dev/mod.rs:117 still passes Some(&html_relative_path) for Godot/Unity in addition to Defold |
New observations from this pass
build pushvswavedash devasymmetry on entrypoint params (src/builds.rs:167-189vssrc/dev/mod.rs:116-141).handle_devfetchesentrypointParamsfrom/cli/entrypoint-paramsforGodot | Unity | Defold, buthandle_build_pushonly does so forDefold— Godot/Unity builds fall through the_ => Nonearm. This asymmetry pre-dated this PR (it was equivalent to the oldexecutable_entrypoint_params()call returningNonefor Godot/Unity), so it's not a regression — but worth confirming this is intentional (server computes params at build time for Godot/Unity, only Defold needs them sent up front), not an oversight when wiring Defold through both flows.
Suggested fix for findings #1, #2, #3 in one shot
let architecture_score = |relative: &str| {
if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") {
0
} else if relative.contains("/js-web/") || relative.starts_with("js-web/") {
1
} else {
2
}
};
html_files.sort_by(|a, b| {
let relative_a = a.strip_prefix(upload_dir).unwrap_or(a).to_string_lossy().replace('\\', "/");
let relative_b = b.strip_prefix(upload_dir).unwrap_or(b).to_string_lossy().replace('\\', "/");
let modified_a = a.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
let modified_b = b.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
architecture_score(&relative_a)
.cmp(&architecture_score(&relative_b))
.then_with(|| modified_b.cmp(&modified_a))
.then_with(|| relative_a.cmp(&relative_b))
});Fix entrypoint sort + Option ordering + nested path →
Fix builds.rs canonicalize + DEFOLD label →
- Gather diff and PR context
- Compare against prior review findings
- Identify what was fixed vs still outstanding
- Look for new issues introduced by the refactor
- Post status summary
• Branch: defold-first-class
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 10f7bb0. Configure here.
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
Wildcard match suppresses compiler exhaustiveness check for future engines
Low Severity
The entrypoint_params match in builds.rs uses _ => None as a catch-all, while the equivalent match in dev/mod.rs uses explicit None => None making it exhaustive over all EngineKind variants. If a future engine variant is added to EngineKind, the compiler will force an update in dev/mod.rs but silently accept the wildcard in builds.rs, risking a missing entrypoint-params computation in the build-push flow — exactly the kind of per-engine handling this PR itself demonstrates with Defold.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 10f7bb0. Configure here.
|
Claude finished @cloud9c's task in 6m 41s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort. Posted 7 inline findings on the latest commit, ranked most-severe first.
Findings posted as inline commentsHigh severity — entrypoint selection is broken in three independent ways that compound
Medium severity
Low severity
Findings from prior passes still applicable but not re-postedAvoiding inline duplicates — these remain valid in the current code state:
Suggested combined fix for findings 1–3let architecture_score = |relative: &str| {
if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") { 0 }
else if relative.contains("/js-web/") || relative.starts_with("js-web/") { 1 }
else { 2 }
};
html_files.sort_by_cached_key(|p| {
let relative = p.strip_prefix(upload_dir).unwrap_or(p).to_string_lossy().replace('\\', "/");
let architecture = architecture_score(&relative);
let modified = p.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
(architecture, std::cmp::Reverse(modified), relative)
});Fix entrypoint sort + Option ordering + nested path + cached key → Fix builds.rs canonicalize + DEFOLD label + exhaustive match → |
| modified_b | ||
| .cmp(&modified_a) | ||
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | ||
| .then_with(|| relative_a.cmp(&relative_b)) |
There was a problem hiding this comment.
Sort key precedence is inverted — architecture preference is effectively dead code.
modified_b.cmp(&modified_a) is the primary key; architecture_score only breaks ties when two HTML files share an identical SystemTime. In real bundle output, files written by the same bundler run differ by at least microseconds (and any subsequent cp -p, a partial rebuild of just one target, or an IDE touching one file changes mtime ordering deterministically). So js-web/index.html wins over wasm-web/index.html whenever it happens to be the newer of the two — even though the whole point of architecture_score is to prefer wasm-web.
Flip the comparator so architecture is primary and mtime is the tiebreaker:
| modified_b | |
| .cmp(&modified_a) | |
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | |
| .then_with(|| relative_a.cmp(&relative_b)) | |
| architecture_score(&relative_a) | |
| .cmp(&architecture_score(&relative_b)) | |
| .then_with(|| modified_b.cmp(&modified_a)) | |
| .then_with(|| relative_a.cmp(&relative_b)) | |
| }); |
(Combine with the Option<SystemTime> fix below — see separate comment on line 42–43.)
| let modified_a = a.metadata().and_then(|m| m.modified()).ok(); | ||
| let modified_b = b.metadata().and_then(|m| m.modified()).ok(); |
There was a problem hiding this comment.
Option<SystemTime> orders None < Some(_), so a file whose metadata().modified() fails becomes "newest" under the current comparator.
.ok() produces Option<SystemTime>. If metadata() errors (broken symlink, FS without mtime support, transient EACCES during walk), modified_a is None. Option's Ord impl ranks None before Some(_), so modified_b.cmp(&modified_a) (descending) puts the None file at the top of the sort — it wins selection over every file with a valid mtime.
Map the failure to a far-past time so missing-mtime files sort to the bottom:
let modified_a = a.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
let modified_b = b.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);This also removes the Option wrapping so the Ord derivation behaves intuitively.
| let architecture_score = |relative: &str| { | ||
| if relative.starts_with("wasm-web/") { | ||
| 0 | ||
| } else if relative.starts_with("js-web/") { | ||
| 1 | ||
| } else { | ||
| 2 | ||
| } | ||
| }; |
There was a problem hiding this comment.
architecture_score only triggers when wasm-web/ / js-web/ is the FIRST path segment relative to upload_dir — Defold's natural bundle layout puts them under a game-name folder.
bob.jar HTML5 bundling writes to <output>/<GameTitle>/wasm-web/... and <output>/<GameTitle>/js-web/.... A user who sets upload_dir = "build/default" (rather than build/default/<GameTitle>) sees relative paths like MyGame/wasm-web/index.html — both architectures score 2, the preference never fires, and selection falls through to mtime + alpha (i.e. whichever bundler happened to write last, or alphabetic on game name).
Match anywhere in the path:
| let architecture_score = |relative: &str| { | |
| if relative.starts_with("wasm-web/") { | |
| 0 | |
| } else if relative.starts_with("js-web/") { | |
| 1 | |
| } else { | |
| 2 | |
| } | |
| }; | |
| let architecture_score = |relative: &str| { | |
| if relative.contains("/wasm-web/") || relative.starts_with("wasm-web/") { | |
| 0 | |
| } else if relative.contains("/js-web/") || relative.starts_with("js-web/") { | |
| 1 | |
| } else { | |
| 2 | |
| } | |
| }; |
| html_files.sort_by(|a, b| { | ||
| let modified_a = a.metadata().and_then(|m| m.modified()).ok(); | ||
| let modified_b = b.metadata().and_then(|m| m.modified()).ok(); | ||
| let relative_a = a | ||
| .strip_prefix(upload_dir) | ||
| .unwrap_or(a) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| let relative_b = b | ||
| .strip_prefix(upload_dir) | ||
| .unwrap_or(b) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| let architecture_score = |relative: &str| { | ||
| if relative.starts_with("wasm-web/") { | ||
| 0 | ||
| } else if relative.starts_with("js-web/") { | ||
| 1 | ||
| } else { | ||
| 2 | ||
| } | ||
| }; | ||
|
|
||
| modified_b | ||
| .cmp(&modified_a) | ||
| .then_with(|| architecture_score(&relative_a).cmp(&architecture_score(&relative_b))) | ||
| .then_with(|| relative_a.cmp(&relative_b)) | ||
| }); |
There was a problem hiding this comment.
Sort comparator does file IO on every comparison — O(N log N) stat() syscalls per sort, not O(N).
Each invocation of the closure calls a.metadata() and b.metadata(). Rust's sort_by calls the comparator O(N log N) times, so for N HTML files we issue ~2·N·log₂(N) stat syscalls (plus another lazy traversal building the relative strings each call). For a typical Defold dist this is small, but for any upload_dir that incidentally contains many HTML files (engine demos, generated docs, asset bundles with HTML readmes), this is noticeable I/O — and it duplicates work that has a stable per-file answer.
Pre-compute keys once with sort_by_cached_key:
html_files.sort_by_cached_key(|p| {
let relative = p.strip_prefix(upload_dir).unwrap_or(p).to_string_lossy().replace('\\', "/");
let architecture = architecture_score(&relative);
let modified = p.metadata().and_then(|m| m.modified()).unwrap_or(std::time::SystemTime::UNIX_EPOCH);
// Reverse mtime via Reverse() so newest sorts first
(architecture, std::cmp::Reverse(modified), relative)
});This also makes the comparator total/consistent if a file is unlinked mid-sort (the key is captured once at the start).
| let html_relative_path = html_path | ||
| .strip_prefix(&upload_dir) | ||
| .unwrap_or(html_path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); |
There was a problem hiding this comment.
upload_dir is not canonicalized here (unlike dev/mod.rs:101), and the .unwrap_or(html_path) fallback silently leaks an absolute filesystem path on strip_prefix failure.
handle_dev resolves upload_dir via .canonicalize()? before walking it (src/dev/mod.rs:101). handle_build_push skips that step, so this call site walks a potentially-relative-or-symlinked path. If strip_prefix(&upload_dir) ever fails (mismatch in normalization between the path returned by WalkDir and upload_dir), unwrap_or(html_path) falls back to the full path — that absolute filesystem path gets sent to the server as htmlPath, leaking the local layout and almost certainly producing a 4xx.
Either canonicalize upload_dir first (mirror dev/mod.rs:101), or replace the fallback with an explicit error so a path mismatch surfaces as a clear bug:
let upload_dir = upload_dir.canonicalize().with_context(|| {
format!("Failed to canonicalize upload_dir: {}", upload_dir.display())
})?;
// ...
let html_relative_path = html_path
.strip_prefix(&upload_dir)
.with_context(|| format!("HTML entrypoint {} is not under upload_dir {}", html_path.display(), upload_dir.display()))?
.to_string_lossy()
.replace('\\', "/");| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| Some( | ||
| fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path)) |
There was a problem hiding this comment.
Engine label hardcoded as "DEFOLD" instead of routed through EngineKind::Defold.as_label().
The parallel call in src/dev/mod.rs:130 correctly uses engine_kind.unwrap().as_label(). Two sources of truth for the same value can drift silently — e.g. if the wire format ever needs to change ("DEFOLD" → "defold" for normalization), dev and build push will diverge until both are updated.
| fetch_entrypoint_params("DEFOLD", ver, html_path, Some(&html_relative_path)) | |
| Some( | |
| fetch_entrypoint_params(EngineKind::Defold.as_label(), ver, html_path, Some(&html_relative_path)) | |
| .await?, | |
| ) |
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
Wildcard _ => None suppresses the compiler's exhaustiveness check for future engine variants.
dev/mod.rs:137 uses an explicit None => None, making the match exhaustive over every EngineKind variant. Here, adding a new engine to EngineKind will compile cleanly in builds.rs — silently routing the new engine through the _ => None arm — even when dev (correctly) forces the author to decide. That asymmetry is exactly the per-engine handling this PR demonstrates by wiring up Defold; the wildcard makes the next engine more likely to slip through build push.
| _ => None, | |
| Some(EngineKind::Godot | EngineKind::Unity) => None, | |
| None => None, | |
| }; |
(Or list each variant explicitly. Either way, drop the wildcard.)


Note
Medium Risk
Changes how builds register entrypoints (API payload + multi-HTML selection) for Godot/Unity/Defold dev and upload flows; mis-picked HTML could break playtests until dist layout matches expectations.
Overview
Adds Defold as a first-class engine across
wavedash init, config parsing, local dev, and build upload.Init & config: Detects
game.project, defaults upload dir todist, prompts for Defold version, and writes a[defold]section.EngineKindand validation now include Defold alongside Godot/Unity.Entrypoint resolution:
locate_html_entrypointno longer returns the first nested.htmlfile; it ranks candidates by newest mtime, preferswasm-web/thenjs-web/, then stable path order—while still using rootindex.htmlwhen present.fetch_entrypoint_paramssendshtmlPath(relative to upload dir) to the CLI API; dev and build push both use this for Godot, Unity, and Defold.Build push: Defold builds resolve HTML + version, fetch server-derived
entrypointParams, and pass them into temp R2 creds (same pattern as executable engines for JSDOS/Ruffle/Ren'Py).Reviewed by Cursor Bugbot for commit b6db7b9. Bugbot is set up for automated code reviews on this repo. Configure here.