Skip to content

require confirmation before wavedash publish - #54

Merged
The64thRealm merged 1 commit into
mainfrom
publish-confirm-yes
Aug 12, 2026
Merged

require confirmation before wavedash publish#54
The64thRealm merged 1 commit into
mainfrom
publish-confirm-yes

Conversation

@The64thRealm

Copy link
Copy Markdown
Contributor

Publishing makes a build live for players, so gate it behind the same confirmation clear-playtest-data uses: an interactive cliclack prompt that defaults to No, -y / --yes / --force to skip it, and a refusal when stdin is not a terminal (or CI is set) and there is no prompt to answer.

Note this makes -y mandatory for scripted/agent publishes, which is the point — an unattended run can no longer make a build live by accident.

Publishing makes a build live for players, so gate it behind the same
confirmation `clear-playtest-data` uses: an interactive cliclack prompt
that defaults to No, `-y` / `--yes` / `--force` to skip it, and a refusal
when stdin is not a terminal (or CI is set) and there is no prompt to
answer.

Note this makes `-y` mandatory for scripted/agent publishes, which is the
point — an unattended run can no longer make a build live by accident.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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 bugs or issues to report.

Open in Devin Review

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 3m 56s —— View job


Code review

The gate itself is correct and faithfully mirrors the clear-playtest-data precedent (src/clear_playtest_data.rs:104-127): same is_non_interactive() check, same initial_value(false), same abort message shape. handle_publish has exactly one call site (src/main.rs:650), so the new PublishArgs.yes field can't leave a construction site behind. Hoisting game_id to src/publish.rs:114 is a net improvement — WavedashConfig::game_id() prints the WAVEDASH_GAME_ID override notice on first read, so the user now sees which game they're publishing to before the prompt rather than after it, and the borrow lives fine to src/publish.rs:148.

One inline comment posted. Everything below is smaller.

Nits

No test pins the new CLI surface. src/main.rs::tests has an established pattern of walking Cli::command() to assert invariants across every subcommand (every_game_id_arg_rejects_a_blank_value at src/main.rs:830, upload_source_is_hidden_and_only_offers_the_godot_plugin at src/main.rs:880). A walk-style test asserting that every command guarded by is_non_interactive() exposes the same skip flag and aliases would have caught the naming divergence in the inline comment, and would keep the next such command consistent. Cli::try_parse_from(["wavedash", "publish", "b1", "-y"]) for each of -y / --yes / --force is the cheap version. Fix this →

Declining exits 0. src/publish.rs:138-141 returns Ok(()) on "no", so a wrapper can't distinguish "published" from "user said no" by exit code. Only reachable interactively, and it matches src/clear_playtest_data.rs:123-126, so it's arguably correct to leave alone — but if you ever change it, change both.

Warning goes to stdout, prompt goes to stderr. println! at src/publish.rs:129 vs cliclack rendering to stderr: with stdout redirected and stdin still a tty, the user sees a bare "Are you sure you want to continue?" with no build/game context. Same pre-existing split as clear-playtest-data; noting it, not asking for a change here.

On the breaking change

is_non_interactive() is CI set or stdin not a tty (src/main.rs:477-479), so this hard-fails any existing unattended wavedash publish — release pipelines, the Godot plugin if it shells out, agent runs — until each adds -y. The PR body says that's the point and I agree with the tradeoff. Since this ships in a released binary and there's no CHANGELOG in the repo, it's worth making sure the release notes for the version that carries this call it out; the error text at src/publish.rs:123-126 is actionable, which covers the person who hits it live, but not the person planning an upgrade.

Not verified

I could not compile: cargo check / cargo clippy / cargo fmt --check were blocked by tool permissions in this environment (and build.rs panics without SITE_HOST / CONVEX_HTTP_URL / PLAYSITE_HOST). My read is that it builds — game_id is &str from WavedashConfig::game_id() -> Result<&str> so .bold() resolves through impl Colorize for &str, build_id.bold() borrows rather than moves so the later uses at src/publish.rs:148 and :165 are fine, and visible_alias on an arg is already used at src/main.rs:172 — but CI should be the judge. Add Bash(cargo *) to --allowedTools if you want me to actually run it next time.

  • Gather context (diff, repo conventions, clear-playtest-data precedent)
  • Review correctness & consistency
  • Post findings
    · branch publish-confirm-yes

Comment thread src/main.rs
Comment on lines +127 to +133
#[arg(
long = "yes",
short = 'y',
visible_alias = "force",
help = "Skip confirmation (required when non-interactive)"
)]
yes: bool,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Flag naming is inverted relative to clear-playtest-data, and --force is now overloaded three ways.

clear-playtest-data (src/main.rs:218-224) declares long = "force", short = 'y', visible_alias = "yes"; this declares the mirror image. Both accept all three spellings, so nothing breaks — but the help output disagrees:

  • wavedash clear-playtest-data --help-y, --force [aliases: yes]
  • wavedash publish --help-y, --yes [aliases: force]

Separately, --force already means something different elsewhere in the CLI: on stat delete (src/main.rs:365) and achievement delete (src/main.rs:463) it means "proceed even though user progress is attached", not "skip the confirmation prompt" — and neither of those exposes -y. Making --force a visible alias here pushes a third meaning into the same word.

--yes reads better for publish (it isn't destructive, just consequential), so I'd keep it canonical and demote the alias to a hidden one — compatibility for anyone who reaches for --force out of habit, without advertising the overloaded name:

Suggested change
#[arg(
long = "yes",
short = 'y',
visible_alias = "force",
help = "Skip confirmation (required when non-interactive)"
)]
yes: bool,
#[arg(
long = "yes",
short = 'y',
alias = "force",
help = "Skip confirmation (required when non-interactive)"
)]
yes: bool,

Flipping clear-playtest-data to match is the other reasonable direction — the point is that the two confirmation gates should read the same way.

Copy link
Copy Markdown
Member

niceee thank you!

@The64thRealm
The64thRealm merged commit 13f83e9 into main Aug 12, 2026
12 of 13 checks passed
@The64thRealm
The64thRealm deleted the publish-confirm-yes branch August 12, 2026 21:06
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