Skip to content

cli attaches upload source - #50

Merged
The64thRealm merged 1 commit into
mainfrom
build-upload-source
Aug 7, 2026
Merged

cli attaches upload source#50
The64thRealm merged 1 commit into
mainfrom
build-upload-source

Conversation

@The64thRealm

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 2m 41s —— View job


Code Review

  • Read the PR diff (git diff origin/main...HEAD)
  • Review changed files in context (src/main.rs, src/config.rs, src/builds.rs, src/dev/mod.rs)
  • Verify findings
  • Post results

No blocking findings. I posted no inline comments because nothing survived verification as a real defect.

What I checked

  • Coverage of build-creating paths. uploadSource is attached at both endpoints that create a build row — builds/create-temp-r2-creds (src/builds.rs:63) and builds/create-local (src/dev/mod.rs:34). Grepping /builds/ finds only two others, upload-completed and publish, which operate on an existing build id, so coverage is complete. The ["wavedash build push", "wavedash dev"] assertion in src/main.rs:905 is a good guard against a future build-creating command forgetting the flag.
  • #[value(skip)] + Default on Cli. With clap 4.5's derive, a skipped variant is excluded from value_variants() and returns None from to_possible_value(), so Cli is unreachable from argv and can't be used with default_value_tOption<UploadSource> + unwrap_or_default() at src/main.rs:614/:628 is the right shape, not an accidental one. WEB being absent from the enum entirely (rather than validated) means the CLI structurally can't claim it.
  • Parse tests. build push has default_value = "./wavedash.toml" for --config (src/main.rs:250), so bare ["wavedash","build","push"] really does parse and the None-defaults-to-Cli assertion is meaningful. ignore_case is off, so the "CLI"/"GODOT_PLUGIN"/"" rejections at src/main.rs:944 genuinely exercise case-sensitive matching rather than passing trivially.
  • Threading. No other match arm destructures Commands::Dev or BuildCommands::Push, and no other caller of handle_dev/handle_build_push/create_local_build exists, so the added parameters are fully wired.

Non-blocking notes

  • The #[arg(...)] block is duplicated verbatim at src/main.rs:90-96 and :255-261. A shared #[derive(Args)] struct would deduplicate it, but the hidden-and-only-godot-plugin test asserts both sites, so drift would fail loudly — fine to leave as is.
  • uploadSource is sent unconditionally, unlike the other optional body fields. That's the correct consequence of having a default, but it does mean a CLI on this commit sends the field to whatever API version the user's install happens to talk to. Given the CLI self-updates, worth confirming the API accepts uploadSource in every environment before this ships (and that omitting it still defaults server-side for older installs). Only you can verify the server side from here.
  • The body-construction change itself isn't covered by a test — the tests cover clap wiring and the label strings. There's no HTTP-mock dependency in Cargo.toml, so there's no existing pattern to follow, and adding one for this seems disproportionate.

Verification caveat

I could not compile or run the test suite: doppler isn't installed in this runner, and cargo invocations weren't permitted in this sandbox. Everything above is from reading the code — please rely on CI for cargo check/clippy/test results.
· branch build-upload-source

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@The64thRealm
The64thRealm merged commit e3f75fd into main Aug 7, 2026
11 checks passed
@The64thRealm
The64thRealm deleted the build-upload-source branch August 7, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants