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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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.
| @@ -53,6 +85,7 @@ pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_pa | |||
| "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.
There was a problem hiding this comment.
@cloud9c this one's legit let's not change the shape of Unity or Godot uploads
Keep the new Defold logic isolated to the Defold case
| 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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
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 → |
| 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('\\', "/");
|
Claude finished @cloud9c's task in 4m 11s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit
What changed since the last review
Findings posted as inline commentsHigh severity — incorrect entrypoint selection in Defold flow
Medium severity
Low severity
Earlier findings still applicable but not re-postedTo avoid inline duplicates these still hold:
Suggested combined fix for findings #1, #2, #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::UNIX_EPOCH);
// Newest-wins design intact; nested-path match restored; one stat per file.
(std::cmp::Reverse(modified), architecture, relative)
}); |
| 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 .unwrap_or(html_path) silently leaks the 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. WalkDir returns absolute paths in some cases (e.g. when given an absolute root) and relative in others — if strip_prefix(&upload_dir) mismatches normalization, unwrap_or(html_path) falls back to the full path, sending an absolute filesystem path to the server as htmlPath. That leaks the local layout ($HOME/projects/<game>/...) and almost certainly produces a 4xx on the server.
Mirror the dev/mod.rs canonicalize step, or replace the fallback with an explicit error:
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('\\', "/");
|
Claude finished @cloud9c's task in 9m 16s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit
What changed since the prior review (commit
|
| Prior state | Latest state |
|---|---|
builds.rs canonicalized upload_dir before strip_prefix (dev parity) |
Removed in 1aa904f — described as "out of scope" |
The remaining state from prior passes is intact: explicit match arms in builds.rs, EngineKind::Defold.as_label() (no hardcoded "DEFOLD"), architecture_score matches anywhere in the path, unwrap_or(UNIX_EPOCH) on stat failure, and pre-computed sort keys.
Findings posted as inline comments
High severity — regression
src/builds.rs:171-175— Canonicalize was removed butstrip_prefix(&upload_dir).unwrap_or(html_path)(introduced by this PR) is still here. Astrip_prefixmismatch (symlinked path, macOS/var↔/private/var,..segments) silently falls through to the absolute path and sends e.g.htmlPath: /home/<dev>/projects/<game>/dist/...to the server.wavedash devis unaffected (still canonicalizes atdev/mod.rs:101), so the two flows now silently disagree onhtmlPathfor the same project. [r3337237169]
Medium severity
-
src/dev/entrypoint.rs:19-23— Rootindex.htmlshort-circuit pre-emptswasm-web/index.htmlfor Defold. The comment now declares the short-circuit is for "single-HTML exports (Godot/Unity)" but it isn't gated on engine kind. A leftover rootindex.html(previous engine, redirect page, hand-written launcher) silently steals selection for Defold. [r3337232704] -
src/dev/entrypoint.rs:71-77— "Newest export wins" makesarchitecture_scoreeffectively unreachable on ns-mtime filesystems. Two HTMLs from the samebob.jarinvocation almost never tie on ext4/APFS/NTFS, so the wasm-web preference branch only fires under coarse-mtime FSes. Ifbob.jarhappens to writejs-webafterwasm-web, every fresh full export inverts the documented preference. Worth empirically verifyingbob.jar's write order — or bucketing mtimes by second so the preference actually decides ties. [r3337233479] -
src/dev/entrypoint.rs:25-38— Unbounded walk + newest-wins lets unrelated HTML steal selection.WalkDirwith nomax_depthor skip-list traversesnode_modules/, asset readmes, vendored library demos. Any of these newer than the bundle (npm install timestamp, IDE coverage report, asset re-extraction) wins under the new comparator. Cap depth or filter common-noise directories. [r3337234316]
Low severity — carried over from prior passes
-
src/init.rs:51-52—default_upload_dir = "dist"is not a Defold convention. New comment explains why notbuild/(Defold's cache lives there), butdist/isn't wherebob.jarwrites either. Default tobuild/defaultor prompt during init. [r3337234853] -
src/init.rs:136-146—detect_defoldhas no version extraction or fallback heuristic.version_hintis alwaysNone; the prompt always falls through to the hardcoded"1.12.4"default which will go stale. [r3337235314]
Verdict
The PR was converging well at 99f8b5d. The latest commit (1aa904f) shipped a real regression by removing a canonicalize block that was added in the prior commit specifically to protect the new Defold-only strip_prefix call. Restoring it (or replacing the unwrap_or fallback with an explicit error) is the single most important change before merge. The medium-severity items are quality-of-life / robustness for Defold's bundle layout. Findings #5 and #6 are unchanged UX cliffs from prior passes.
• Branch: defold-first-class
| pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | ||
| let default_index = upload_dir.join("index.html"); | ||
| if default_index.is_file() { | ||
| return Some(default_index); | ||
| } |
There was a problem hiding this comment.
Root index.html short-circuit still pre-empts wasm-web/index.html for Defold.
The comment at lines 17–18 documents the intent: this short-circuit is for "single-HTML" exports (Godot/Unity) and the ranking below is "for nested multi-HTML Defold dists." But the short-circuit doesn't check engine kind — it runs for everyone. So in a Defold project:
- Any stale
index.htmlatupload_dir/(leftover from a previous engine, a redirect placed at the root, a hand-written launcher, a build-tool artifact, even an OS-Indexer-generated file) wins immediately. - The properly nested
MyGame/wasm-web/index.htmlis never considered. fetch_entrypoint_paramsthen parses the wrong HTML; the server returns params for a non-existent build, and the playtest URL renders blank / 404s on missing chunks.
For Defold migrations specifically this is a real footgun — a dist/index.html from a previous threejs/phaser setup silently steals the entrypoint. Gating on engine kind closes it cleanly:
pub fn locate_html_entrypoint(upload_dir: &Path, engine: Option<EngineKind>) -> Option<PathBuf> {
if !matches!(engine, Some(EngineKind::Defold)) {
let default_index = upload_dir.join("index.html");
if default_index.is_file() {
return Some(default_index);
}
}
// ... walk + sort for Defold (and as a fallback)
}Callers in dev/mod.rs:111 and builds.rs:165 would pass engine_kind through.
| @@ -28,16 +31,60 @@ | |||
| if entry.file_type().is_file() { | |||
| if let Some(ext) = entry.path().extension() { | |||
| if ext.eq_ignore_ascii_case("html") { | |||
| return Some(entry.into_path()); | |||
| html_files.push(entry.into_path()); | |||
| } | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Unbounded walk + "newest mtime wins" lets unrelated HTML files steal selection.
WalkDir::new(upload_dir).min_depth(1) with no .max_depth(...) and no skip-list traverses everything under upload_dir, including:
node_modules/(if anyone vendors deps into the upload dir)- asset-pack readmes (
docs/index.html,examples/foo.html) - engine demo HTML inside vendored libraries
.git/(no — WalkDir hits it; you'd need.filter_entryto skip)- IDE-generated coverage / report HTML
Under the new "newest wins" comparator (line 73-77), any of these with an mtime newer than the actual wasm-web/index.html wins selection. Concrete scenario:
upload_dir/
MyGame/wasm-web/MyGame.html # bob.jar build, mtime T
MyGame/js-web/MyGame.html # bob.jar build, mtime T+1ms
node_modules/foo/example.html # npm install at T+1h
locate_html_entrypoint returns node_modules/foo/example.html (newest). fetch_entrypoint_params reads that file and sends its contents as htmlContent to the server; the server returns nonsense params and the playtest is broken — with no diagnostic pointing at the cause.
Two ways to harden:
- Cap depth: Defold's deepest legitimate path under
upload_diris 2–3 levels (<GameTitle>/wasm-web/<file>.html)..max_depth(4)would prune most accidental candidates. - Filter common noise: skip
node_modules,.git,vendor,docs,examplesvia.filter_entry(|e| !is_noise(e.file_name())).
Either makes selection robust against the most common ways stray HTML lands under upload_dir.
| 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.
Regression in 1aa904f ("remove out of scope changes"): the canonicalize safety net for this strip_prefix was deleted.
Commit a291f10 ("path safety") added a canonicalize() block just above the file-staging step with the rationale "so a strip_prefix miss can't leak an absolute path as htmlPath (mirrors wavedash dev)." Commit 1aa904f removed that block, calling it out of scope — but the strip_prefix(&upload_dir).unwrap_or(html_path) right here was introduced by this same PR to produce htmlPath for Defold. So the canonicalize wasn't out of scope; it was the guard for new code this PR added.
What goes wrong now:
upload_dir = config_dir.join(&wavedash_config.upload_dir)is unnormalized. With a symlinked path (macOS/var↔/private/var,/tmpsymlinks),..segments, or platform-specific normalization,WalkDircan emit entries whose textual prefix doesn't matchupload_direxactly.strip_prefix(&upload_dir)returnsErr;.unwrap_or(html_path)falls through to the full absolute path.- The server receives
htmlPath: "/home/<dev>/projects/<game>/dist/MyGame/wasm-web/MyGame.html"(leaks the dev's layout) instead ofMyGame/wasm-web/MyGame.html, and almost certainly produces a 4xx because nothing under R2 is keyed that way. wavedash devis unaffected (it still canonicalizes atdev/mod.rs:101), so the two flows silently disagree onhtmlPathfor the same project.
Two fixes; either is fine:
Option A — restore the canonicalize block above FileStaging::prepare (mirrors dev):
let upload_dir = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize upload_dir: {}", upload_dir.display()))?;(also restore use anyhow::{Context, Result}; at the top of the file.)
Option B — replace the unwrap_or fallback here with an explicit error so a mismatch surfaces loudly instead of leaking a path:
| let html_relative_path = html_path | |
| .strip_prefix(&upload_dir) | |
| .unwrap_or(html_path) | |
| .to_string_lossy() | |
| .replace('\\', "/"); | |
| 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('\\', "/"); |
|
Claude finished @cloud9c's task in 9m 4s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit Posted 5 inline findings, ranked most-severe first.
What changed since the last review
Findings postedHigh severity
Medium severity
Low severity
VerdictThe pivot to explicit |
| Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => { | ||
| let engine_label = kind.as_label(); | ||
| let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "No HTML file found in upload_dir; required for {} builds", | ||
| engine_label | ||
| ) | ||
| })?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?; | ||
| Some(fetch_entrypoint_params(engine_label, ver, html_path).await?) | ||
| let html_relative_path = html_path | ||
| .strip_prefix(&upload_dir) | ||
| .unwrap_or(&html_path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| Some( | ||
| fetch_entrypoint_params(engine_label, ver, &html_path, Some(&html_relative_path)) | ||
| .await?, | ||
| ) | ||
| } |
There was a problem hiding this comment.
htmlPath is now sent for Godot/Unity uploads — explicitly flagged by @calvinstudebaker on the prior pass: "let's not change the shape of Unity or Godot uploads. Keep the new Defold logic isolated to the Defold case."
The Godot/Unity branch (lines 113–132) computes html_relative_path and passes Some(&html_relative_path) into fetch_entrypoint_params. Combined with fetch_entrypoint_params always serializing "htmlPath": … unconditionally, the JSON request body for established Godot/Unity callers grew a new htmlPath field that wasn't there pre-PR — exactly the contract change the maintainer asked to avoid.
Suggested isolation: keep locate_html_entrypoint use for Godot/Unity, but pass None for html_relative_path (and have fetch_entrypoint_params skip the field when None — see separate comment at entrypoint.rs:91). Concretely:
Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => {
let engine_label = kind.as_label();
let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| {
anyhow::anyhow!(
"No HTML file found in upload_dir; required for {} builds",
engine_label
)
})?;
let ver = engine_version
.ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?;
Some(fetch_entrypoint_params(engine_label, ver, &html_path, None).await?)
}This also makes the strip_prefix(&upload_dir).unwrap_or(&html_path) block at lines 123–127 unnecessary (and removes its silent-leak fallback in the case strip_prefix ever fails).
| pub fn resolve_defold_entrypoint( | ||
| upload_dir: &Path, | ||
| entrypoint: Option<&str>, | ||
| ) -> Result<(PathBuf, String)> { | ||
| let entrypoint = entrypoint.ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Defold builds need an `entrypoint` in wavedash.toml pointing to your HTML5 export, e.g.\n entrypoint = \"wasm-web/<game>/index.html\"\n(a Defold bundle can contain both wasm-web/ and js-web/ — pick one)" | ||
| ) | ||
| })?; | ||
| let relative_path = entrypoint.replace('\\', "/"); | ||
| let html_path = upload_dir.join(&relative_path); | ||
| if !html_path.is_file() { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` not found under {}. Point `entrypoint` in wavedash.toml at your export's index.html.", | ||
| entrypoint, | ||
| upload_dir.display() | ||
| ); | ||
| } | ||
| Ok((html_path, relative_path)) | ||
| } |
There was a problem hiding this comment.
resolve_defold_entrypoint joins a user-controlled string straight into upload_dir with no normalization or upload-dir containment check — the entrypoint from wavedash.toml can escape the build directory via .. segments or be an absolute path.
Two concrete misconfigurations:
-
entrypoint = "../../something/index.html"inwavedash.toml→upload_dir.join("../../something/index.html")resolves aboveupload_dir. If that file happens to exist,is_file()passes;fs::read_to_string(&html_path)reads its content; andhtml_relative_path = "../../something/index.html"is sent verbatim to the server ashtmlPath. The build then references assets via that escape path on the playsite — almost certainly serving a broken bundle. -
entrypoint = "/Users/dev/somethingelse/index.html"→ Rust'sPath::joinwith an absolute argument replaces the receiver, soupload_dir.join("/Users/dev/…")is just/Users/dev/….is_file()andread_to_stringboth succeed on the absolute path;html_relative_path = "/Users/dev/…"leaks the dev's filesystem layout to the server ashtmlPath.
The user "owns their own machine" so this isn't a security exploit, but it surfaces typos and copy-paste mistakes as silent wrong-file uploads instead of clear errors. A cheap guard: canonicalize both and check containment:
let html_path = upload_dir.join(&relative_path);
if !html_path.is_file() {
anyhow::bail!(/* same as today */);
}
// Reject anything that escapes upload_dir.
let canon_html = html_path
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", html_path.display()))?;
let canon_upload = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", upload_dir.display()))?;
if !canon_html.starts_with(&canon_upload) {
anyhow::bail!(
"Defold entrypoint `{}` resolves outside upload_dir ({}). The path must be relative to upload_dir and stay inside it.",
entrypoint,
upload_dir.display()
);
}Bonus: also reject paths whose lowercase extension isn't .html / .htm — FileStaging::prepare does this for Custom builds but skips Defold entirely because wavedash_config.entrypoint() returns None for engines.
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| "htmlPath": html_relative_path, |
There was a problem hiding this comment.
htmlPath is serialized unconditionally — Option<&str> with None produces "htmlPath": null in the JSON body, not an omitted field. Both halves of the contract change for Godot/Unity (carrying the field at all, and emitting it as null rather than skipping) are encoded here — so even after dev/mod.rs is updated to pass None for Godot/Unity, the request body would still carry "htmlPath": null.
To make htmlPath truly Defold-only, build the JSON body conditionally before posting:
let mut body = serde_json::json!({
"engine": engine,
"engineVersion": engine_version,
"htmlContent": html_content,
});
if let Some(html_path) = html_relative_path {
body["htmlPath"] = serde_json::json!(html_path);
}
let response = client.post(&endpoint).json(&body).send().await
.with_context(|| "Failed to call CLI entrypoint params endpoint")?;Combined with the dev/mod.rs change to pass None for Godot/Unity, Godot/Unity requests go back to the pre-PR three-field shape, and only Defold sends htmlPath.
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||
| Some( | ||
| fetch_entrypoint_params( | ||
| EngineKind::Defold.as_label(), | ||
| ver, | ||
| &html_path, | ||
| Some(&html_relative_path), | ||
| ) | ||
| .await?, | ||
| ) | ||
| } |
There was a problem hiding this comment.
FileStaging::prepare at line 153 doesn't validate the Defold entrypoint because wavedash_config.entrypoint() returns None for any engine (see config.rs:292-301: the method only returns the entrypoint when no engine is configured). For Custom builds, FileStaging::prepare enforces .html/.htm/.js extension and file existence. For Defold, both checks are skipped, and validation happens later inside resolve_defold_entrypoint (line 60), which only checks is_file() — extension is never validated.
Result: a user who sets entrypoint = "wasm-web/<game>/something.png" (typo or wrong asset) sails through FileStaging::prepare, gets to resolve_defold_entrypoint which passes is_file(), then fetch_entrypoint_params reads a binary file and POSTs it as htmlContent. The server most likely returns a parse error; the user sees an opaque API failure instead of "Entrypoint must be an HTML file".
Two ways to close this:
- Move the Defold validation into
FileStaging::prepareso it mirrors the Custom path (check extension + existence early, with a clean error message). - Or have
resolve_defold_entrypointitself validate the extension before reading the file.
Either way, the gap is in FileStaging::prepare — wavedash_config.entrypoint() (the method) silently filters Defold out of validation, while wavedash_config.entrypoint (the field) is what actually drives Defold behavior at lines 166 and dev/mod.rs:135. Two access patterns for the same field that can drift.
| EngineType::Defold => { | ||
| let version = engine_version.unwrap_or("1.12.4"); | ||
| // `entrypoint` is a top-level key, so the hint must precede the [defold] | ||
| // table. Left commented — the dev fills in the export they want to ship; | ||
| // `wavedash dev` / `build push` errors clearly until they do. | ||
| toml.push_str( | ||
| "\n# Defold HTML5 bundles can contain both wasm-web/ and js-web/ folders.\n# Set `entrypoint` (top-level) to the index.html you want to ship, e.g.:\n# entrypoint = \"wasm-web/<game>/index.html\"\n", | ||
| ); | ||
| toml.push_str(&format!("\n[defold]\nversion = \"{}\"\n", version)); | ||
| } |
There was a problem hiding this comment.
wavedash init for Defold writes a comment-only entrypoint hint, not an actual entrypoint = … line. A user who runs init and then immediately tries wavedash dev / build push hits the error from resolve_defold_entrypoint ("Defold builds need an entrypoint in wavedash.toml…") before anything works. There's no signal during init that more editing is required.
The error message is clear once you trigger it, but the UX cliff is:
wavedash init— completes successfully, prints "Created wavedash.toml! Next steps: → Runwavedash devto test locally".- User runs
wavedash dev— errors out becauseentrypointis missing. - User opens
wavedash.toml, sees a hint in a comment, fills it in.
Two cheaper alternatives, in order of effort:
- Tell the user during
init("Defold requires anentrypoint— edit wavedash.toml before runningwavedash dev") viacliclack::log::infoor a prefilled-but-required prompt. - Better: actually prompt for the entrypoint during
initso the producedwavedash.tomlworks end-to-end on a clean run. The path is dynamic (wasm-web/<GameTitle>/index.html) but the user knows it; the same prompt that asks forupload_dircan ask forentrypointfor Defold.
Right now wavedash init for Defold is the only engine where the next step the outro promises doesn't work without manual file editing. Worth at least an info line so the user isn't surprised.
|
Claude finished @cloud9c's task in 12m 16s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit
What changed since the last review
Findings postedMedium severity
Low severity
VerdictThe pivot to explicit |
| let entrypoint_params = match engine_kind { | ||
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; |
There was a problem hiding this comment.
For Defold, resolve_defold_entrypoint runs after FileStaging::prepare (line 153) and scan_directory (line 156) — both of which are no-ops for catching a missing/invalid Defold entrypoint, so the user pays for a full directory walk before getting a clear error.
Trace for a user who ran wavedash init and hasn't yet edited entrypoint into their wavedash.toml:
- Line 153 —
FileStaging::prepare(&upload_dir, &wavedash_config)callswavedash_config.entrypoint()(the method atconfig.rs:292-301), which returnsNonefor any non-Noneengine_type. Defold entrypoint is not validated. - Line 156 —
scan_directory(&upload_dir)walks the entireupload_dir, hashing each file. On a real Defold bundle withwasm-web/+js-web/+ assets, this is the slow step. - Line 167-190 — only now does the
Some(EngineKind::Defold)arm callresolve_defold_entrypoint(...)and finally surface the "Defold builds need anentrypoint…" error.
For dev/mod.rs (handle_dev), there's no scan_directory step, so the cost is small. For handle_build_push, the scan is meaningful work to throw away.
Moving resolve_defold_entrypoint (or at minimum the entrypoint existence check) above FileStaging::prepare and scan_directory makes the error fast and matches the contract that FileStaging::prepare advertises for Custom engines (validate-before-upload).
| // Get temporary R2 credentials (includes build size) | ||
| let engine_kind = wavedash_config.engine_type()?; | ||
| let entrypoint = match engine_kind { | ||
| Some(EngineKind::Defold) => wavedash_config.entrypoint.as_deref(), |
There was a problem hiding this comment.
Slash-normalization drift between the entrypoint field and entrypointParams.htmlPath sent to the server.
resolve_defold_entrypoint deliberately normalizes \\ → / (entrypoint.rs:58) — the maintainer anticipated users writing Windows-style paths in wavedash.toml. That normalized value lands in entrypointParams.htmlPath.
But at line 163 (and the parallel dev/mod.rs:108), the top-level entrypoint field uses wavedash_config.entrypoint.as_deref() — the raw unmodified string. So if a user writes:
entrypoint = "wasm-web\\game\\index.html"the server receives "entrypoint": "wasm-web\\game\\index.html" and "entrypointParams": { "htmlPath": "wasm-web/game/index.html" }. Two different shapes for the same file; whichever the server uses for resolution decides which is correct.
It's a narrow case (most users will type forward slashes), but the back-slash handling in resolve_defold_entrypoint exists specifically for users who don't — and the inconsistency negates that effort.
Cheapest fix: reuse the normalized html_relative_path returned by resolve_defold_entrypoint as the top-level entrypoint field, so both fields agree on slashes. Concretely, hoist the resolve call so its html_relative_path is in scope for both the entrypoint binding and the entrypoint_params arm.
| } | ||
|
|
||
| // Get temporary R2 credentials (includes build size) | ||
| let engine_kind = wavedash_config.engine_type()?; |
There was a problem hiding this comment.
upload_dir is still not canonicalized in handle_build_push — only resolve_defold_entrypoint's internal canonicalize-containment check runs, while dev/mod.rs:101 canonicalizes once and the rest of the flow operates on a clean absolute path.
This was raised in a prior review and is moot for path-escape safety — resolve_defold_entrypoint re-canonicalizes both sides before its starts_with check (entrypoint.rs:92-104), so escapes are rejected. But the broader symmetry concern still applies:
- The returned
html_pathat entrypoint.rs:106 is the un-canonicalizedupload_dir.join(relative_path). It's read viafs::read_to_string(fine), but it flows intofetch_entrypoint_paramsand any error messages downstream — the two flows disagree on the textual path for the same project. - Error messages and diagnostic output differ between
wavedash devandwavedash build pushfor the same misconfiguration —devreports a/private/var/...absolute path;build pushreports./build/default/.... That asymmetry makes user-reported failures harder to triage. R2Uploaderat line 215 walks the non-canonical path; pre-PR behavior, but worth confirming the new Defold flow doesn't introduce sensitivity to symlinks underupload_dir.
Restoring the canonicalize block (the same one added in a291f10 and removed in 1aa904f) is the simplest fix and keeps the two flows symmetric:
let upload_dir = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize upload_dir: {}", upload_dir.display()))?;(requires restoring use anyhow::Context;.)
| #[test] | ||
| fn resolves_defold_entrypoint_inside_upload_dir() { | ||
| let upload_dir = temp_upload_dir("inside"); | ||
| let html_path = upload_dir.join("wasm-web/example/index.html"); | ||
| fs::create_dir_all(html_path.parent().expect("html parent")).expect("create dirs"); | ||
| fs::write(&html_path, "<html></html>").expect("write html"); | ||
|
|
||
| let (resolved, relative) = | ||
| resolve_defold_entrypoint(&upload_dir, Some("wasm-web/example/index.html")) | ||
| .expect("resolve entrypoint"); | ||
|
|
||
| assert_eq!(resolved, html_path); | ||
| assert_eq!(relative, "wasm-web/example/index.html"); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_defold_entrypoint_escape() { | ||
| let upload_dir = temp_upload_dir("escape"); | ||
| fs::create_dir_all(&upload_dir).expect("create upload dir"); | ||
|
|
||
| let err = resolve_defold_entrypoint(&upload_dir, Some("../index.html")) | ||
| .expect_err("escape should fail"); | ||
|
|
||
| assert!(err.to_string().contains("relative path inside upload_dir")); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_defold_entrypoint_non_html() { | ||
| let upload_dir = temp_upload_dir("non-html"); | ||
| let asset_path = upload_dir.join("wasm-web/example/something.png"); | ||
| fs::create_dir_all(asset_path.parent().expect("asset parent")).expect("create dirs"); | ||
| fs::write(&asset_path, "not html").expect("write asset"); | ||
|
|
||
| let err = resolve_defold_entrypoint(&upload_dir, Some("wasm-web/example/something.png")) | ||
| .expect_err("non-html entrypoint should fail"); | ||
|
|
||
| assert!(err.to_string().contains("HTML file")); | ||
|
|
||
| fs::remove_dir_all(upload_dir).expect("cleanup"); | ||
| } |
There was a problem hiding this comment.
Test cleanup leaks temp directories if any assertion before fs::remove_dir_all panics.
Each test ends with fs::remove_dir_all(upload_dir).expect("cleanup") — but if expect_err, assert_eq!, or assert! panics earlier (e.g., if resolve_defold_entrypoint's behavior regresses), the cleanup line never executes and the temp directory remains under /tmp/wavedash-cli-entrypoint-* indefinitely. On a CI loop that re-runs the test suite, this accumulates.
This won't bite production users — only test infrastructure — but it makes it harder to diagnose flaky-test situations on a long-lived runner.
tempfile::TempDir (already a common Rust testing crate) handles RAII cleanup on Drop so panics don't leak. If pulling in another dev-dependency is heavier than wanted, a manual Drop guard struct around PathBuf works too. Minor / quality-of-life only.
| "\n# Defold HTML5 bundles can contain both wasm-web/ and js-web/ folders.\n# Set `entrypoint` (top-level) to the index.html you want to ship, e.g.:\n# entrypoint = \"wasm-web/<game>/index.html\"\n", | ||
| ); | ||
| toml.push_str(&format!("\n[defold]\nversion = \"{}\"\n", version)); | ||
| } |
There was a problem hiding this comment.
wavedash init for Defold still produces a TOML where the next step (wavedash dev) errors out — the new comment acknowledges it but the cliff is unchanged.
The comment at lines 256–258 documents the design ("the dev fills in the export they want to ship; wavedash dev / build push errors clearly until they do"), and the strict "no inference" rule in resolve_defold_entrypoint is sound. But the resulting UX:
wavedash initcompletes successfully and the outro at line 472-475 promises "Next steps: → Runwavedash devto test locally"- The user runs
wavedash dev - It errors with the long "Defold builds need an
entrypoint…" message from entrypoint.rs:53-57 - The user opens
wavedash.toml, finds a#-prefixed hint, edits the file
This is the only engine where the next step the outro promises doesn't work without manual file editing. Two cheap mitigations, in order of effort:
- Surface a
cliclack::log::infoline during init telling the user to editwavedash.tomlbefore runningwavedash dev. Costs ~3 lines, fully removes surprise. - Prompt during init: add a
cliclack::input("Defold HTML5 export path")with a placeholder likewasm-web/<game>/index.htmland emit the line uncommented. Costs ~10 lines, letswavedash devwork end-to-end after init.
Either is fine — the goal is just to make the cliff visible before the user hits it. Acknowledged in the previous review pass; flagging once more since the latest commit ("edge cases") had room to address it.
|
Claude finished @cloud9c's task in 6m 38s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit
What changed since the last review
New inline findingLow severity — only one, on the smallest of the remaining items.
Carried over but not re-postedTo avoid inline duplicates, these stand:
New minor observation (not posted inline)
Missing test coverage worth considering
VerdictThe PR has reached a good landing spot. High-severity findings from prior passes are resolved or mooted. The pivot to explicit |
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 d852111. Configure here.
| if detected.engine_type == EngineType::Defold { | ||
| cliclack::log::info( | ||
| "Set `entrypoint` in wavedash.toml to your Defold HTML export before running `wavedash dev`.", | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Closes most of the UX cliff for Defold, but the outro that follows still contradicts the info.
The new info (lines 472–474) warns the user to set entrypoint before running wavedash dev — good. But the very next call (line 478, unchanged in this PR) is:
cliclack::outro(format!(
"Created wavedash.toml! Next steps:\n → Run `wavedash dev` to test locally\n ..."
));So a Defold user sees a small info line, then an outro promising "Next steps: → Run wavedash dev" — which the info has just told them won't work yet. The outro is the visually dominant message, and a user who skims will act on its promise and hit the resolve_defold_entrypoint error.
Two cheap follow-ups, in order of effort (not blockers):
- Conditionalize the outro text for Defold so
Edit entrypoint in wavedash.tomlis step 1 andRun wavedash devis step 2 — keeps the next-steps list honest. - Prompt during init for the Defold entrypoint (placeholder
wasm-web/<game>/index.html) and emit an uncommentedentrypoint = …line; then the outro's claim is actually true on a clean run.
The current state is a real improvement over the prior commit; just calling out that the surface area is now small but still present.
|
Claude finished @cloud9c's task in 6m 54s —— View job Code Review — Extra-high effort (recall mode)Reviewed at extra-high effort against commit
What changed since the last reviewThis commit makes two changes:
Why no new inline findingsI went through the diff at five angles (line-by-line, removed-behavior, cross-file tracer, Rust pitfalls, edge-case sweep) and verified each candidate:
Carried over from prior passes (not re-posted to avoid duplication)
VerdictThe PR has fully converged. The latest commit was a small, correct, well-tested polish round. I would not block on any remaining item. The Defold pivot from heuristics to explicit |
2 participants
Note
Medium Risk
Changes build/dev upload metadata and entrypoint resolution for a new engine; mistakes could ship the wrong HTML export or break Defold local/play flows until
entrypointis set correctly.Overview
Adds Defold as a first-class engine across
wavedash init,wavedash dev, andbuild push.Defold HTML5 exports can include both
wasm-web/andjs-web/trees, so the CLI no longer guesses an HTML file for Defold. Developers must set a top-levelentrypointinwavedash.toml(e.g.wasm-web/<game>/index.html).resolve_defold_entrypointvalidates that path (relative,.html, exists underupload_dir, no path escape) and uses the normalized relative path when talking to the API.wavedash initdetectsgame.project, defaultsupload_dirtodist, writes a[defold]section, and documents the requiredentrypoint. Godot/Unity still uselocate_html_entrypoint; theirfetch_entrypoint_paramscalls are unchanged aside from an optionalhtmlPathfield sent for Defold.build pushanddevshare the same Defold branch: resolved entrypoint + server-derivedentrypointParamsfrom/cli/entrypoint-params(HTML content plushtmlPath).Reviewed by Cursor Bugbot for commit fb68137. Bugbot is set up for automated code reviews on this repo. Configure here.