Skip to content

refactor(executor): share the yes/no confirmation between the setup steps - #542

Open
moha-bekh wants to merge 1 commit into
mainfrom
cod-3537-share-the-yesno-confirmation-prompt-between-the-setup-steps
Open

moha-bekh wants to merge 1 commit into
mainfrom
cod-3537-share-the-yesno-confirmation-prompt-between-the-setup-steps

Conversation

@moha-bekh

Copy link
Copy Markdown
Member

Two setup steps ask before installing something on the user's machine: the valgrind-codspeed source build, for the distributions we publish no package for, and the Homebrew bash samply needs on macOS, since the Apple-signed /bin/bash cannot be profiled. Each carried its own copy of the same logic — skip the question with no TTY, print the explanation and the question on stderr, read a line, count an empty answer as a yes.

Both now go through confirm_default_yes, taking the valgrind prompt as the reference implementation. The only behaviour change is on macOS: the question is asked with the progress bar suspended, so a running spinner can no longer overwrite it. The valgrind caller still decides on CODSPEED_VALGRIND_BUILD_FROM_SOURCE first and treats a decline as a legitimate answer; the macOS one still fails on a decline.

The macOS prompt was not exercised by hand — macos-basic-run-test covers the compile, and its runner has no TTY, so it takes the accept-without-asking path.

Closes COD-3537

@codspeed

codspeed Bot commented Sep 21, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 23 untouched benchmarks


Comparing cod-3537-share-the-yesno-confirmation-prompt-between-the-setup-steps (aebd177) with main (7150303)

Open in CodSpeed

…teps

Two setup steps ask before installing something on the user's machine: the
valgrind-codspeed source build, for the distributions we publish no package
for, and the Homebrew bash samply needs on macOS, since the Apple-signed
/bin/bash cannot be profiled.

Each carried its own copy of the same logic: skip the question with no TTY,
print the explanation and the question on stderr, read a line, count an empty
answer as a yes. Move both onto confirm_default_yes, taking the valgrind prompt
as the reference implementation.

The macOS question is now asked with the progress bar suspended, so a running
spinner can no longer overwrite it. Everything else keeps its shape: the
valgrind caller still decides on CODSPEED_VALGRIND_BUILD_FROM_SOURCE first and
treats a decline as a legitimate answer, the macOS one still fails on a
decline.

Closes COD-3537
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@moha-bekh
moha-bekh force-pushed the cod-3537-share-the-yesno-confirmation-prompt-between-the-setup-steps branch from e9e8ff6 to aebd177 Compare September 21, 2026 11:07
@moha-bekh
moha-bekh marked this pull request as ready for review September 21, 2026 11:16
@greptile-apps

greptile-apps Bot commented Sep 21, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The behavioral refactor appears sound, but the explicit repository requirement against direct stderr writes must be satisfied before merging.

Fix All in Claude CodeFindings

  1. P2 Direct stderr writes
Fix with agent prompt
### Issue 1
src/executor/helpers/confirm.rs:12-13
The shared helper prints the explanation and question with `eprintln!` and `eprint!`. This violates the repository directive that Rust code must use tracing macros such as `info!`, `debug!`, `warn!`, or `error!` instead of writing directly to stdout or stderr. This repository requirement must be satisfied before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR extracts the duplicated default-yes setup confirmation into a shared helper and uses it for both the valgrind source build and macOS Homebrew bash installation.

  • Preserves the callers’ existing non-interactive and decline behavior.
  • Suspends the progress bar while an interactive confirmation is displayed.
  • Introduces direct stderr writes that conflict with an explicit repository rule.

Reviews (1) · Last reviewed commit: "refactor(executor): share the yes/no con..."

Comment thread src/executor/helpers/confirm.rs
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.

1 participant